search

WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.57k stars 488 forks source link

WordPress.Security.EscapeOutput: Wiki misses hint about lower case entry for customEscapingFunctions #2508

Open datengraben opened 2 weeks ago

datengraben commented 2 weeks ago

phpcs version: 3.10.3 wp-cs version: 3.1.0

If I want to add a custom escaping function (for example myCustomEscapingFunction) in my phpcs.xml config via array parameter customEscapingFunctions of the WordPress.Security.EscapeOutput rule, I need to add it lower case, because the implementation forces the compare operation to use lowercase names.

See implementation:

https://github.com/WordPress/WordPress-Coding-Standards/blob/ffec7bfced474ed83c4e1b66225d37f84fdb65a3/WordPress/Helpers/EscapingFunctionsTrait.php#L254

So the problem is, if I provide a function name in an case sensitive way, at the moment of parsing the config value and instatiating the object, it does not get transformed into lowercase string. So the (above) line in the code of EscapingFunctionsTrait compares it the later to it's lowercase'd counterpart and thus will not match and my custom escaping function does not get recognized as configured.

Of course it gets recognized, if I use already the lower case variant in the phpcs.xml config.

Config Example:

    <rule ref="WordPress.Security.EscapeOutput">
        <properties>
            <property name="customEscapingFunctions" type="array">
                <element value="myCustomEscapingFunction"/><!-- won't work -->
                <element vlaue="mycustomescapingfunction" /> <!-- works -->
            </property>
        </properties>
    </rule>

I'm unsure if you can call it a bug. But I would suggest that the Customizable sniff wiki page in the section of custom escape output functions misses a hint about this behaviour. I couldn't get my head around it and went debugging, instead of remembering that php is case insensitive. So maybe the wiki can include a passage about this, at least as long as https://github.com/WordPress/WordPress-Coding-Standards/pull/2391 is not merged.

Anyway, thank you for your work!

GaryJones commented 1 week ago

Seems like something we should handle, rather than the developer discovering it should be lowercase.

dingo-d commented 1 week ago

@datengraben did you test out this patch: https://github.com/WordPress/WordPress-Coding-Standards/pull/2370 and see if that would fix your issue?

datengraben commented 1 week ago

@dingo-d no I did not test your patch, but I fixed it locally by myself, using pretty much the same approach as you have in your patch.