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

ValidHookName: verify correct handling of single vs double quotes names #242

Closed
jrfnl opened this issue May 26, 2021 · 1 comment · Fixed by #243
Closed

ValidHookName: verify correct handling of single vs double quotes names #242

jrfnl opened this issue May 26, 2021 · 1 comment · Fixed by #243

Comments

@jrfnl
Copy link
Collaborator

jrfnl commented May 26, 2021

Inspired by a question in Slack from @Djennez :

The NamingConventions/ValidHookName sniff needs a quick review for correct handling of double backslashes.

When single quoted strings are used, the "namespace-like" prefix for hooks should be fine.
However, when double quoted strings are used, the namespace separator(s) in the hook name prefix need to be escaped as they could otherwise result in a PHP escape character in the hook name.

// Correct (single quoted).
apply_filter( 'Namespace\Like\Prefix\hookname', $var);

// Incorrect (double quoted - the `\` needs to be escaped).
apply_filter( "Namespace\Like\Prefix\hookname", $var);

// Correct (double quoted).
apply_filter( "Namespace\\Like\\Prefix\\hookname", $var);

The sniff should:

  1. Not throw false positives when the prefix is correct, but uses escaped backslashes in a double quoted string.
  2. Possibly throw an error when a double quoted string is used for the hook name, but the backslashes aren't escaped.

Implementation note:

  • This requires that the actual surrounding quotes for the text string is checked as PHPCS will tokenize double quotes strings without interpolation as single quoted strings. (T_CONSTANT_ENCAPSED_STRING versus T_DOUBLE_QUOTED_STRING).
@jrfnl
Copy link
Collaborator Author

jrfnl commented May 27, 2021

@Djennez I've created PR #243 to address this. Would you feel comfortable doing a review of the PR ?

@jrfnl jrfnl added this to the 2.x Next Release milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant