Skip to content

Conversation

rodrigoprimo
Copy link
Collaborator

@rodrigoprimo rodrigoprimo commented Oct 15, 2025

Description

This PR fixes how the sniff handles non-lowercased null when passed as the value of the $ver parameter. Now the sniff consistently handles null regardless of the case and always returns a warning. Before, it would return a warning only for lower-cased null and an error for all other variations of null.

It also fixes how the sniff handles fully qualified \false and \null. Before this PR, passing \false or \null as the $ver parameter would result in a false negative. Now it correctly results in a warning for \null and an error for \false. For this fix, it was necessary to raise the minimum supported PHPCS version to 3.13.4 to benefit from a related change in how PHPCS tokenizes \false and \null.

Besides that, the PR includes a separate commit that fixes some mistakes in the code comments of this sniff.

Suggested changelog entry

Changed:

  • The minimum required PHP_CodeSniffer version to 3.13.4 (was 3.13.0).

Fixed:

  • WordPress.WP.EnqueuedResourceParameters: correctly handles non-lowercase null when passed as the $ver parameter value.
  • WordPress.WP.EnqueuedResourceParameters: now correctly handles fully qualified \null and \false values in the $ver parameter.

This commit fixes how the sniff handles non-lowercased `null` when passed as the value of the `$ver` parameter. Now the sniff consistently handles `null` regardless of the case and always returns a warning. Before, it would return a warning only for lower-cased `null` and an error for all other variations of `null`.
@jrfnl
Copy link
Member

jrfnl commented Oct 17, 2025

Fully qualified \null will be addressed in a subsequent PR together with fully qualified \false.

Why ? This feels to me like you are doubling the work for reviewers. Unless the changes are huge (which I don't expect them to be), it could/should just be a separate commit in this PR.

@rodrigoprimo rodrigoprimo changed the title WP/EnqueuedResourceParameters: fix handling of non-lowercased null WP/EnqueuedResourceParameters: fix handling of non-lowercased null and fully qualified \null and \false Oct 20, 2025
@rodrigoprimo
Copy link
Collaborator Author

rodrigoprimo commented Oct 20, 2025

Why ? This feels to me like you are doubling the work for reviewers. Unless the changes are huge (which I don't expect them to be), it could/should just be a separate commit in this PR.

I opted to separate precisely because I thought it would make the work for reviewers easier. The changes are indeed not huge and are related, but slightly different. I thought they were different enough to justify a separate PR.

Since you prefer to review them together, I added another commit to this PR with the fix for handling fully qualified \null and \false and edited the PR title and description. Adding this commit here uncovered an issue that I had missed.

My fix for handling fully qualified \null and \false only works on PHPCS >= 3.13.3, as this version changed how they are tokenized (PHPCSStandards/PHP_CodeSniffer#1206), and WPCS still supports PHPCS >= 3.13.0. I don't think it is worth changing the code here to also support how PHPCS tokenizes fully qualified \null and \false before 3.13.3. Is bumping the minimum required PHPCS version to 3.13.3 an option? If it is not an option, I'm inclined to think that it is better to leave this change to after PHPCS 3.x support is dropped.

@jrfnl
Copy link
Member

jrfnl commented Oct 20, 2025

Is bumping the minimum required PHPCS version to 3.13.3 an option?

Totally and this change/fix is a good reason to justify the dependency bump.

@rodrigoprimo rodrigoprimo force-pushed the enqueued-resource-parameters-case-insensitive-null branch from b43f8b9 to a3cff7f Compare October 20, 2025 16:43
This is necessary to benefit from the new way to tokenize fully qualified `\false` and `\null` introduced in 3.13.3 (PHPCSStandards/PHP_CodeSniffer 1206). It will simplify a fix for how `WordPress.WP.EnqueuedResourceParameters` handles fully qualified `\false` and `\null` (see 2630).

Bumping the version to 3.13.4 instead of 3.13.3 as 3.13.4 contains a fix to a bug that affected 3.13.3 that prevents WPCS sniffs tests from executing (PHPCSStandards/PHP_CodeSniffer 1213).
…null` correctly

Before this change passing `\false` or `\null` as the `$ver` parameter would result in a false negative.
@rodrigoprimo rodrigoprimo force-pushed the enqueued-resource-parameters-case-insensitive-null branch from a3cff7f to 0b1af1e Compare October 20, 2025 16:50
@rodrigoprimo
Copy link
Collaborator Author

Ok, I added a new commit to this PR bumping the minimum required PHPCS version to 3.13.4. Let me know if you prefer that this change is introduced in a separate PR and I will do that.

We need to bump to 3.13.4 instead of 3.13.3 due to the bug introduced in 3.13.3 and fixed in 3.13.4 that prevents the execution of sniff tests in external standards.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants