2.x and seems to break hook implementations.

[b-sharpe]

When using 2.x with Drupal, we get errors on all hook implementations, even for Drupal Core:

For example.

   27 | ERROR   | [ ] Function claro_theme_suggestions_form_element_alter() does not have parameter type hint nor @param annotation for its parameter $suggestions.
   27 | ERROR   | [ ] Function claro_theme_suggestions_form_element_alter() does not have parameter type hint nor @param annotation for its parameter $variables.

I'm not sure if these were just being skipped before, but it would seem this will cause a fair amount of pain for themes/modules. The change was introduced in #58

[danepowell]

What standard are you using? I don't recall messing with the Drupal standards at all in 2.x. But now that I think about it, they might inherit from AcquiaPHP...

Is there a reason you can't or shouldn't type hint the functions as the error suggests? It might be a lot of work but that's why it's in a new major release.

[b-sharpe]

We're using BLT, so this is coming from blt validate, but essentially is just the Drupal standard. There's nothing preventing us from fixing it, I'm just worried this is something I should change, as the standard for hooks has always been:

/**
 * Implements hook_preprocess_HOOK().
 */

We've just locked to V1.1 for now, my main concern was that we had many projects just automatically upgrade to 2.x here and immediately have thousands of lines to fix.

My guess is this is actually an issue with the acquia/blt-phpcs package and it's requirement of "acquia/coding-standards": "*",

If you believe there's no issue here and we should fix all hook implementations, I can create an issue on that project suggesting 1.x unless they release a new major version.

[danepowell]

I'll be honest, I haven't implemented a Drupal hook in some time so I'm not sure what best practice is. But as a PHP generalist, I would strongly advocate for type hints, hence why we enforce them in coding standards 2.x.

If this conflicts with a documented Drupal standard I'm happy to walk it back, otherwise I'd recommended following the stronger standards.

This will require updating any hooks you've implemented, though it shouldn't be hard to add a few type hints. It shouldn't affect Drupal core since you shouldn't be using Acquia standards to scan core/contributed files in the first place.

[nmillin]

I came across this issue after finding lots of errors after updating my project. I did the same thing as b-sharpe and set acquia/coding-standards-php to 1.1. No more errors (for now at least).

Re: Drupal Standard I did some looking to see what the standard is and found:

1. https://www.drupal.org/docs/develop/standards/php/php-coding-standards#indenting which isn't much, but only has the Implements hook_help() in the comment. No @param for the $route_name is included.
2. the coder module (version 8.3.18) has a bit more information. The module throws a warning if @param or @return is used in a hook implementation. So if we follow acquia/coding-standards-php, the coder module will not be happy with us.

@danepowell thoughts?

[danepowell]

Can you type hint without using the param or return annotations? That should satisfy coder and the Acquia standards.

The intent with the standards change in 2.0 is to enforce type hinting. That the rule apparently allows either type hints or annotations is unfortunate; I actually prefer not to annotate params except for particularly complex methods.

[nmillin]

@danepowell sure, but that isn't what I've seen on Drupal.org. Trying to match AcquiaPHP would be a lot of work when the Drupal documentation (drupal.org) isn't following the AcquiaPHP standards. Ex: hook_form_alter() - https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21form.api.php/function/hook_form_alter/10 $form and $form_id don't have type hints. The CKeditor5 module implements this hook - https://api.drupal.org/api/drupal/core%21modules%21ckeditor5%21ckeditor5.module/10 - but provides the type hint of array instead of mixed for $form. Function ckeditor5_form_filter_format_form_alter() does not have @param annotation for its traversable parameter $form. $form_id still no type hint. Sigh...

It looks like this change in the 2.x version was from an update to the AcquiaPHP/ruleset.xml. Since all of the Rulesets contain AcquiaPHP, which isn't Drupal focused, it seems like this project isn't for me anymore.

[mglaman]

We're hitting this.

We're adding the following to our @file doc:

* @phpcs:disable Drupal.Commenting.HookComment.HookParamDoc

I wish defining array shapes/schema was delegated to PHPStan for actual static analysis and @phpstan-param or @pslam-param workarounds.

[fiasco]

I hit this also. Anyone who uses this project and implements a module hook in a custom module will hit this. I propose we turn off the SlevomatCodingStandard type hints for .module files as this is the only place hooks reside. This is what I'm running in my project's local php.xml.dist to get around this issue.

<ruleset>
    <!-- ... other config -->
    <rule ref="AcquiaDrupalStrict"/>

    <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingTraversableTypeHintSpecification">
        <exclude-pattern>docroot/modules/custom/*/*.module</exclude-pattern>
    </rule>
    <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingAnyTypeHint">
        <exclude-pattern>docroot/modules/custom/*/*.module</exclude-pattern>
    </rule>
    <rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingAnyTypeHint">
        <exclude-pattern>docroot/modules/custom/*/*.module</exclude-pattern>
    </rule>
    <rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingTraversableTypeHintSpecification">
        <exclude-pattern>docroot/modules/custom/*/*.module</exclude-pattern>
    </rule>
</ruleset>

The tradeoff is that this also impacts other functions in this file. Though this file should only contain hook implementations at this point we could consider adding rules such as functions without hook implementations.

[danepowell]

Can someone affected by this please test and verify https://github.com/acquia/coding-standards-php/pull/66 ?

[igoragatti]

It "worked" for me.

I had to change from docroot to web. And since I use .inc files I had to add them to the exclusion as well.

Other than that, it worked.

[fiasco]

I think we should at .install files too.

[danepowell]

I updated the PR, please test again

[b-sharpe]

Sorry for long delay here, Test PR on multiple projects and working great, thank you.