[policy adopted] Stop documenting methods with "Overrides …"

Setting up the issue properly...

I am not in favor of this proposal. The current documentation standard does give useful information to people who are not using IDEs, whereas "inheritdoc" doesn't.

If I care what the actual documentation for the method is, I always have to go and look up the interface anyway - and that's in the use statement and the class definition. I think {@inheritdoc} gives enough information for us non-IDE users to be able to quickly note that this is where they should go look.

Does the current redundancy actually provide useful information? How? I'm all for @inheritdoc.

The current shape of the standard, mentionning the parent class name, is not maintainable and bound to get obsolete.

example :
MyClassC extends ContribClassB extends CoreClassA
MyClassC::foo() overrides ContribClassB::foo() overrides CoreClassA::foo()
--> So in the current standard, MyClassC::foo() mentions "Overrides \...\ContribClassB::foo()"

Then in the next release, the contrib module changes ContribClassB and remove the override for foo() (now directly inherits CoreClassA::foo())
--> MyClassC::foo() now overrides CoreClassA::foo(), and my comment is incorrect.

There's no way we can keep that information to date.

@inheritdoc exists and is a standard in the outside world, lets use that...

OK, I'm beginning to be convinced... but:

(a) Is @inheritdoc actually the standard? In what docs systems? I haven't looked.

(b) Why would we put the tag in {} when no other @ tag is done that way? Seems quite inconsistent.

(c) Is this just for methods on classes and interfaces, or would it apply to hooks, theme overrides, etc.?

We don't use {} in the Drupal API module for inline tags. We shouldn't adopt it for this tag.

Also, the @inheritdoc tag seems to have a different use in JavaDoc and PHPDoc than what is proposed here: it causes just one section to be inherited (one @param, one @return, one @throw, or the long description), as an addition to other documentation. That is why it is an inline tag -- it isn't a global "inherit the documentation" tag, but rather an "inherit the corresponding piece of parent documentation and insert it here with this other doc I've written" tag.

Also, JavaDoc automatically inherits missing docs when there is no @inheritdoc:
http://docs.oracle.com/javase/6/docs/technotes/tools/solaris/javadoc.htm...
The API module does something very similar now, without @inheritdoc, if you just leave the doc block out.

So... I'm not sure the proposal is very good. It seems that the standard in JavaDoc (and probably PHPDoc) is that you either need to put proper documentation on the method or completely leave it out to inherit the parent docs. @inheritdoc does not do what is proposed here in those two projects.

RE #9 - This example does not prove the point. According to the docs on PHPDoc, it automatically inherits any doc pieces that are missing, and @inheritdoc in this context just says "inherit the long description". This example does not disprove that description at all. Anyway, it's pointless to argue this -- we can use inheritdoc any way we want to.

For purposes of Drupal and the API module let's please not put it in {} since we don't have any other tags we use that way, contrary to JavaDoc and PHPDoc. And let's just say we'll only use it for "inherit everything", not for "inherit this little piece and insert it inline", which is much more complicated to implement.

But, once again: can I just say that in PHPDoc and the API module, if you completely leave the docs out they will be automatically inherited. If you are going to argue that the "Overrides ClassName::x()" is pointless, is it not equally pointless to put in @inheritdoc? And why put it in /** */ since the API module doesn't care?

I say, if we're going to do this, let's make it as simple as possible and just make it

  // @inheritdoc
  function foo() {}

The API module will thereby ignore this comment and inherit the docs (no code change necessary), but we'll have something there so we know it was intentional. And it's short.

And just one more time I'd like to articulate why I still mostly think this whole thing is a bad idea...

A class can easily implement several interfaces and extend a base class, which itself might extend other classes and so on. Someone who is just reading the code (without the benefit of api.d.o or an IDE which would give them this information directly) would benefit from knowing where to look to figure out where the parent method is. Without that information they'd have to guess, and potentially look through several parent classes and interfaces (maybe climbing up the hierarchy several rungs). I think it is a benefit to have the information there. And I'm not sure I buy any of the arguments so far about why this is so difficult and hard to maintain.

On the other hand, here's an idea:

// @inheritdoc \Class\That\Actually\Has\The\Docs::methodName()

Yes, typing out the "Implements..." is a pain. But its not the real problem.
The trouble is actually the "Overrides..." documentation pattern, and the switching between the two.

If we always documented a method by the interface it was *from*, and didn't document which parent class it happened to override (which is provided by api.d.o anyway), we could find a happy medium here.

Seems like #12 would be a solution to #13 - this would point to wherever the method is actually documented (presumably the interface or in a few cases the base class).

#12 doesn't solve the "this is way too hard to keep in sync" problem noted by yched in #4.

Also, regarding {@inheritdoc} vs. @inheritdoc... I tend to use {@inheritdoc} a lot because I'm copying a method from a base class in Symfony and then modifying it. If the other systems Drupalers are going to be exposed to (PHPDoc-using projects, Symfony in particular) are using {@inheritdoc}, what's the value in doing something different and easily-confused?

What @yched describes is which base class up the chain overrides what, and adjusting to that. If the class stops implementing an interface, this is moot, because it will all be deleted.

If we just say "always document the method with what interface it implements", I do think this addresses @jhodgdon's valid point in #11.

And it doesn't mean we have ever-shifting classnames to keep on top of.

Here is an example:
BlockListController currently extends ConfigEntityListController. There may come a time when ConfigEntityListController is actually no longer needed, and BlockListController can go back to extending EntityListController. But when that happens, all of the docs everywhere will need to be switched (which is what @yched was describing).

However, these methods are always coming from EntityListControllerInterface, and if they are always documented as such, we'll never need to switch it.

diff --git a/core/modules/block/lib/Drupal/block/BlockListController.php b/core/modules/block/lib/Drupal/block/BlockListController.php
index 4b424dd..e2988f3 100644
--- a/core/modules/block/lib/Drupal/block/BlockListController.php
+++ b/core/modules/block/lib/Drupal/block/BlockListController.php
@@ -31,7 +31,7 @@ class BlockListController extends ConfigEntityListController implements FormInte
   protected $theme;
 
   /**
-   * Overrides \Drupal\Core\Config\Entity\ConfigEntityListController::load().
+   * Implements \Drupal\Core\Entity\EntityListControllerInterface::load().
    */
   public function load() {
     // If no theme was specified, use the current theme.
@@ -50,7 +50,7 @@ public function load() {
   }
 
   /**
-   * Overrides \Drupal\Core\Entity\EntityListController::render().
+   * Implements \Drupal\Core\Entity\EntityListControllerInterface::render().
    */
   public function render($theme = NULL) {
     // If no theme was specified, use the current theme.

Reasons I don't want to see {@inheritdoc} and reasons I don't want it to be inside /** */ are mostly pragmatic:

1. As the maintainer of the API module, I don't have any other tags that are inside {} so I'd have to write different code to detect {@inheritdoc} that what I already have for other tags.

2. As the maintainer (more or less) of node/1354... We don't have any other tags used in our Drupal docs standards that are inside {}, and I like consistency. Other tags that Java/PHPDoc consider to be inline and require {} like @link etc.: in Drupal API docs, we instead use @tag ... @endtag which I think came from Doxygen. Adopting one tag that is like this is just odd. The only reason Java/PHPDoc need the {} is that you can embed this tag inside other docs, and it seems no one wants this for Drupal (and I certainly don't want to implement it in the API module).

3. As the maintainer of the API module, if we use // instead of /** */ comments, then I will not have to change the code of the API module at all, because it ignores // comments and if there is no doc it inherits, thereby satisfying the intention of @inheritdoc without actually having to do any coding. Always a plus.

Regarding maintainability... All docs are hard to maintain. When we change parameter names, we have to fix docs. When we change function names or class names, or get rid of them, @see references have to be fixed. If we remove a method, ideally we should grep in the code for references to it and make sure they're fixed. I don't see why this is a huge deal or adds more burden than what should already be checked anyway.

Regarding copying from Symfony -- they have way different docs standards than the Drupal project, and we already can't copy their docs. For instance, they have @author and a bunch of other tags we don't support/require, and their formatting is different... Don't copy Symfony docs. :)

So... The questions are:

a) Is it valuable to some people to have information in the method about where it came from (what it overrides or implements)? If so, probably our current doc standard is the best way to convey this information... although we could easily cut out the namespace part with no loss of api.d.o functionality, making the whole thing a LOT less cumbersome and I think more readable.

b) Is it valuable to some people to have a pointer to where the method is documented (which is not always, or maybe even not often, the same as (a) since it would be the "top of the chain" -- the interface rather than the base class)? If so, I think #12 is the best way to convey this information (again, plus or minus the namespace information, assuming the documentation location is in a used namespace for the current file).

c) If the answer to either (a) or (b) is Yes, then which one is more important? And is that benefit outweighed by the maintenance cost to doing it (given that any code naming refactoring probably requires some due diligence with grep to make sure @see references and the like are fixed)?

cross-post...

Regarding #16 though, I think we would be better off with //inheritdoc foo(), which at least is saying "this documentation is elsewhere", rather than always "Implements", which loses information.

Note that overriden methods are not always part of an interface

That is also a good point (#19), in regards to the points in #16.

Also in regards to maintainability: if a previously overridden method is removed from the base class, and it didn't come from an interface, it would need to have a real docs header added to it no matter how it was previously documented...

So I still think that

// inheritdoc \Where\It\Is\Documented::method()

covers all the cases the best. If that method is removed or renamed, you'll have to grep code for references, so you'll find this one and either fix it to point to the new location, or put the removed docs there because it's a method not inheriting docs any more. Doesn't really sound like too much of a maintenance burden...

Wait, what? Where exactly are you suggesting to put // comments? Do you mean:

class Foo extends Bar {
  // inheritdoc: Bar::thing()
  public function thing() {}
}

Because that makes no sense to me whatsoever. The only benefit seems to be "then we don't have to parse it", but since we're using our own proprietary parser we can parse or not parse whatever the heck we want.

Yeah, pretty much, except it would say @inheritdoc (@ sign is missing).

And yes I can make the API module parse whatever we want it to. The point is that if we do it that way it already is doing the right thing and I don't have to change it. I don't see much value of

class Foo extends Bar {
  /**
   * {@inheritdoc Bar::thing()}
   */
   public function thing() {}

over #21, do you? It seems like this is extra typing vs. #21 with zero benefit, and plus I'd have to make a change to the API module.

Anyway... I'm offline until next Tuesday, so please do not adopt a new standard until then. :)

Enjoy your vacation (hopefully)!

1) All functions/methods/classes/everything needs a docblock, period, unconditionally. We've been trying to drill that into people's heads for years. Adding a "oh, unless it's a method implementing an interface and then you use an unparsable comment instead that even your IDE won't be able to cope with" exception to that does not help with education or consistency.

2) I don't know off hand which IDEs understand {@inheritdoc}, but I do know that the number of IDEs that may support it is a positive number since it's part of phpDoc. The number of IDEs that understand // @inheritdoc I can guarantee is 0. As a heavy IDE user, a standard that cripples my IDE is not something I can support.

3) I freely admit that I'm not putting much weight into the implementation cost for api.module, in part because I'm focused on the DX for other users. I do feel that if api.module gets too complex or time consuming to maintain, it's probably time for us to switch to a standard doc parser. That's probably a topic for another thread, however...

RE #24 item 3: If we switch to a standard documentation parser, we lose all the functionality of api.drupal.org (like, where is this hook invoked and other drupalisms, and the new Views-based listing pages with filters, among other things). Besides that many of our current doc comments will not parse correctly.

Point taken on #24 item 1.

Anyway, it's not a huge deal to add features to API module, but if we could avoid having to do {} on this one tag (since we don't on many many other tags that PHPDoc et al use {} on), that would be better as far as keeping the code for the API module clean. And I think it would be better for internal consistency of our Drupal API doc commentn standards if we didn't have one tag that needed {} around it, given that we have zero other tags that do.

So, would it be possible for some of the IDE users to check whether their IDE understands @inheritdoc with and without the {}? And to tell us what "understands" means for their IDE -- what does the IDE do with the information?

Using NetBeans 7.3 (recently released) and the following sample code:

class Action {
  public function go() { }
}

class Foo {
  /**
   * Be useful.
   *
   * @param string $type
   *   How to behave.
   * @return Action
   */
  public function behave($type) {
    return new Action();
  }
}

class Bar extends Foo {
  public function behave($type) {
    return parent::behave($type);
  }
}

$b = new Bar();

// Typing this line...
$b->behave($type)->go();

With no docblock on Bar::behave():

- $b-> [IDE shows behave(), including full doc in popup]
- $b->behave('well')-> [IDE shows go()]

With {@inheritdoc}:

- $b-> [IDE shows popup with {@inheritdoc} as a literal]
- $b->behave('well')-> [IDE shows go()]

With @inheritdoc:

- $b-> [IDE shows popup with "PHPDoc not found"]
- $b->behave('well')-> [IDE shows go()]

So it appears to handle {@inheritdoc} slightly less bad, although neither one is particularly useful. The only one that does work in the IDE is leaving out the docblock entirely... which is unfortunate.

(The point still remains that the odds of NetBeans supporting {@inheritdoc} in the future are, IMO, higher than @inheritdoc, although it's disappointing that it still doesn't handle either one.)

Mark makes a very good point. Right now, we have tons of {@inheritdoc} in the repository. Even if we manage to get those projects back out (which I'd prefer), people using Drupal will still have it them in their local dev environment. As we continue to encourage people to work "upstream", they'll be using {@inheritdoc} there. Why introduce an inconsistency for them?

Yes, please.

so, i have a take on this that may or may not help things, but...first, from #17:

Regarding maintainability... All docs are hard to maintain. When we change parameter names, we have to fix docs. When we change function names or class names, or get rid of them, @see references have to be fixed. If we remove a method, ideally we should grep in the code for references to it and make sure they're fixed. I don't see why this is a huge deal or adds more burden than what should already be checked anyway.

the difference between @see references, etc. vs. @inheritdoc is specifically the issue that some of the earlier comments were dealing with - where in the inheritance chain was a docblock most recently defined? by choosing to place an @inheritdoc in a docblock method, i'm making an indication that i am essentially following in the general purpose of the method that was defined by the parent classes/interface, and so it is the class hierarchy that matters when it comes to determining what that documentation should be. explicitly indicating what i'm inheriting docs should not happen, as that information is already implicit in the inheritance chain.

also, i kinda like the idea of slicing stuff in selectively. i don't know how it works elsewhere, so i'm entirely blue-sky-ing (don't shoot me...too much :P), but what makes sense to me is:

class FlowerPower {
  public function go() {}
}

interface AnInterface {
  /** 
   * Do cool stuff.
   *
   * We like cool stuff!
   *
   * @param string $arg
   *   Foundation for our cool stuff!
   * @return FlowerPower
   */
  public function coolStuff($arg);
}

class Foo implements AnInterface {
  /** 
   * {@inheritdoc}
   */
  public function coolStuff($arg) {
    return new FlowerPower();
  }
}

class Bar extends Foo {
  /** 
   * {@inheritdoc}
   *
   * BarBar, BarBar!
   */
  public function coolStuff($arg) {
    return parent::coolStuff($arg);
  }
}

class Baz extends Bar {
  /** 
   * {@inheritdoc}
   *
   * Baz: makes mouths happy.
   *
   * @param string $arg
   *   Coolness gets cooler by the day.
   * @return FlowerPower
   */
  public function coolStuff($arg) {
    return parent::coolStuff($arg);
  }
}

given that chain, i'd ideally like to see the docblock on Baz::coolStuff represented as follows:

/**
 * Do cool stuff.
 *
 * We like cool stuff!
 *
 * BarBar, BarBar!
 *
 * Baz: makes mouths happy.
 *
 * @param string $arg
 *   Coolness gets cooler by the day.
 * @return FlowerPower
 */

which implies that @inheritdoc will inherit ALL docs by default - first line of docblock, long description in docblock, and param/return info. things further down the chain can ADD to the long description, or OVERRIDE the param/return info, and it all gets stitched together.

obviously we can't rely on IDEs to do that, but i imagine this might be somewhat close to the concept of inheriting the long desc that's used elsewhere?

RE #31 - That is definitely pie in the sky and not the semantics of how PHPDoc et al use {@inheritdoc}. It is a placement thing -- for instance you can use @param $foo {@inheritdoc} to inherit the docs for that parameter, and {@inheritdoc} as the main body to inherit the main body docs.

OK... Anyway given all the above arguments, I think for the Drupal project and API module we should support

/**
 * {@inheritdoc}
 */

only, and have it mean the same as leaving the doc block off currently does in both IDEs and the API module (the first parent up the chain with docs for that method/property defines the docs).

I'm not excited about this but am marginally OK with it. I definitely do not want to do anything more complicated with @inheritdoc in the API module, and I see the value of requiring the {} for IDEs.

I could definitely see value in allowing stuff like :

/**
 * {@inheritdoc}
 *
 * This is overriden because of this and that.
 */
public function coolStuff($arg) {

Also, there is the case of methods overriding a parent method (or implementing an interface) *and* adding a parameter (PHP allows that provided it comes with a default value).
We need to document this additional param somewhere ?

@jhodgdon - ahh, i see - @param $foo {@inheritdoc} clarifies the way others use it. more granular, and more complex than i had in mind. it's nice that @yched sees the basic value of being able to tag-on to the long desc - really, that's more important than the overriding of the param stuff. just providing a bit of extra docs, particularly to explain why a method is being overridden.

anyway, good that we can agree on {@inheritdoc}. i'm not clear on why it would only go up a single parent, though - would that mean that a concrete class extending an abstract base class that implements an interface would be unable to inherit from the interface? or does it work, but iff the abstract base has also marked @inheritdoc on the method? that particular pattern is, i think, fairly common. again, it seems best to me that we would traverse the class/interface hierarchy until we find a docblock, then use that.

i, for one, appreciate your willingness to go along with this despite your reservations and the work it creates for the api module, @jhodgdon. this has been really irking me since i started working on D8, it brings us closer to wider standards and, IMO, is more in line with the fluidity-of-logic-to-be-used that is pretty inherent to OO programming, anyway.

RE #34 - the way it works currently in the API module if you leave out docs completely, and I think also in PHPDoc, is that it would go up the hierarchy of class/interface inheritance until it finds documentation that exists for a method, and use that method's docs.

I really don't want to have it both ways though. Either you inherit the docs, or you don't. If you're not inheriting, you could do something like:

/**
 * Overrides BaseClass::methodName() in order to foo in a different way.
 *
 * This method is very similar to BaseClass::methodName(), but after it does foo,
 * it adds bar, baz, and something else.
 */

This would provide a link to the base class method, not be too hard to maintain I hope, and provide explanation. I would suggest NOT using the full namespace here on the class name in order to save space. The base class would need to be in a use statement or in the same namespace anyway, and API module at least could figure this out.

And then you could provide param docs if they were different from the base class (in which case do all of them, not just the new one).

+1 to #32/#35, and punting on anything more complex until Later(tm).

Fine with me too. Thanks @jhodgdon.

So... can someone update the issue summary with:
- What we'd need to change in http://drupal.org/node/1354
- What we'd need to patch
and then mark this issue RTBC? (We do that on standards issues and let them sit for a few days to make sure anyone who follows RTBC standards issues has some time to comment.) Or actually this one hasn't even been marked "needs review", so making a concrete proposal and marking "needs review" would probably be the first step.

I am really really really busy right now so I wouldn't be able to get to this for a while, sorry!

Summary updated.

Thanks! I think everyone who has been involved here so far is +1 on the current proposal (or at least willing to live with it). And I also think we've hashed out the possibilities and their technical merits/debts/etc., and this is probably the best we can do (for now at least).

So I am going to (contrary to what I said earlier today) give this a preliminary RTBC. Let's let it sit for several days to see if the wider "coding standards interested" community or "RTBC interested" communities have any dissenting opinions.

Add planned next steps and a proposal.

Made a very slight tweak to switch the capitalization to {@inheritDoc}, since that's what Symfony seems to be using. It shouldn't impact anything else.

Fix capitalization.

Hm. I sit corrected then. We should add that to the list of things to do to the Symfony documentation at some point. :-)

Assigning to jhodgdon, not necessarily that she has to do this, but just to reflect that this isn't something I'm dealing with in my current RTBC queue cull...

So far there's been no dissension, and a couple of people are starting to use this already in patches. I think we can/should move ahead here and update the coding standard docs.

OK. I've added this to:
http://drupal.org/coding-standards/docs#classes
http://drupal.org/coding-standards/docs#inheritdoc

Let me know if you think the changes are OK:
http://drupal.org/node/1354/revisions/view/2613884/2635576

So... Do we want to try to make a grand "clean up this mess" patch that replaces the existing Overrides... docblocks? And if so, on this issue or a separate issue?

I also filed this issue on the API module to support this tag:
#1962592: Support {@inheritdoc}

It seems like we're doing the patches as they come rather than starting a major "cleanup" effort. So, marking this fixed. If someone wants to start a clean-up meta-issue, file another issue and link here. Thanks!

just checking..
So I will know where to look for what it's implementing or overriding by looking at the extends [edit:] or implements on the class?

Yeah, that's the idea. On api.drupal.org, the page will tell you what it overrides or inherits. Most IDEs should too (I guess? I don't use an IDE).

followup #1994890: Allow {@inheritdoc} and additional documentation

Testing standards doc:
https://drupal.org/node/325974
needs to be updated to remove the
"There is no PHPDoc on this function since it is an inherited method."
from both the setUp and getInfo and update the examples there to use {@inheritdoc}

Here is an issue for it:

#2007766: {@inheritdoc} in tests on setUp and getInfo

There was a bit of the obsolete "Overrides" standard at https://drupal.org/coding-standards/docs#drupal:

When overriding a base class method, implementing a hook, or implementing an interface method, use a short form summary, such as "Overrides BaseClassName::methodName()." or "Implements hook_menu().". Omit the parameter and return value documentation in this case.

I've rewritten it as:

When implementing a hook, use a short form summary of the form "Implements hook_menu().". Omit the parameter and return value documentation in this case.

Thanks for taking care of that, mikeryan!

Unfix all capitalization.

Quick question to the people involved with making this change: How do we actually process it?

I.e. how does one establish (automatically) the correct source for any given instance?

Let's take core/modules/user/src/Entity/Role.php and Role::preSave() as an example.

namespace Drupal\user\Entity;

use Drupal\Core\Config\Entity\ConfigEntityBase;
use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\user\RoleInterface;
...
class Role extends ConfigEntityBase implements RoleInterface {
  ...
  /**
   * {@inheritdoc}
   */
  public function preSave(EntityStorageInterface $storage) {
    ...
  }
  ...
}

What's a command I can run, and the necessary arguments, to produce the documentation for that method?

Anyone?