Allow {@inheritdoc} and additional documentation

I would be fine with that.

I must have misunderstood our policy, I've been doing this all along.

+1

+1

Most of the time you might want to either add another parameter and document what you override in a certain method.

I am already using it in core so +1.

Sigh. Please can we not do this?

We had a long discussion on the other issue and while phpdoc uses inheritdoc this way, other doc systems don't, and I do not agree that it's being mostly used that way in core (in most cases I think it's being used the way we adopted, to inherit everything from description to parameters, isn't it?).

If we do need to change this standard, then we need to either say "it means inherit everything" (what we're doing now mostly and what our standard is) or "it means inherit just the long description" (what phpdoc does). In the latter case, you would need to do something like this:

/**
 * Short description here.
 *
 * {@inheritdoc}
 *
 * @param etc.
 */

and you would need to list out *all* the params and return value. Which I think is precisely what we wanted to avoid in the previous issue. How is this a benefit?

If I've misunderstood something, please comment here or better yet update the issue summary with a clear idea of what you want the standard to be for inheritdoc.

I just did a grep through Drupal 8 core for inheritdoc.

The vast vast majority of uses are just

/**
 * {@inheritdoc}
 */

as is currently specified in our standards.

There are actually only 2 exceptions outside of core/vendor:

a) core/modules/user/lib/Drupal/user/Form/UserRoleDelete.php

  /**
   * {@inheritdoc}
   * @param \Drupal\user\RoleInterface $user_role
   *   The role being deleted.
   */
  public function buildForm(array $form, array &$form_state, RoleInterface $user_role = NULL) {

This case -- looks like they intended it to mean "Inherit all of the documentation for short description, long description, other parameters, see alsos, and everything; add/override just this one parameter".

b) This same format is also used in:
core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmForm.php

Since we are only doing this in two places in core, I do not think that we need to amend the standard. I think we just need to fix these two pieces of documentation.

Actually, they are both functions that override this nearly useless documentation from ConfirmFormBase that should actually probably be {@inheritdoc} anyway:

  /**
   * Implements \Drupal\Core\Form\FormInterface::buildForm().
   */
  public function buildForm(array $form, array &$form_state) {
 

And that in turn should be inheriting this documentation from FormInterface:

  /**
   * Form constructor.
   *
   * @param array $form
   *   An associative array containing the structure of the form.
   * @param array $form_state
   *   An associative array containing the current state of the form.
   *
   * @return array
   *   The form structure.
   */
  public function buildForm(array $form, array &$form_state);

So really... In both of these cases, probably the BuildForm documentation for the two specific classes should just be following our standards for form constructors, not using @inheritdoc:
http://drupal.org/node/1354#forms

So... Given that the rest of core is currently using inheritdoc according to our current standards, do we need to amend them? If so, I'd like to see a concrete proposal of how and some examples of where people would like to use the new standard. Because these two usages do not match how PHPDoc defines it (where it just means "inherit the long description text) nor our current standard (where it means "inherit everything", but it has to be alone).

Otherwise, I'd propose fixing those 3 pieces of documentation in ConfirmFormBase (to use @inheritdoc) and the two specific confirm forms (to follow standard form constructor documentation), and closing this as "won't fix".

There will be hundreds of these coming into core soon. 90% of issues listed on this page will be doing it this way: #1971384: [META] Convert page callbacks to controllers

There are only two in HEAD because they were just the first to get committed.

In all form callbacks in D7, we do not redocument $form and $form_state. We just document the added parameters.

That's what I'd like to continue to do. Only when adding new @params in a method like buildForm.

But we already have a long-standing standard for form builder functions:
http://drupal.org/node/1354#forms

Can't we use that? This @inheritdoc does not make sense to me, as it is used on those pages:
- It doesn't conform to how @inheritdoc is defined in PHPDoc, which just says "it means inherit the long description". In PHPDoc, you still need to provide the short description (first line) and all the params, which is not being done in these samples.
- There is no first line telling what form is being created, like in our old form builder docs standard. This information could be useful?
- It is not any more compact than our current form standard.
- It doesn't put @ingroup form in there.
- There really isn't much long description to inherit from the base form builder function anyway.

I just did this for something else, and thought it might be useful here:

ag -B 10 --literal 'public function buildForm(array $form, array &$form_state, ' core

output attached, but it's nicer when run, because it has colors.

It shows 29 implementations of buildForm dont document additional parameters at all.

Dang it. There are now a LOT of these in core. It simply isn't going to work the way it is being done.

We have three purposes for documentation of our API:

a) For people to read. In this case, things like

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\search\Plugin\SearchInterface
   */

are fine because they are perfectly clear for people: "It's the same thing but with a different return value class".

b) For parsers to read and display. In this case, the parser we mostly use is the API module and it only can parse stuff we have defined in our standards.

c) For IDEs to do intelligeent things with. This might work for them, I don't know, I don't use one.

So... Right now we are using the API module to parse/display docs. It doesn't support this. Neither does any other existing standard for docs parsers. We need to adopt standards that make sense for our project but also that are consistent with FIG and PHPDocumentor and Doxygen as much as possible. What is currently in core and proposed here is NOT.

So we need a proposal that would be and then we need to go fix all the docs that were put in that violated our standards in the first place.

Coding standards decisions are now supposed to be made by the TWG

This question has recently come up again on #2481775: Fix @return doxygen in ConfigEntityStorageInterface

A few notes:

a) Here's the issue where we previously discussed @inheritdoc and adopted it initally:
#1906474: [policy adopted] Stop documenting methods with "Overrides …"

b) The standards we currently have: https://www.drupal.org/node/1354#inheritdoc

c) Notes about how other projects are using the inheritdoc tag (and ours currently):

So there are current uses in Core that are misusing @inheritdoc according to our current standards. It seems that people want our usage to be "Inherit everything, and then allow us to put in stuff that overrides certain @params or whatever we want". But this is actually pretty hard to parse out, in practice. The usage in PHPDoc where it's just "inherit the long description" is clear enough, and in JavaDoc where it's "inherit docs based on context", and in JSDoc and our current API module where it's "all or nothing". But no one is offering what people are asking for here, because it's really hard to code in a docs parser.

So. If we want to have more fine-grained inheritance, I think we can have something like PHPDoc or JavaDoc (and if so we should define a different tag rather than overloading our current JSDoc-like standard), but not what is being asked for here.

[Edit: I screwed up in the last two paragraphs between JavaDoc and JSDoc in my comments, fixing this]

One common use case for this is to explain why we're overriding a parent method. Eg, from Flag module:

/**
 * @inheritdoc
 *
 * We override the render() method to add helpful text below the entity list. 
 */

See #12. If we are going to change what @inheritdoc does, I think we should pick one of the behaviors that other doc parsers have, rather than inventing something completely new or worse yet, inventing a behavior that hasn't been adopted elsewhere because in practice it is not workable for a docs parser. The specific request in this issue is NOT one of the behaviors that other doc parsers do.

At this point I do not think we should actually change what @inheritdoc means, because most of our uses are what we defined it to be, and these would all break if we changed the meaning.

@alexpott, @xjm, @dawehner, @YesCT, and I had a discussion about this today at BADCamp.

Here is what I think we would like to propose -- and I think the API module would be able to support it if we adopt this idea:

----------
a) If a method, to be documented correctly, would need to *change* anything in the parent class/interface method docs in order to be correct (such as a different 1-line description, main description block, parameters, or return value), then it should NOT use @inheritdoc. It should instead have a full doc block with its own params/return docs and everything.

b) If a method can inherit all the existing docs (they all apply), but it would be helpful to add something to those docs, then it can do something like this:

/**
 * {@inheritdoc}
 *
 * Some explanation lines about why this was overridden, implementation details, etc.
 *
 * @throws \Some\Additional\Exception
 *    Under some condition the original method didn't have.
 *
 * @see something_different
 */

------

Thoughts?

Note also that #15 seems to work in PHPStorm as well, based on @dawehner's testing.

#15++.

The main reason I "add" to inheritdoc is to change the @return type, which would not be allowed under #15. :(

Why would you be changing a return type...? Even if you're making it more specific (subclass of the parent's return), that wouldn't be legal in PHP 7's native syntax anyway. And you can't do that with parameters, either.

PHP 7 return types aren't covariant? That sucks. But PHP 5 doesn't care at all!

Here's an example of how we use this.

A Drupal\Component interface defines the @return. It's implementation uses {@inheritdoc}
The Drupal\Core implementation wants to define a corresponding @return, so it does.

namespace Drupal\Component\Plugin {

interface ContextAwarePluginInterface {

  /**
   * Gets the context definitions of the plugin.
   *
   * @return \Drupal\Component\Plugin\Context\ContextDefinitionInterface[]
   *   The array of context definitions, keyed by context name.
   */
  public function getContextDefinitions();

}

class ContextAwarePluginBase implements ContextAwarePluginInterface {

  /**
   * {@inheritdoc}
   */
  public function getContextDefinitions() {
    $definition = $this->getPluginDefinition();
    return !empty($definition['context']) ? $definition['context'] : array();
  }

}

}

namespace Drupal\Core\Plugin {

class ContextAwarePluginBase extends ComponentContextAwarePluginBase {

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface[]
   */
  public function getContextDefinitions() {
    return parent::getContextDefinitions();
  }

}

}

This saves us from needing to use inline @var typehints after every call.

This is even further useful in plugin collections, where the parent says @return mixed and each individual implementation defines the plugin interface they return.

RE changing the @return --

When we discussed this in the BADCamp sprint room yesterday, our thinking was that if you are *changing* something in the original documentation, that indicates you are making a change to the "contract" that is documentation, so you'd better just redo the whole thing. So philosophically, the idea is "Additional information is OK but not changes". I realize that a change like "For convenience of IDEs document that you're actually returning a subclass" is not really a change to the contract, but ... well, it's still a change to the docs.

The other reason for that standards proposal is practicality. For the API module to have logic like "@inhertidoc + something" means "Inherit all the docs and add some more", that's fairly straightforward -- although already requiring some rearchitecting, because currently @inhertidoc is treated the same as "This function has no docs"... I'll have to figure out some way to store the additional docs and also have a flag to say that this method has some... and allowing the additions to contain things like @see and @throws is already going to be annoying enough to code.

But allowing functions to pick and choose what they override... really gets complicated. Especially if people want to do things like "Override just this one parameter's first line docs with the type but keep the other parameters and all parameters' docs", or "Override the @return first line docs with the type but retain the description", as proposed in #21. Ugh.

So the proposed standard is kind of a balance between what we have now (all or nothing on @inheritdoc) vs. allowing *some* flexibility. As API module maintainer, I'd really rather not support any additions at all, and I really don't want to do the truly major rearchitecture that we'd need in order to support a more fine-grained overriding capability.

And... another question. I know you want to do #21 for IDEs... if you do that in PHPStorm, when it shows you the docs does it retain/inherit the @return description from the base class, or does it replace it with just the return type line? And what if you have one substitute @param? Just curious.

This keeps coming up over and over on other issues. I am updating the issue summary with the reasons why we cannot do this in the way that everyone wants to. Really, we can't. There's a reason that no other docs parser supports it: it is not feasible in a docs parser to support it.

Overriding @param or @return seems a bit much to me as well. I do think the ability to add extra notes would be useful though - this just came up again in #2674780: Unexpected API change in UrlGenerator::generate().

Can I just say one more time: There is not a single other API documentation system out there that allows @inheritdoc to be "inherit everything, with additions". Take a look at the chart in the issue summary.

There is a reason for that -- this would be very complicated to implement in practice, for a docs system where files are parsed once and the results are saved in a database for later viewing, which (as far as I know) is how all of these types of systems are done.

So I know that everyone wants to do this, and I can see the lure, but... it just is not practical and even if it was, it would make our tags even more non-standard than they already are. Sorry. I don't think this is OK at all.

I believe @jhodgdon makes a valid argument. I mean one can always workaround that by putting the additional documentation onto the top of the function or actually document the parameters again, but maybe with less verbose bits + @see parent or so.

Yes. For instance we can always do something like:

/**
 * Returns the foo bar baz.
 *
 * Overrides \Whatever\Whatever\Whatever\TheBaseClass::whateverMethod()
 * in order to add a 'fluffy kittens' element to the returned array. See the base
 * method documentation for details on parameters and return value.
 */

This is not difficult to write or maintain, and gets the point across.

FTR, I'm fine with no additions when using @inheritDoc other than "more longdesc". That seems fine for the majority case, and implementable.

No, you've misunderstood. Take a look at the table in the issue summary again. Your choices, assuming you want our docs system to mirror some other use of @inheritdoc, are:

a) @inheritdoc is always alone and means "inherit everything", which is basically what we have now and what JSDoc supports.

b) @inheritdoc doesn't mean "inherit everything" but something more limited. I do not think that anyone really wants this, because it would mean that the vast majority of our uses of it would not work as people expect.

Any proposal other than these two is very difficult to implement, and would lead us to an even more non-standard docs system than we already have.

I like the proposal in #15 AKA "1." in the issue description.

It seems like it would satisfy #24.

Sigh. Everyone likes that idea. But it is not feasible to implement, as has been discussed multiple times on this issue. No other documentation system out there implements this idea, because it is so difficult to manage. This was already in the issue summary> I have added a boldface header to that part of the issue summary, because people chiming in and saying they would prefer option 1 happens over and over. I also clarified why it isn't feasible to implement.

Oh sorry about that. It's a bit confusing, I thought you'd been speaking out against "2." I didn't realize you had changed your mind between #22 and #23 and figured "2." was "the way everybody wants it".

It's interesting that extracting a specific piece out of the parent documentation like the longdesc is more feasible than grabbing the whole thing. I wouldn't have guessed that.

Having to repeat the summary line and params and return all the time seems less convenient than the way it is now, even if it's more flexible. I think it would be better to leave it how it is than do that.

"3." could still be useful for some cases though. There's a couple of ways it could work without interfering too much with the current functionality.

- Change the functionality of {@inheritdoc} as proposed, and also change the standard so that it's ok to have functions without a comment block if their documentation should be inherited. I'm not sure if that's something that could be considered, but I remember when all files were supposed to have @file tags and now most of them shouldn't. It seems like a similar change.

- Make a new tag for it instead of changing what {@inheritdoc} does. Maybe something like {@parentdesc}. It makes me wonder if the longdesc is the only thing that could be extracted from the parent. I really have no idea about what is feasible, but if that was done it would be neat if there could be more tags like {@parentsummary} {@parentparams} {@parentreturn} etc. (Edit - I guess that's not really different than a context sensitive @inheritdoc . I just think it would be good if it could be done while still having a way to grab the whole documentation from the parent even if each comment block could only be one way or the other.).

#33 (second idea) is an interesting proposal. The main drawback I see is that it is yet another Drupalism, meaning that it is less likely that IDEs will support it.

Just so you understand the issue... the reason that partial inheritance of documentation is a problem is that when you parse the file that contains class A, you may not yet have parsed the file that contains class B or interface C or wherever up the chain of inheritance actually has the documentation. And furthermore, when you first parse class A, you may be inheriting docs from interface C, while later on class B that is in between them in the inheritance chain adds its own documentation to a method, and now class A should inherit class B's docs and not interface C's docs.... it's kind of complicated to deal with. With the current definition of what @inheritdoc means, at least it's binary: you treat @inheritdoc as "this method has no documentation defined", and try to inherit the full docs from the first parent you can find. But if you have to get into pieces of the docs, some of which may be defined on class B and others not until you get to interface C... things get complicated. To say the least.

So... I guess my point is, it will require considerable work to implement this type of logic for the API module. Then you'd also need to convince IDE developers to support these new tags that the Drupal project came up with, for them to be useful outside of api.drupal.org (which a lot of developers don't even use, as they just live in their IDEs). So, we shouldn't adopt these new tags, or change the meaning of @inheritdoc, unless there's a huge need to make the change, and if making the change will be a clear improvement for developers who need to write and maintain documentation.

As things stand now, developers have these options (assuming they follow our current standards):

a) Use @inheritdoc when they want to inherit all the docs from a parent class/interface.

b) If the inherited docs need clarification or a change, do something like #27.

To me, this seems sufficient to be able to write Good Documentation (TM) -- I don't see a strong need to add new inherit-parts functionality. I don't think this standard is really standing in the way of good documentation writing, and I don't think it's worth the considerable trouble it would be to change it.

I've been making the assumption that one of these proposals was "the easy one." It does seem like it would be a small improvement for that much work.

I like #27 but it still suffers from the problem mentioned in #1906474: [policy adopted] Stop documenting methods with "Overrides …".

It is a huge pain when you change a namespace in an interface, then you have to go through and find every implementation and edit each method's docblock to reflect the change.

What if instead of having the reference written out there was a tag that would be replaced with the correct one? Would that be more feasible since it wouldn't be trying to grab the actual documentation from the parent?

Not sure what you mean in #35 "What if instead of having the reference written out there was a tag that would be replaced with the correct one?" -- you mean a tag like @parent? Doesn't seem all that workable.

However, I will just say that the API module is perfectly capable of figuring out which class is meant, in a doc block, without any namespace -- as long as the class is in one of the "use" namespaces in the file. IDEs, I think, don't even parse doc blocks except for things just after @ tags, like "@see \Foo\Bar\Baz... " or "@return \Foo\Bar\Baz... ".

But we do have a docs standard currently saying "You must use a namespace", the reason being that our classes do not have unique names across namespaces, so it's probably easier to read (for a human reader not using api.drupal.org).

Then again, we also have a written, but completely outdated, standard saying our classes should have names that are unique without the namespace. See item 10 in
https://www.drupal.org/node/608152#naming
We totally don't follow that. In fact, just the opposite. That standard really needs an update to reflect reality. Anyway that is a side trip...

Well not like a tag to say what the parent is, I mean an inline tag like {@parent} so you could write

Overrides {@parent}

If the API module can figure out what class is meant then I feel like it should be able to replace {@parent} with that value.

Maybe I don't fully understand the reasoning behind the decision to "Stop documenting methods with "Overrides …"". It seems like it's about Overrides being difficult to keep up to date and an inline tag would solve that part by letting the API module handle it.

Bringing Overrides back without any changes might not be so bad, but I'm under the impression that that's exactly what people don't want and the reasons make sense to me.

If the point of "Overrides..." is to indicate that there is more information in the parent documentation that is worth seeing then another alternative could be a tag that indicates the same thing. Maybe something like @seeparent. People with an IDE or on the api site might already know it's overriding something but a tag would tell them when the parent documentation is relevant. It would also tell people in a text editor that an override is happening. Even with a text editor it's not hard to find the corresponding use statement, and it's better than if the namespace is in the docblock but it's an outdated reference.

As I recall (not reading all the discussion on that issue again), the main thrust of that discussion was that the standard doc block that only had the one line saying "Overrides \name\of\the\class::method()" was that it was totally pointless. We went with @inheritdoc because it concisely said the same thing.

The point of #27 is slightly different. The point there is that if plain @inheritdoc is not enough for a given method, you would use a standard format that said something like "Overrides \base\class\name::method() because ....". That is not the same thing at all -- there is a reason you want to say more, and the point of putting the class name in there is that it would point people to the base class's method so they could see what that method does.

It seems that proposed changes are de facto standard, aren't they?

Check with pcregrep in drupal /core directory gives a lot of hits where @inheritdoc tag is not the only line in the docblock, as suggested by inheritdoc comment standard:

pcregrep --recursive --include='.*php$' --multiline '\n\s*\*\s*\{@inheritdoc\}\n\s*\*\n\s*\*' .

You may also check /vendor/symfony or /vendor/zendframework, where @inheritdoc sometimes is followed by additional comments.

Yes, there are many places where it's being used. That doesn't mean it will work or that it's a good idea. See issue summary -- there is a reason that, as far as I know, there is not a single documentation system or IDE that works the way people are hoping it would work: ot is VERY technically difficult to implement. So yes, we can write "inheritdoc plus more" in doc blocks, but actually turning that into meaningful output on a site like api.drupal.org is another thing entirely -- not really possible to do.

Also, I think that most of us agree that the Drupal community should not even consider adopting standards that are purely invented by Drupal. We are instead moving our coding standards towards more conformity with other projects. And this idea of "inheritdoc plus more" is not supported anywhere else. There is a table in the issue summary showing how PHPDoc, JSDoc, and other docs systems use the inheritdoc tag. As far as I am concerned, we can choose between those possibilities, but inventing our own meaning is not going to work out.

So just because a lot of code in Drupal Core violates the existing standard doesn't mean we should adopt a unique and unfeasible to implement (as far as making use of the documentation) standard.

@Chi --The new item you have added is the same as an existing one, so I am combining them in the table.

Fixed the issue summary to state that PHPStorm supports option 1 now.

Are you certain about that? In my experience looking through many other docs systems, no one supports "option 1".

So can you show an example where the base doc has @param, @return, and things like this, and the inherited doc has @inheritdoc plus, for example, an @see or some more text, and PHPStorm shows you the entire doc from the base plus the added information? I don't personally use PHPStorm... It could be that they do this, but is it possible they are only inheriting the description?

PhpStorm (tested on 2019.3.2) does currently support option 1 and also allows the user to override parameter and return definitions. For example, the following code will result in the attached documentation:

class ExampleForm extends FormBase {

  /**
   * {@inheritdoc}
   *
   * Example description.
   *
   * @param bool $exampleBool
   *   Example boolean.
   * @param \Drupal\node\NodeInterface $exampleNode
   *   Example node.
   *
   * @return \Drupal\Core\Form\FormStateInterface
   *   Example form state.
   *
   * @see \Drupal\example\Form\ExampleForm::buildForm()
   */
  public function buildForm(array $form, FormStateInterface $form_state, $exampleBool = NULL, $exampleNode = NULL) {
    return $form;
  }

}

OK, that's interesting. phpDoc does not support that way of doing things, at least officially.

In addition to PhpStorm (tested on 2019.3.2), this is also the case for VS Code 1.42.1 where Option #1 is concerned.

FWIW, Symfony has abandoned the usage of @inheritdoc in 6.2,

It looks like this PHP Doc is useless

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

phpDocumentor support is similar to #45, also overriding parameter & return declarations. The layout is similar to the VS Code Intelephense version.

This code:

class A {
    /**
     * Parent method summary.
     *
     * Parent method long description.
     *
     * @param bool $input The input.
     *
     * @return bool
     */
    public function method($input = TRUE) {
        return $input;
    }
}

class B extends A {
    /**
     * {@inheritDoc}
     *
     * Child method additonal details.
     *
     * @param string $input The input.
     *
     * @return string
     */
    public function method($input = 'default') {
        return $input;
    }
}

Outputs:

Converting to new issue template