-
Notifications
You must be signed in to change notification settings - Fork 53
feat(ValidHookName): Add sniff for hook name in PHP attributes #260
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RuZniki nice to hear from you :)
Left some comments, please also check the test fails.
coder_sniffer/Drupal/Sniffs/NamingConventions/ValidAttributeHookNameSniff.php
Outdated
Show resolved
Hide resolved
{ | ||
$tokens = $phpcsFile->getTokens(); | ||
|
||
if ($tokens[$stackPtr + 1]['type'] === 'T_STRING' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all those conditions we should check first with isset() if the next token is there. This will help prevent failures in live coding when the code is just being written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swtiched to ->findNext
, do you think this is enough?
coder_sniffer/Drupal/Sniffs/NamingConventions/ValidAttributeHookNameSniff.php
Outdated
Show resolved
Hide resolved
coder_sniffer/Drupal/Sniffs/NamingConventions/ValidAttributeHookNameSniff.php
Outdated
Show resolved
Hide resolved
ff01836
to
0f46b15
Compare
@klausi I am also happy to get feedback from you, thanks a lot for your review. I moved sniff to #[Hook(method: "userLogin", hook: "hook_user_login")] I believe this should not be a part of this sniff. I add special handling for hook_hook_info(). But still not sure about this case. PR is ready for review. |
I removed special handling for "hook_hook_info()". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
Looks good to me, just 2 minor things inline.
|
||
if (strpos($hookNameValue, 'hook_') === 0) { | ||
$fix = $phpcsFile->addFixableWarning("Hook name should not start with 'hook_'. Hook name used: $hookNameValue", $hookName, 'HookPrefix'); | ||
if ($fix === true && strlen($hookNameValue) > 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the strlen() condition here can be removed, this was already checked before.
We should not add additional conditions with $fix
, it would be very confusing if the fixer does not always work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to avoid case when hook is not finished, and it's just "hook_".
Now I get your point, I will move second clause to above statement. Thanks.
$fix = $phpcsFile->addFixableWarning("Hook name should not start with 'hook_'. Hook name used: $hookNameValue", $hookName, 'HookPrefix'); | ||
if ($fix === true && strlen($hookNameValue) > 5) { | ||
// Remove "hook_" prefix. | ||
$hookNameValueFixed = substr($hookNameValue, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use this in the error message above like "The hook name should not start with 'hook_', expected '%s' but found '%s".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I update error message.
Result example
The hook name should not start with 'hook_', expected 'node_alter' but found 'hook_node_alter' (Drupal.Attributes.ValidHookName.HookPrefix)
5e27267
to
826142d
Compare
Thanks, Drupal core regression tests are failing, but looks unrelated to this issue. Will check in a follow-up! |
Issue: https://www.drupal.org/project/coder/issues/3499291