Hotfix - Lowercase custom sniff properties

[dingo-d]

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.

[jrfnl]

@dingo-d Thanks for setting up.

Note: The tests aren't actually testing the issue. This needs fixing.

[jrfnl]

Reviewed again. Left feedback.

Also looks like the tests are better than before, but still not completely targeted to this patch.
Basically, the phpcs:set directives should only contain mixed/uppercase functions to test this patch.

[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?

[jrfnl]

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]

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

[dingo-d]

Ready for the final review @jrfnl 🙂