Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions coder_sniffer/Drupal/Sniffs/Attributes/ValidHookNameSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php
/**
* \Drupal\Sniffs\Attributes\ValidHookNameSniff.
*
* @category PHP
* @package PHP_CodeSniffer
* @link http://pear.php.net/package/PHP_CodeSniffer
*/

namespace Drupal\Sniffs\Attributes;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;

/**
* Checks that Hook attribute argument name not starts with "hook_" prefix.
*
* @category PHP
* @package PHP_CodeSniffer
* @link http://pear.php.net/package/PHP_CodeSniffer
*/
class ValidHookNameSniff implements Sniff
{


/**
* Returns an array of tokens this test wants to listen for.
*
* @return array<int|string>
*/
public function register()
{
return [T_ATTRIBUTE];

}//end register()


/**
* Processes this test, when one of its tokens is encountered.
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The PHP_CodeSniffer file where the
* token was found.
* @param int $stackPtr The position in the PHP_CodeSniffer
* file's token stack where the token
* was found.
*
* @return void|int Optionally returns a stack pointer. The sniff will not be
* called again on the current file until the returned stack
* pointer is reached. Return $phpcsFile->numTokens + 1 to skip
* the rest of the file.
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$attributeName = $phpcsFile->findNext(T_STRING, ($stackPtr + 1));
if ($attributeName !== false
&& $tokens[$attributeName]['content'] === 'Hook'
) {
$hookName = $phpcsFile->findNext(T_CONSTANT_ENCAPSED_STRING, ($attributeName + 2));
if ($hookName !== false
) {
// Remove outer quotes.
$hookNameValue = trim($tokens[$hookName]['content'], '"\'');

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) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

// Remove "hook_" prefix.
$hookNameValueFixed = substr($hookNameValue, 5);
Copy link
Collaborator

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".

Copy link
Contributor Author

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)

// Return outer quotes.
$hookNameValueFixed = str_replace($hookNameValue, $hookNameValueFixed, $tokens[$hookName]['content']);
$phpcsFile->fixer->replaceToken($hookName, $hookNameValueFixed);
}
}
}
}//end if

}//end process()


}//end class
110 changes: 110 additions & 0 deletions tests/Drupal/Attributes/ValidHookNameUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<?php

/**
* @file
* Contains Test.
*/

/**
* Valid hook.
*/
#[Hook('valid')]
function module_valid() {

}

/**
* Single quotes.
*/
#[Hook('hook_info')]
function module_info() {

}

/**
* Double quotes.
*/
#[Hook("hook_node_load")]
function module_node_load() {

}


/**
* Attribute named argument.
*/
#[Hook(hook: 'hook_node_delete')]
function module_node_delete() {

}

/**
* Attribute named arguments.
*/
#[Hook(hook: 'hook_node_alter', module: 'custom_module')]
function module_node_alter() {

}

/**
* "hook" is a part of hook name.
*/
#[Hook('hook_piratehook_view')]
function module_piratehook_view() {

}

/**
* Implements hook_hookpirate_view().
*
* "hook" is a part of hook name.
*/
#[Hook('hook_hookpirate_view')]
function module_hookpirate_view() {

}

/**
* Valid hook.
*/
#[Hook('valid', 'validMethod', 'module')]
class ValidHooks {

/**
*
*/
public function validMethod() {

}

}

/**
*
*/
#[Hook('hook_user_cancel', 'userCancel', 'custom')]
class Hooks {

/**
*
*/
public function userCancel() {

}

}

/**
* Named arguments, double quotes.
*/
#[Hook(hook: "hook_user_login", method: "userLogin", module: "views")]
class MyHooks {

/**
*
*/
public function userLogin() {

}

}
109 changes: 109 additions & 0 deletions tests/Drupal/Attributes/ValidHookNameUnitTest.inc.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
<?php

/**
* @file
* Contains Test.
*/

/**
* Valid hook.
*/
#[Hook('valid')]
function module_valid() {

}

/**
* Single quotes.
*/
#[Hook('info')]
function module_info() {

}

/**
* Double quotes.
*/
#[Hook("node_load")]
function module_node_load() {

}

/**
* Attribute named argument.
*/
#[Hook(hook: 'node_delete')]
function module_node_delete() {

}

/**
* Attribute named arguments.
*/
#[Hook(hook: 'node_alter', module: 'custom_module')]
function module_node_alter() {

}

/**
* "hook" is a part of hook name.
*/
#[Hook('piratehook_view')]
function module_piratehook_view() {

}

/**
* Implements hook_hookpirate_view().
*
* "hook" is a part of hook name.
*/
#[Hook('hookpirate_view')]
function module_hookpirate_view() {

}

/**
* Valid hook.
*/
#[Hook('valid', 'validMethod', 'module')]
class ValidHooks {

/**
*
*/
public function validMethod() {

}

}

/**
*
*/
#[Hook('user_cancel', 'userCancel', 'custom')]
class Hooks {

/**
*
*/
public function userCancel() {

}

}

/**
* Named arguments, double quotes.
*/
#[Hook(hook: "user_login", method: "userLogin", module: "views")]
class MyHooks {

/**
*
*/
public function userLogin() {

}

}
54 changes: 54 additions & 0 deletions tests/Drupal/Attributes/ValidHookNameUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

namespace Drupal\Test\Attributes;

use Drupal\Test\CoderSniffUnitTest;

class ValidHookNameUnitTest extends CoderSniffUnitTest
{


/**
* Returns the lines where errors should occur.
*
* The key of the array should represent the line number and the value
* should represent the number of errors that should occur on that line.
*
* @param string $testFile The name of the file being tested.
*
* @return array<int, int>
*/
protected function getErrorList(string $testFile): array
{
return [];

}//end getErrorList()


/**
* Returns the lines where warnings should occur.
*
* The key of the array should represent the line number and the value
* should represent the number of warnings that should occur on that line.
*
* @param string $testFile The name of the file being tested.
*
* @return array<int, int>
*/
protected function getWarningList(string $testFile): array
{
return [
19 => 1,
27 => 1,
36 => 1,
44 => 1,
52 => 1,
62 => 1,
85 => 1,
100 => 1,
];

}//end getWarningList()


}//end class