Coding standards fixes #58
Conversation
|
Thanks for taking on this work! I would like to ask you to pull this apart into two pull requests, one for coding standards changes and one for filter name changes. This will mean that some additional changes will be needed to disable CS sniffs in comments if they apply to the older filter syntax, but this is a lot to review in one go and the filter name change is much more impactful and will be surprising for existing users of the plugin; we’ll be able to move much faster on the individual pieces if they’re split out. Thanks! |
|
Thanks for the feedback @kadamwhite . I have broken up this PR into 2. There is #59 for the filter names. I know it is a a lot of code, but you can see the linters passing in travis now, meaning at least the code is valid. |
There was a problem hiding this comment.
I've reviewed and don't see any major issues with the changes, they all make sense from an escaping or code style consistency perspective.
As the changes here are adjusting the project to follow WordPress core coding standards, not VIP Go's specifically, I'm going to approve and merge. Thanks, @spacedmonkey
However, to clarify: I am opposed to forcing this project to follow VIP Go coding standards; it is a core project and should follow the unmodified Core WordPress Core coding standards. I have never had an issue getting VIP to accept other plugins using WP Core standards, so I would expect that would be sufficient.
| } | ||
|
|
||
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
It horrifies me that this satisfies a sniff 😬
| <?php endif ?> | ||
| <?php | ||
| } | ||
|
|
||
| /** | ||
| * Render a single row. | ||
| * Render a single row. |
There was a problem hiding this comment.
How is this correct? This is not aligned with anything else in the comment.
| <p><?php echo $name ?></p> | ||
| <p><?php echo implode( ' | ', $details ) ?></p> | ||
| <p><?php echo $name; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?></p> | ||
| <p><?php echo implode( ' | ', $details ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?></p> |
There was a problem hiding this comment.
Is there a reason we don't just add esc_html_e here?
As I wish to use this project in a VIP GO site, I have taken it apon myself to fix the coding standards and security issues with this plugin.
I run this against VIP GO 2.0.0 and Core 2.0.0 standards. Many of the fixes are automated, but other I did by hand.
I also improved some of the comments (mainly the commons defining in the plugin).
One other thing, many of the filters and actions were not named correctly. Instead of removing them, I have added new filters and deprecated the old ones.