Skip to content

fix(NO-TASK): Make SlowOrderBy work and open the PHP target - #45

Open
aaronware wants to merge 1 commit into
mainfrom
fix/slow-orderby-sniff
Open

fix(NO-TASK): Make SlowOrderBy work and open the PHP target#45
aaronware wants to merge 1 commit into
mainfrom
fix/slow-orderby-sniff

Conversation

@aaronware

Copy link
Copy Markdown
Contributor

Linchpin.Performance.SlowOrderBy has never worked

Two independent reasons, either of which alone would have been enough.

1. It fatalled. callback() 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:

Uncaught Error: Call to undefined method
Linchpin\Sniffs\Performance\SlowOrderBySniff::addMessage()

Because it only ran on a query that actually ordered by one of the slow values, it looked healthy on any codebase that never hit the pattern. SlowMetaQuerySniff hit the same wall and grew a MessageHelper shim; this one never did, so consuming projects excluded it. linchpin.com carries exactly that exclusion, referencing LINCHPIN-5245.

2. It could never have matched. AbstractArrayAssignmentRestrictionsSniff builds the value from raw tokens, so a literal arrives as 'rand' — quotes included — and the switch compared against rand. Stripping the quotes is what makes it fire at all.

The fix

Rather than re-adding a shim, callback() returns true and lets the parent emit. That's how the abstract class 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, a more accurate position.

The group message becomes %2$s: the parent passes array( $key, $value ) as replacements, so %s resolves to the literal "orderby" rather than the value worth naming.

Verified against a fixture:

 8 | WARNING | Ordering query results by 'rand' is not performant.
 9 | WARNING | Ordering query results by 'meta_value_num' is not performant.
10 | WARNING | Ordering query results by 'meta_value' is not performant.

…and 'date' correctly ignored.

testVersion becomes open-ended

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. A closed upper bound goes stale silently and needs a release here before any consumer can be checked correctly. 8.2- means 8.2 and above; a project that wants to pin one version still can.

Please note

composer test is already red on main, before this change — the fixture tests fail to load the ruleset with Class "WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff" not found. That's a harness autoloading problem, unrelated to this and deliberately untouched. Worth a separate look, since it means the sniff tests aren't currently protecting anything.

Downstream

Once released, linchpin.com can drop both its Linchpin.Performance.SlowMetaQuery / SlowOrderBy exclusions and its local testVersion override.

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.
@aaronware

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant