Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for WPCS 1.0 release #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update for WPCS 1.0 release #8

wants to merge 4 commits into from

Conversation

JPry
Copy link

@JPry JPry commented Aug 1, 2018

This updates our ruleset to take into account the recent release of WPCS 1.0. With this update, I've tried to keep our existing rules in place by going through what we had to start with, and what is now found in the ruleset.xml file.

One major difference is that the extended rules related to working with WC have been moved to a separate ruleset that builds on the main ruleset. Plugins such as Subscriptions should be able to use this, and we can use the main ruleset for our (very few) plugins that don't require WC.

I've added a composer.json file here so that we can take advantage of the phpcodesniffer-standard project type, which facilitates auto-installation of rulesets when the dealerdirect/phpcodesniffer-composer-installer package is used.

Closes #5.
Closes #6.

Also, in case anyone is interested, here's the link to the documentation on how WPCS recommends customizing the rules: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties.

Copy link
Contributor

@menakas menakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JPry Thanks for the review request. It was a learning experience.

Within limits of my understanding, I have tried to review this.

<ruleset name="Prospress-WC">
<description>Prospress WooCommerce Coding Standards</description>

<!-- Include the main Prospress stnadard -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆎 stnadard -> standard.

<exclude-pattern>*/templates/emails/plain/*</exclude-pattern>
<properties>
<!-- e.g. body_class, the_content, the_excerpt -->
<property name="customAutoEscapedFunctions" type="array" value="woocommerce_wp_select,wcs_help_tip"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 This release note says that the above form of specifying array values has been deprecated and element tags should be used. Not sure if it is only in the case of key-value pairs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding that, I didn't know that was the case 🙂

<!-- e.g. body_class, the_content, the_excerpt -->
<property name="customAutoEscapedFunctions" type="array" value="woocommerce_wp_select,wcs_help_tip"/>
<!-- e.g. esc_attr, esc_html, esc_url-->
<property name="customEscapingFunctions" type="array" value="wcs_json_encode,htmlspecialchars,wp_kses_allow_underscores"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Setting array values using element tag - same as above

<rule ref="PEAR.Functions.FunctionCallSignature.CloseBracketLine">
<severity>0</severity>
</rule>
<!-- Selectively import other WordPress rules -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Any particular reason why Generic.Files.LowercasedFilename is not imported?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of our libraries (mainly Action Scheduler) use lowercase file names. Also, in my opinion, that's not one that we necessarily need to follow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're also asking why I didn't keep it when it used to be part of the ruleset? The reason for that is that it appeared to be superseded by the WordPress.Files.FileName rule.

<property name="customEscapingFunctions" type="array" value="0=>wcs_json_encode,1=>htmlspecialchars,2=>wp_kses_allow_underscores"/>
<!-- e.g. _deprecated_argument, printf, _e-->
<property name="customPrintingFunctions" type="array" value=""/>
<property name="strict_class_file_names" value="false"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ As far as I have observed in WCS code, I see class file names starting with "class-". Any reason why this has not been enforced strictly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of our libraries use the class- prefix in file naming.

@JPry
Copy link
Author

JPry commented Aug 7, 2018

Thanks for the review @menakas! I've made some adjustments based on your feedback.

@jconroy jconroy removed their request for review July 2, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants