[10.0.x and 9.5.x backports] Don't allow {@inheritDoc} annotation in PHPDocBlocks

I am very confused, my phpstorm tells me there are 85 results, this patch doesn't find that many?

Adding related issue from the Coding Standards project, #3060580: Allow inheritdoc and inheritDoc?.

Using a dynamic rule provided by the Slevomat package is a really nice solution to this!

10.1.x

With the patch applied:

[ayrton:maintainer | Mon 17:12:34] $ grep -rl "inheritDoc" * | grep -v "vendor" |grep -v "node_modules"
core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionClass.php
core/lib/Drupal/Component/Annotation/Doctrine/SimpleAnnotationReader.php
core/phpcs.xml.dist

I'm assuming the Annotation component is what #6 is referring to as well. I confirmed the files in the component are already ignored for coding standards. And hopefully #3252386: Use PHP attributes instead of doctrine annotations will get rid of that component for us. I was also able to commit the patch locally, which means PHPCS passed.

10.0.x

Rather than cherry-picking the patch to 10.0.x, I applied and committed it there too in order to check it separately. Same grep results:

[ayrton:maintainer | Mon 17:21:14] $ grep -rl "inheritDoc" * | grep -v "vendor" |grep -v "node_modules"
core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionClass.php
core/lib/Drupal/Component/Annotation/Doctrine/SimpleAnnotationReader.php
core/phpcs.xml.dist

PHPCS passed there locally as well.

9.5.x

The patch does not apply. Can we get a backport version?

Thanks everyone!

Sorry, folks, I spaced out. We need a version of the changes only, without the new rule enabled, for the 10.0.x backport. Reverted from 10.0.x for now.

Oops.

This broke CI in 9.5.x:

FILE: ...tml/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 37 | ERROR | [x] Use of annotation @inheritDoc is forbidden.
    |       |     (SlevomatCodingStandard.Commenting.ForbiddenAnnotations.AnnotationForbidden)
----------------------------------------------------------------------

Reverted from both 10.0.x and 9.5.x, as we cannot add new coding standards there now.

Code changes only, no new sniff - you can still run the sniff locally to check you got everything, or upload a test-only patch with the sniff included if you want to prove it with DrupalCI. The 9.5.x patch will need to cover AjaxCssForm as well.

OK:

1. The 10.0.x patch just needs to be identical to the 10.1.x patch, with no rule. Because that passed tests.
2. The 9.5.x patch needs to be created separately, because 9.5.x includes more inherirtDoc than 10.0.x that we need to correct.
3. We keep the same coding standard across the three branches for ease of backports and so that the community gets use to the standards. We just only enforce it with a rule on 10.1.x because that's a disruption.

I have no idea what's going on with the git integration. I reverted it from 10.0.x last night and I swear I never even committed it to 9.5.x. I also swear I did not re-commit them, so I don't understand how there was anything to revert. Anyway. The path forward is described above :)

This broke 9.5.x Ah, @longwave already said this in #21 and reverted. 👍

Better status.

FWIW, Symfony has abandoned the usage of @inheritdoc,

It looks like this PHP Doc is useless

.

https://github.com/symfony/symfony/pull/47390

MR created with all the {@inheritDoc} changes.
I found those files in the Drupal\Component\Annotation namespace were checked and confirmed they were all ignored.

I tested it and it looks good for me.

I check the patch like this:
I copied this sniffs to core/phpcs.xml.dist:

<!-- SlevomatCodingStandard sniffs -->
<rule ref="SlevomatCodingStandard.Commenting.ForbiddenAnnotations">
<properties>
  <property name="forbiddenAnnotations" type="array">
    <element value="@inheritDoc"/>
  </property>
</properties>
</rule>
<rule ref="SlevomatCodingStandard.Commenting.ForbiddenComments">
<properties>
   <property name="forbiddenCommentPatterns" type="array">
     <element value="/@inheritDoc/"/>
   </property>
</properties>
</rule>

I ran this command to find any issues:
./vendor/bin/phpcs --standard="core/phpcs.xml.dist" --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml --sniffs=SlevomatCodingStandard.Commenting.ForbiddenAnnotations,SlevomatCodingStandard.Commenting.ForbiddenComments core/

I fixed all errors, ran the previous command , it gave me no errors. So I removed the code in core/phpcs.xml.dist, generated a patch and compared it with the Fabian's. They were the same.

Thanks @rpayanm.

In addition to testing the same steps as #32, I reviewed the changeset with git diff --color-words to include that it includes only changes to mis-capitalized docblocks. I also spot-checked the phpcs results against grep:

[ayrton:maintainer | Fri 14:07:51] $ grep -rl "inheritDoc" * | grep -v "vendor" | grep -v "node_modules"
core/lib/Drupal/Component/Annotation/Doctrine/StaticReflectionClass.php
core/lib/Drupal/Component/Annotation/Doctrine/SimpleAnnotationReader.php

Those remaining instances are ignored in PHPCS checks as per #13, so this is the complete fix for 10.0.x. Therefore, committed there and moving to 9.5.x for the final backport.

In the process of reviewing #31, I did notice that there is an additional pattern not currently covered by the phpcs rule, where the curlies are missing but the correct casing is used:

[ayrton:maintainer | Fri 14:09:10] $ grep -ril " @inheritdoc" * | grep -v 

Most of the results for that are generated CKE5 assets and therefore out of scope for our PHP coding standards. However, there were two results in core PHPUnit tests:

core/tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php
core/tests/Drupal/KernelTests/Core/Database/DriverSpecificDatabaseTestBase.php

E.g.:

  /**                                                                           
   * @inheritdoc                                                                
   */
  protected function setUp(): void {

This issue is also present in 10.1.x and not covered by the current rule. It's out of scope for this issue, but maybe we could file a followup and use the Slevomat rule to check for the no-curly format?

Thanks!

Thanks, @rpayanm, and @xjm for the tests and feedback.

MR was created with all the {@inheritDoc} changes but now for branch 9.5.x.
Those files in the Drupal\Component\Annotation namespace were checked and it confirmed they were all ignored.

The review process can be made with comments #32 and #34

Thanks!

The patch does not apply.

Actually it applies fine if I use the "plain diff" link and 9.5.x HEAD. I also just clicked the rebase button so that tests will keep running.

I reviewed the patch from #36 following the review process in comments #32 and #34 and I see the patch ok.

This was tagged "Needs followup" for #3331633: Don't allow @inheritdoc (no curly braces) annotation in PHPDocBlocks which exists now.

Committed and pushed the backport to 9.5.x, thanks!

I also added a release note and updated the doc at #3331949: 10.1.0 release notes with it.