WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

Hotfix - Lowercase custom sniff properties

Open dingo-d opened this issue 1 year ago • 4 comments

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.

dingo-d avatar Sep 17 '23 08:09 dingo-d

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?

dingo-d avatar Sep 19 '23 06:09 dingo-d

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.

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.

jrfnl avatar Sep 19 '23 08:09 jrfnl

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

jrfnl avatar Sep 23 '23 00:09 jrfnl

Ready for the final review @jrfnl 🙂

dingo-d avatar Sep 23 '23 07:09 dingo-d