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

Review hook setup in Whip_Hosts.php #67

Open
jrfnl opened this issue May 28, 2018 · 1 comment
Open

Review hook setup in Whip_Hosts.php #67

jrfnl opened this issue May 28, 2018 · 1 comment

Comments

@jrfnl
Copy link
Contributor

jrfnl commented May 28, 2018

In the Whip_Host.php file, there are a couple of calls to apply_filters() with variable filter names.

I'm not familiar enough with the code to properly review these, but I do believe this setup should be reviewed for the following reasons:

  • Variable filter names with unpredictable values make it hard to hook into these.
  • All hook names should be prefixed to prevent conflicts with other plugins/themes. This is not safeguarded in the current setup.

Relevant warnings thrown by PHPCS when run without exclusions:

FILE: src\Whip_Host.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------
  47 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "strtolower( self::HOST_NAME_KEY )".
  77 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "strtolower( $messageKey )".
 102 | WARNING | Hook names invoked by a theme/plugin should start with the theme/plugin
     |         | prefix. Found: "self::HOSTING_PAGE_FILTER_KEY".
------------------------------------------------------------------------------------------
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 21, 2018

When this issue is solved, the exception which is currently made for this will need to be removed from the .phpcs.xml.dist file.

whip/.phpcs.xml.dist

Lines 101 to 105 in 0b22d36

<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound">
<!-- These hook setups should be reviewed.
Ticket: https://github.com/Yoast/whip/issues/67 -->
<exclude-pattern>/src/Whip_Host\.php$</exclude-pattern>
</rule>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant