Sniff: move "missing unslashing" handling to callback function #2347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As things were, the "missing unslashing" error was being thrown from the `Sniff::is_sanitized()` method, based on whether the `$require_unslash` parameter was set to `true` or `false`. In my opinion, it should not be the responsibility of utility functions to throw errors/warning, but always of the originating sniff. This commit replaces the boolean `$require_unslash` parameter for the `Sniff::is_sanitized()` method to a nullable callable `$unslash_callback` parameter. Instead of letting the `Sniff::is_sanitized()` method throw the error/warning, the callback will now be called and as the originating sniff passes the callback, the originating sniff can now handle the throwing of the error/warning. To remove any presumptions of the callback being passed, being in the same context as the utility method/originating sniff and having access to the `$phpcsFile` via a property, the callback will receive both the `$phpcsFile` as well as the `$stackPtr` as parameters. The "missing unslash" check can still be skipped by not passing the parameter or by passing anything non-callable. This maintains the original behaviour for the method for the default value/`false` case. The `Sniff::add_unslash_error()` method can with this change now be moved to the `ValidatedSanitizedInput` sniff and that sniff will now have full control over the error message and error code. Note: the callback is called using `call_user_func()` instead of `$callable` to allow some tolerance for the _partially supported callables_, which were deprecated in PHP 8.2. While WPCS itself does not use these, the `ValidatedSanitizedInputSniff` is one of the exceptions which has not been made `final`, so an extending class _could_ use one of the _partially supported callables_ and the `Sniff::is_sanitized()` method should not be the reason that doesn't work.
dingo-d
approved these changes
Aug 17, 2023
|
Looks like we may need to tweak the codecov (patch) config a little more. As project is ➕ , I don't think it should be a blocker for now though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As things were, the "missing unslashing" error was being thrown from the
Sniff::is_sanitized()method, based on whether the$require_unslashparameter was set totrueorfalse.In my opinion, it should not be the responsibility of utility functions to throw errors/warning, but always of the originating sniff.
This commit replaces the boolean
$require_unslashparameter for theSniff::is_sanitized()method to a nullable callable$unslash_callbackparameter.Instead of letting the
Sniff::is_sanitized()method throw the error/warning, the callback will now be called and as the originating sniff passes the callback, the originating sniff can now handle the throwing of the error/warning.To remove any presumptions of the callback being passed, being in the same context as the utility method/originating sniff and having access to the
$phpcsFilevia a property, the callback will receive both the$phpcsFileas well as the$stackPtras parameters.The "missing unslash" check can still be skipped by not passing the parameter or by passing anything non-callable. This maintains the original behaviour for the method for the default value/
falsecase.The
Sniff::add_unslash_error()method can with this change now be moved to theValidatedSanitizedInputsniff and that sniff will now have full control over the error message and error code.Note: the callback is called using
call_user_func()instead of$callableto allow some tolerance for the partially supported callables, which were deprecated in PHP 8.2. While WPCS itself does not use these, theValidatedSanitizedInputSniffis one of the exceptions which has not been madefinal, so an extending class could use one of the partially supported callables and theSniff::is_sanitized()method should not be the reason that doesn't work.