From 8f0a2d276f022bfc081c427b76a50c84b253520d Mon Sep 17 00:00:00 2001 From: Aaron Ware Date: Fri, 14 Aug 2026 14:36:10 -0400 Subject: [PATCH] fix(NO-TASK): Make SlowOrderBy work and open the PHP target SlowOrderBySniff has never worked, for two independent reasons. It called `$this->addMessage()`, the shape WPCS 2 offered on WordPressCS\WordPress\Sniff. WPCS 3 removed it, so the call resolved to nothing on the sniff or its parent and the sniff fatalled the moment it found something to report - "Call to undefined method Linchpin\Sniffs\Performance\SlowOrderBySniff::addMessage()". SlowMetaQuerySniff hit the same wall and grew a MessageHelper shim; this one never did, so consuming projects excluded it outright. linchpin.com carries exactly that exclusion. And it could not have matched anything anyway. The parent builds the value from raw tokens, so a literal arrives as `'rand'` with its quotes, and the switch compared against `rand`. Stripping the quotes is what makes the sniff actually fire. Rather than re-adding a shim, the callback now returns true and lets the parent emit. That is how AbstractArrayAssignmentRestrictionsSniff is designed to be used, and it drops the $stackPtr property and the process_token() override that existed only to carry a token pointer to the manual call - the parent reports against the key's own token, which is a more accurate position. The group message becomes %2$s because the parent passes `array( $key, $value )`, so %s is the literal "orderby" rather than the value worth naming. Verified against a fixture: three warnings for rand, meta_value and meta_value_num, each naming the value, and `date` correctly ignored. testVersion also becomes open-ended. A closed upper bound goes stale silently - it said 8.2-8.4 while linchpin.com had already moved to PHP 8.5, so PHPCompatibility was checking that project against the wrong target and the project had to override this line in its own ruleset to get correct results. "8.2-" means 8.2 and above. Note the package's own `composer test` is red on main already, before this change: the fixture tests fail to load the ruleset with "Class WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff not found". That is a harness autoloading problem, unrelated and untouched here. --- .../Sniffs/Performance/SlowOrderBySniff.php | 65 +++++++++---------- Linchpin/ruleset.xml | 17 ++++- 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/Linchpin/Sniffs/Performance/SlowOrderBySniff.php b/Linchpin/Sniffs/Performance/SlowOrderBySniff.php index 22dc66c..3923191 100644 --- a/Linchpin/Sniffs/Performance/SlowOrderBySniff.php +++ b/Linchpin/Sniffs/Performance/SlowOrderBySniff.php @@ -7,6 +7,7 @@ namespace Linchpin\Sniffs\Performance; +use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff; /** @@ -14,16 +15,14 @@ */ class SlowOrderBySniff extends AbstractArrayAssignmentRestrictionsSniff { - /** - * Current stack pointer. - * - * @var int - */ - protected $stackPtr; - /** * Groups of variables to restrict. * + * The message uses %2$s rather than %s on purpose. The parent emits with + * `array( $key, $value )` as the replacements, so %s resolves to the array key + * — always the literal "orderby" — and %2$s is the assigned value, which is the + * part worth naming. + * * @return array */ public function getGroups() @@ -31,7 +30,7 @@ public function getGroups() return [ 'slow_order' => [ 'type' => 'warning', - 'message' => 'Ordering query results by %s is not performant.', + 'message' => 'Ordering query results by %2$s is not performant.', 'keys' => [ 'orderby', ], @@ -40,22 +39,31 @@ public function getGroups() } /** - * Process a token. + * Callback to process each confirmed key, to check value. * - * Overrides the parent to store the stackPtr for later use. + * Returning true lets the parent emit the group message above. This sniff used + * to call `$this->addMessage()` here and return false to suppress the built-in + * message — the shape WPCS 2 offered on WordPressCS\WordPress\Sniff. WPCS 3 + * removed that method, so the call resolved to nothing on the class or its + * parent and the sniff fatalled the moment it found something to report: * - * @param int $stackPtr The position of the current token in the stack. - */ - public function process_token( $stackPtr ) - { - $this->stackPtr = $stackPtr; - parent::process_token($stackPtr); - unset($this->stackPtr); - } - - /** - * Callback to process each confirmed key, to check value. - * This must be extended to add the logic to check assignment value. + * Uncaught Error: Call to undefined method + * Linchpin\Sniffs\Performance\SlowOrderBySniff::addMessage() + * + * Because it only ever ran on a query that actually ordered by one of these + * values, it looked healthy on any codebase that never hit the pattern, and + * consuming projects worked around it by excluding the sniff outright. + * + * Deferring to the parent rather than re-adding a shim also drops the + * `$stackPtr` property and the `process_token()` override that existed only to + * carry a token pointer to the manual call. The parent already reports against + * the key's own token, which is the more accurate position. + * + * The quotes have to come off first. The parent builds the value from the raw + * tokens, so a literal arrives here as `'rand'` — quotes included — and the + * comparison against `rand` could never match. Combined with the fatal above, + * the sniff had two independent reasons never to work: it matched nothing, and + * anything it did match would have crashed. * * @param string $key Array index / key. * @param mixed $val Assigned value. @@ -66,20 +74,11 @@ public function process_token( $stackPtr ) */ public function callback( $key, $val, $line, $group ) { - switch ( $val ) { + switch ( TextStrings::stripQuotes( (string) $val ) ) { case 'rand': case 'meta_value': case 'meta_value_num': - $this->addMessage( - 'Ordering query results by %s is not performant.', - $this->stackPtr, - 'warning', - 'slow_order', - [ $val ] - ); - - // Skip built-in message. - return false; + return true; default: // No match. diff --git a/Linchpin/ruleset.xml b/Linchpin/ruleset.xml index 7e8e785..fc99e8d 100644 --- a/Linchpin/ruleset.xml +++ b/Linchpin/ruleset.xml @@ -14,8 +14,21 @@ - - + + node_modules/* vendor/*