WordPress-Coding-Standards
WordPress-Coding-Standards copied to clipboard
Hotfix - Lowercase custom sniff properties
This PR will address the issue of handling mixed case function/method/class names when those are passed as custom-allowed properties in certain sniffs.
For instance, if we pass a custom sanitization function as a property my_custom_sanitization
then it doesn't matter if we are calling My_custom_Sanitization
or MY_CUSTOM_SANITIZATION
(or any combination of cases for that matter), PHP is case insensitive in that regard and the same function will be called.
So the sniff shouldn't trigger on those variations (in some cases it will do that, as shown in PR #2370 which holds the fix for the custom escaping functions).
I've added tests to ensure that these changes will be caught.
I'm not 100% sure if I've covered all the cases, so any help in pointing out if I've missed something helps.
I've just now realized that this helper is kinda doing too much on its own than its name suggests.
The helper says it will merge the custom array, but can also flip the base array in the case the custom array is empty and the $flip
parameter is true
(which is the default). And also do the same for the base array.
Would it make sense to rename it to something else? manipulate_arrays
is a bit too generic and doesn't really describe what this helper does. Maybe merge_and_prepare_arrays
, flip_and_merge_arrays
? Something like that? The second one fits a bit more since the $flip
parameter is always true.
EDIT:
Also, I think we can optimize it a bit by checking if both $base
and $custom
are empty arrays and just return an empty array, no?
I've just now realized that this helper is kinda doing too much on its own than its name suggests.
The helper says it will merge the custom array, but can also flip the base array in the case the custom array is empty and the
$flip
parameter istrue
(which is the default). And also do the same for the base array.Would it make sense to rename it to something else?
manipulate_arrays
is a bit too generic and doesn't really describe what this helper does. Maybemerge_and_prepare_arrays
,flip_and_merge_arrays
? Something like that? The second one fits a bit more since the$flip
parameter is always true.
Hard "no" from me on this. The function merges two arrays, whether flipped or not etc, those are just implementation details for the merge, so functionality-wise, the name is perfectly fine.
The fact that the Helper does not have a BC-promise, still doesn't mean that BC should be broken without good reason and there is no poignant reason to change the method name.
@dingo-d Is this PR ready for its final review in the mean time ?
Maybe dismiss "requested changes" once they have been addressed and ask for a new review via the interface when you think it is ready ? It's now not always clear whether or not it's time to review again.
Ready for the final review @jrfnl 🙂