Stop using {@inheritdoc} in DocBlocks, use [#\Override] instead

Is {@inheritdoc} used by the API module to create the documentation pages on api.drupal.org?
IDEs could be able to find the documentation from a parent method, but I think that the API module still uses {@inheritdoc} to decide what documentation is shown for a method.

I am glad that I found this already existing issue and I did not need to open a new one :) let's go for this!

@apaderno: Yes, I think so, too.

Instead of dropping {@inheritdoc}, how about we replace it with the PHP 8.3 #[Override] attribute?

API module can use that instead of the {@inheritdoc} tag, and we get the benefit that attribute brings (see https://stitcher.io/blog/override-in-php-83).

Something we're considering for https://github.com/previousnext/coding-standard/issues/21 is removing these tags (without replacement) and using SlevomatCodingStandard.Commenting.UselessInheritDocComment as it suggests inheritance is implied.

So a vote for removal without replacement here.

When 8.3 becomes our minium, #5 sounds interesting!

Do we need to wait for that?

We need API module to understand that attribute, yes.
IDEs detect inheritance with PHP directly I think?

We don't have to wait, but we would have to define the attribute ourselves and then deprecate it and remove it favor of the built in attribute. Maybe I'm missing something.

An attribute doesn't need to have a class defining it: https://www.php.net/manual/en/language.attributes.classes.php

PHP < 8.3 will just treat it as a custom attribute without a class and do nothing with it.
PHP < 8 will just treat it as a comment.

Yeah pretty sure we've done similar for other attributes in the past.

Seems like we should move forward with adopting it since it serves the same propose we use inheritDoc for with additional language features that theoretically makes it more useful.

Thinking through more this actually opens up some options for developers too. The phpdoc standard specifies inheritDoc should be the only documentation which can be annoying when you want to provide additional context to your method. The attribute shouldn't come with this limitation so we can improve documentation in some cases.

> The attribute shouldn't come with this limitation so we can improve documentation in some cases.

I thought of that too, but the reason that inheritDoc doesn't allow additional docs is because how you merge them is a complex can of worms (there's an issue about it -- see the comments: #1994890: Allow {@inheritdoc} and additional documentation). That problem would exist with the Override attribute too.

Had to think on that some. I don't know if we run into the same problem precisely because this solves one of the problems causing that complexity. Basically the problem is a classic case of complexity, we use {@inheritDoc} to mean two thing.

1. Document everything/"copy" from parent
2. Explicit override documentation

In some respects, #1 is kinda pointless for all the reasons being discussed. Implicit documentation is the norm, tools are better, etc. If that where the only reason we did this #6 and this issue would make a lot of sense. It does fulfill the Drupal "always have _some_ documentation" idea but I feel that might be a dying sentiment.

#2 however is pretty powerful especially for something like Drupal because we can see when a method is stranded by a refactor or vendor change without hours of digging through git logs possibly through multiple repositories.

By separating these concepts we can require #Override to explicitly capture #2 and be more flexible with one #1. And if you're using @inheritDoc and we've basically solved #1994890: Allow {@inheritdoc} and additional documentation because if you want to provide additional documentation just drop it per the phpdoc standard and the explicit inheritance isn't lost.

Jumping in mid discussion I didn't actually read the summary and though this was a different issues not about removing inherit[dD]oc.

I guess it goes without saying I'm strongly of the opinion inheritDoc(or some analog) provides real value. The Symfony change actually came from me pushing for better adoption in their project so I've got a bad track record though.

Maybe the summary is out of date but I'd like to address some points.

> PHPCS Fixer also dropped it after Symfony
We don't use a lot of PHPCS' rules so 🤷

> In addition. see https://docs.phpdoc.org/guide/guides/inheritance.html#the-inheritdoc-tag (note 'Doc' with capital D):
Yeah we chose to stick with the older standard though the phpdocumenation implementation claims to be case insensitive. I remember pushing to update it but I can't find the issue. 🤷

> is kind of Drupalism in both usage and syntax.
The wording can be confusing but the documentation actually explicitly acknowledges the way Drupal uses it. It just says it doesn't match their normal pattern for tag naming since its "inline" and that they'd like to add a new one, @inheritDoc, without the brackets. But 10 years after that comment was added I don't think we should hold our breath.

So while its dropped from Symfony but its still pretty widely used in this manner and our usage follows the PSR draft document. Its usage is not only not a Drupalism, its not even really a phpDocism since its the same tag in jsdoc, typedoc, javadoc, and even a similar annotation in .net. All with similar if not identical meaning and usage to Drupal's usage.

I think the strongest argument for moving away from it is actually that most of those languages also have moved to annotations which is also seems to be the push in PHP.

I kinda brain dumped on this issue and probably stalled it. Sorry.

tl;dnr. IMHO we should move forward with #[Override].

I do that too on issues sometimes :)

I think in summary:

- for people who want to keep {@inheritdoc} (such as me!), #[Override] is acceptable, because it still looks like docs and it conveys the same thing
- for people who want to remove {@inheritdoc}, #[Override] is acceptable, because it performs a useful DX function.

What we need now is support for the attribute in API module.

I'm surprised that #5 has made [#Override] such a popular choice in this issue. Especially since the linked Stitcher blogpost makes a good case against its existence and usage.

I realise we should not prematurely optimise, but I'm quite hesitant in adding an attribute, which is a runtime check, for something that's essentially documentation (and thus belongs in comments). I'm assuming the impact of an attribute is generally negligible but I do wonder whether that's still true if we're talking about 1000s of annotations. grep -R '{@inheritdoc}' core | wc -l for 11.x shows 119507 instances of {@inheritdoc}.

As for the existance/usage of {@inheritdoc} itself, it currently provides an easy cmd + click target in IDEs to go to the first class/interface in the chain that provides docs (in PHPStorm). I can see that possible becoming a bit more challenging if it's not there, but I'm sure I can figure out what other keyboard shortcut for that exists.

Re #18 and it being a runtime check, some excerpts from the PHP internals discussion

Just to make sure there is no misunderstanding I'd like to clarify that the proposed check of this RFC is not a runtime error. It's a compile time error, just like the check for incompatible method signatures during inheritance. In fact it shares a non-trivial amount of the existing logic of that check, because it's so similar.

My understanding is that once the class is successfully sitting in Opcache there will not be any further overhead.
— source

and a response

Although OPcache is ubiquitous in production environments now, it's not obligated and the cache only lasts as long as the SAPI process.
— source

This may already be clear to everyone but it wasn't for me (:

Update to new issue template.

Next steps are finding 3 supporters for this issue, updated the issue title and summary.

@borisson_ et al, I would like to see think if we find another Drupal-aligned PHP project with a similar rule, we use that as an endorsement.

PHP has had it for 3 years now, there should either be implementations or discussions about it. This is also one of those features that doesnt require end users to update their PHP for code to continue to run, ie, syntax errors.

From what I can see, PHPStan provides this extra opt-in parameter, which looks nice: https://phpstan.org/config-reference#checkmissingoverridemethodattribute

Rector has https://getrector.com/rule-detail/add-override-attribute-to-overridden-m...

There doesnt appear to be anything in code sniffer or slevomat set.

If we decide to implement something in this project for code sniffer, we should also have a parallel task for enabling the PHPStan parameter, while using the rector for mass-migration.

@borisson_ et al, I would like to see think if we find another Drupal-aligned PHP project with a similar rule, we use that as an endorsement.

Other projects don't usually influence our coding standards very much. This step is to ensure that we in the coding standards community want to change this.

I added myself as supporter + filled in the benefits.

What does 'allowing more specific return tags where possible' mean?

class Foo implements Bar {

  /**
   * {@inheritdoc}
   * 
   * @return NodeInterface
   */
  public function qux(): EntityInterface {
  }

}

This is currently not allowed, but if we stop using inheritdoc and use this instead, it would become like this and would be allowed.

class Foo implements Bar {

  /**
   * @return NodeInterface
   */
  [#\Override]
  public function qux(): EntityInterface {
  }

}

It still wouldn't be allowed, because API module wouldn't know how to handle it. There has already been a long discussion about allowing the something like your first example, and it's too complicated to define how the parent docs and the child docs would be merged.

PHP has return type covariance. You may use a more narrow type as the return when overriding a method.

This is valid:

interface EntityInterface {
}

interface NodeInterface extends EntityInterface {
}

class PluginBase {
    public function qux(): EntityInterface {}
}

class Plugin extends PluginBase {
    public function qux(): NodeInterface {}
}

It still wouldn't be allowed, because API module wouldn't know how to handle it. There has already been a long discussion about allowing the something like your first example, and it's too complicated to define how the parent docs and the child docs would be merged.

This is true, but we will need to rewrite part of the api module because of this change anyway, so I think that means we can do this, and perhaps part of #1994890: Allow {@inheritdoc} and additional documentation together? Or am I being overly optimistic do you think?

@znerol, yes that works, but not if you're returning an array of objects for example ;)

> This is true, but we will need to rewrite part of the api module because of this change anyway, so I think that means we can do this,

It's not about doing a rewrite, it's about the complexity of the logic. Please see past issues about it, and @jhodgdon's explanations about it.