infinum / eightshift-coding-standards

Eightshift coding standards for WordPress
https://eightshift.com
MIT License
16 stars 2 forks source link

[BUG] - Add additional test for the components escape sniff #59

Open dingo-d opened 8 months ago

dingo-d commented 8 months ago

I just ran into some weird issues when doing some maintenance on the eightshift-forms (wanted to update CS, and add some exclusions in the phpcs.xml.dist file, so that the plugin doesn't have to have // phpcs:ignore comments in the files), when I noticed that this code was reporting an error on the Components::checkAttr method when it shouldn't:

echo Components::renderPartial('component', $manifest['componentName'], 'multistep', [  // phpcs:ignore Eightshift.Security.ComponentsEscape.OutputNotEscaped
    'steps' => $progressBarSteps,
    'componentClass' => $componentClass,
    'jsClass' => UtilsHelper::getStateSelector('stepProgressBar'),
    'hideLabels' => Components::checkAttr('progressBarHideLabels', $attributes, $manifest),
]);

Added rule was:

<rule ref="Eightshift.Security.ComponentsEscape">
    <properties>
        <property name="overriddenClass" value="EightshiftFormsVendor\\EightshiftLibs\\Helpers\\Components"/>
        <property name="allowedMethods" type="array">
            <element key="render" value="render"/>
            <element key="outputCssVariables" value="outputCssVariables"/>
            <element key="checkAttr" value="checkAttr"/>
        </property>
    </properties>
</rule>

so the checkAttr shouldn't have been flagged. My guess is since the renderPartial wasn't on the list, but instead was manually ignored, the next call to the Components::checkAttr method was triggering the sniff (even though it shouldn't because it's in the allowed list).

I'm not 100% sure if this is intended behaviour of PHPCS or not, will have to check upstream to be sure.