whip icon indicating copy to clipboard operation
whip copied to clipboard

Review hook setup in `Whip_Hosts.php`

Open jrfnl opened this issue 6 years ago • 1 comments

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 avatar May 28 '18 14:05 jrfnl

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

https://github.com/Yoast/whip/blob/0b22d36087ab1afe459e8d8cc18043c9ff7b47e9/.phpcs.xml.dist#L101-L105

jrfnl avatar Sep 21 '18 23:09 jrfnl