WP/EnqueuedResourceParameters: fix handling of non-lowercased null and fully qualified \null and \false

[rodrigoprimo]

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.

[jrfnl]

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]

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]

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]

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.

[jrfnl]

Just got some nitpicky remarks. Other than that, all good.

[rodrigoprimo]

Thanks for your review, @jrfnl! As we discussed during our call yesterday, I implemented the changes that you suggested directly into the original commits and force pushed. I left a follow up comment in one of the remarks above regarding including the name of the parameters in the sniff class docblock.