[policy] Remove DI verbosity by removing unnecessary doc lines

It is great to have a separate discussion.

Should we maybe keep the typehinting of the properties as other static tools might not be able to figure out the type from the constructor.

Proposal: combine declaration of DI property variables, __construct(), and create() into a single section of code, with only a single, minimal, PHPDoc for the entire combination.

One general question: what do we do with stuff like plugins which do have dependencies but also have just plain old arguments in the constructor as well.

Hm. Not sure about this. It seems to just be slapping lipstick on a pig in terms of reducing verbosity, and the parts of the verbosity it *does* remove are actually really useful pointers to people who aren't familiar with D8 code and trying to figure out WTF is going on.

Also, while many times these variable names will map 1:1 with the class brought in via use statement, this isn't always the case:

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.php
@@ -17,22 +17,12 @@
   /**
-   * @var \Drupal\ban\BanIpManager
+   * Construction.
    */
   protected $ipManager;

Case in point. Replacing a pointer to the right place to read up on where this comes from (which is helpful) with a boilerplate "Construction." (which is just noise) doesn't feel like an improvement to me, personally.

Not all IDEs derive property type information from the constructor, either. NetBeans doesn't, at least in 7.4.

Maybe as a first step and as a sort of compromise we can agree on removing the required *descriptions* from @var and constructor docs at least?!

I.e.:

// Currently
/**
 * The module handler.
 *
 * @var \Drupal\Core\Extension\ModuleHandlerInterface
 */
  protected $moduleHandler;

// Proposal
/**
 * @var \Drupal\Core\Extension\ModuleHandlerInterface
 */
  protected $moduleHandler;

// Currently
/**
 * Constructs a Foo.
 *
 * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
 *   The module handler.
 */
  public function __construct(ModuleHandlerInterface $module_handler) { ... }

// Proposal

/**
 * Constructs a Foo.
 *
 * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
 */
  public function __construct(ModuleHandlerInterface $module_handler) { ... }

I think that would be a good first step and then we can decide in a follow-up if we want to go any further.

Those one-line descriptions of services rarely, if ever, convey any useful information.

I would be sad if the @var were removed from the properties. That's very useful to me, and to PHPStorm.

Agreed about removing the one-line docs. I'd even be okay with removing the @param from constructors, since they should just be assigning the params to properties in most cases.

I don't like the idea of removing the one-line docs.

A few reasons:

a) On a Class page on api.drupal.org, such as:
https://api.drupal.org/api/drupal/8/search/PluginBase
the one-line description is what appears in that right column. @var doesn't.

b) In some cases the @var doesn't really tell you much anyway (like @var string or @var int), so you need a one-line description in those cases to explain what the member variable is for. I would expect that in some cases, even @var lines with classes/interfaces in them may need more explanation and would call for a one-line description too?

c) Clear standards are easier to understand than standards with a lot of exceptions. So we could have a standard like "If a class member variable's type is a class or interface with a really clear name and you have an @var line with that name in it, and you don't have any other information to provide, you can omit the one-line description", but a standard like "Everything needs a one-line description" is a lot clearer and easier to follow/understand, at the expense of an extra line of documentation.

b) In some cases the @var doesn't really tell you much anyway (like @var string or @var int), so you need a one-line description in those cases to explain what the member variable is for. I would expect that in some cases, even @var lines with classes/interfaces in them may need more explanation and would call for a one-line description too?

I do agree that non-classes need more context, but soem services really should not be tried to be documented all over the place.

Let's just show what I think would be kinda useful:

// Currently

  /**
   * The entity manager.
   *
   * @var \Drupal\Core\Entity\EntityManagerInterface
   */
  protected $entityManager;

  /**
   * The ID of a node.
   *
   * @var int
   */
   protected $nodeNid;

  /**
   * Constructs a EntityCreateAccessCheck object.
   *
   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   *   The entity manager.
   * @param int $node_nid
   *   The ID of a node.
   */
  public function __construct(EntityManagerInterface $entity_manager, $node_nid = 1) {
    $this->entityManager = $entity_manager;
  }

// Proposal
 /**
   * @var \Drupal\Core\Entity\EntityManagerInterface
   */
  protected $entityManager;

  /**
   * The ID of a node.
   *
   * @var int
   */
   protected $nodeNid;

  /**
   * Constructor.
   */
  public function __construct(EntityManagerInterface $entity_manager, $node_nid = 1) {
    $this->entityManager = $entity_manager;
    $this->nodeNid = $node_nid;
  }

Whatever the community decides to do is probably fine. But if you want to change the docs standards, I challenge you to make specific edit suggestions for node/1354 that result in coherent, easily-understood, easily-followed standards.

We should probably go through that whole page and simplify it... it tends to get bloated and I usually feel like I'm the only one who knows what the standards are... separate issue though.

RE #13 - do IDEs actually go off the @param or the types that are also in the function signature? Can you test? (I don't use an IDE.)

At least phpstorm can do that. What are other important IDEs these days?

$node_id might not have a type hint, but it does have a good variable/parameter name at least. ;)

I really don't think leaving the params off a constructor in most of our classes is a problem. And in the rare case where it *is* a problem, there is no reason more documentation cannot be added to clarify what is going on, right?

So...

It seems like the one-line doc remaining on __construct() is completely pointless. Maybe it is time to just let that go. It's not like anyone who understands OO PHP would really have any doubt about what the __construct() method is for, and a docs line like

  /**
    * Constructs a Drupal\Component\Plugin\PluginBase object.
    */

really provides zero new information.

Agreed.

Hm. I don't really get that, though. We don't do that in e.g. SimpleTest tests to separate whatever variables it uses to track test state from the getInfo() method, which also was deemed too unimportant to document properly. And unlike getInfo(), __construct() is a standard PHP 5.x language construct, so arguably more people would be able to pick it out easily.

It seems like the one-line doc remaining on __construct() is completely pointless. Maybe it is time to just let that go. It's not like anyone who understands OO PHP would really have any doubt about what the __construct() method is for, and a docs line like

See #2140961: Allow constructor methods to omit docblocks.

Hm. In general, I agree with the cases in which classed type-hints are involved, because that is really unnecessarily duplicated information.

However, in case of strings, integers, booleans, and also arrays, removals like the following are not really represented anywhere else and not necessarily obvious:

-   * @param array $configuration
-   *   A configuration array containing information about the plugin instance.
-   * @param string $plugin_id
-   *   The plugin_id for the plugin instance.
-   * @param array $plugin_definition
-   *   The plugin implementation definition.

In case the arguments are stored as class properties, I assume you're making the point that the class properties will hold the docs already. — However, it is not guaranteed that each class property will hold the identical value that was passed into the constructor. Sometimes the passed in values are preprocessed, before they are assigned to properties, so the injected value is not necessarily the same value that is documented in the class property.

In addition, we have a couple of cases like the following throughout core:

  public function __construct(ConfigStorageInterface $install_storage, ConfigStorageInterface $schema_storage, ConfigStorageInterface $active_storage)

Without explanations in phpDoc and just this constructor, you will have to study the full class implementation in order to figure out which dependency is used in what exact way, and thus, what you need to inject.

These are the cases I wanted to bring to attention. Ultimately, I agree with the proposal though. I guess what I'm trying to say is that we should apply common sense when executing on this proposal; i.e., if the classed type-hints are truly obvious, then there's no point in writing duplicate phpDoc. But if there are primitives or arrays, then those should be reflected in phpDoc.

Yeah, people should try to turn on their brains. The documentation should be concentrated on the bits which actually matter to understand the class.

We should invoke the people working on PSR-5 here, both to make sure they're aware of the issue and to see if there's another solution we can borrow from the draft.

Hello everybody, my name is Mike van Riel and I am lead developer for phpDocumentor. Crell has asked my to join this thread to provide some of my insights on the changes that you are discussing here. As well as lead developer for phpDocumentor I am editor for the PSR-5 recommendation that describes the PHPDoc Standard (in excruciating and sometimes painful detail ;)).

Before I provide my view on this topic, let me start of with saying that I am a documentation evangelist and biased towards the (proper) use of DocBlocks. Knowing this will hopefully provide you with a little background where I am coming from.

I cannot recommend omitting the @param tags for any method; having them consistently helps with skimming your class and evaluating which method suits best in your use case. In addition a DocBlock, and especially @param, provides you with more information than a typehint or assumption may offer you.

As an example:

class MySpecializedDateTime extends MyDateTime
{
    public function __construct($date)
    {
        $this->date = $date;
    }
}

The first person to tell me what you should pass to this constructor gets a cookie.

In this case we can read that a date should be passed but what does this date represent? Is it a starting date? date of occurance? birthdate? But also: it is a string notation, if so: in what format? Is it perhaps an integer? It might be a unix timestamp? It could also be a datetime object where someone omitted the typehint.

Writing a DocBlock forces you to think about these things and write down what is necessary. But perhaps you might even be flexible and allow both a string, integer, DateTime object and null? This you can only describe in a DocBlock. In this case I cannot even deduce the type from a property since that is declared in the parent class as a protected property.

Another advantage is that when writing code in an IDE, such as PHPStorm, you can often call up the details using CTRL+Q without visiting that class. When you properly DocBlock your code, including summary, you can continue coding without visiting every method that you happen to invoke.

The class and constructor DocBlocks are also the perfect places to describe how to use the class and what people can expect from this class and it can do that in 1 to 6 lines of comment. Compare that to reading a 200 lines long class, which is verbose now?

I can talk hours on this (actually, I give talks on conferences on the use of DocBlocks and how to properly write documentation) but my recommendation is: don't remove documentation because it is daunting, rewrite your code to make it less daunting. Because honestly: documentation helps and does not add to the complexity of your code, it helps to reduce it.

RE #30, I share your passion for docs. But the example cited is not relevant here -- we are only saying we could get rid of the @param for a constructor ***when the argument is a typed class, is set directly into a class member, and the class member is documented***. In this case, the constructor function is very short and the @param doesn't really add any additional information, and the Drupal project coders are always looking to reduce the horrible burden of documentation....

RE #31:
I could perhaps pick a more relevant example, because there are tons, but I find myself disagreeing with your assertions based on the information in this discussion. Please allow me to elaborate.

You said the following:

is set directly into a class member

However the text in #24 does not mention that it must be a direct class member.

This, however, is merely semantics and the real 'thing' lies in this piece of text:

and the @param doesn't really add any additional information

I had hoped to demonstrate that the `@param` provides for a lot of value in this instance but lets zoom in again for a moment.

Compare the following

class MySpecializedDateTime extends MyDateTime
{
    public function __construct($date)
    {
        // ... code that detects whether this is a string or integer and normalizes the $date variable to a DateTime object
        $this->date = $this->normalizeDate($date);
    }
}

to

class MySpecializedDateTime extends MyDateTime
{
    /**
     * Initializes this object with the provided date.
     *
     * @param string|int $date either a string in the format YYYY-MM-DD or a Unix Timestamp.
     */
    public function __construct($date)
    {
        // ... code that detects whether this is a string or integer and normalizes the $date variable to a DateTime object
        $this->date = $this->normalizeDate($date);
    }
}

The above example demonstrates that the @param string provides a clear definition on what input is expected and in what format that input should be. Without the @param you would have to read the contents of constructor AND at least the normalizeDate method to determine what you should provide to the constructor.

This example also shows a difference between constructor arguments and what is in the property.

Actually, #24 says:

Constructors whose every parameter is directly assigned to a documented class property may omit the @param tags, potentially resulting in no DocComment at all.

So the proposal here is only to eliminate @param in the case where all the parameters go directly into documented class properties, and then if that is all that is happening in the constructor, to eliminate the doc block completely as being redundant (since any PHP programmer presumably knows what __construct() does in general).

Most of the time I'd agree with Mike: Good documentation forces good design, and what you think is superfluous now is what will save you 3 hours of "WTF was I thinking?" 6 months from now. I'm on record in numerous public presentations and patch reviews saying that good API docs are never optional.

That said, here's a code sample from a recent Drupal 8 demo module I wrote:

use Guzzle\Http\Client;
use Drupal\Core\KeyValueStore\StateInterface;
use Drupal\Core\Config\ConfigFactoryInterface;

class Weather {

  /**
   * The Guzzle HTTP client.
   *
   * @var \Guzzle\Http\Client
   */
  protected $guzzle;

  /**
   * The state API store.
   *
   * @var \Drupal\Core\KeyValueStore\StateInterface
   */
  protected $state;

  /**
   * The base URl from which to request data.
   *
   * @var string
   */
  protected $url;

  /**
   * The config object for the weather subsystem.
   *
   * @var Config
   */
  protected $config;

  /**
   * Constructs a new Weather object.
   *
   * @param \Guzzle\Http\Client $guzzle
   *   The HTTP client.
   * @param \Drupal\Core\KeyValueStore\StateInterface $state
   *   The State API store.
   * @param string $url
   *   The base URL from which to request weather data.
   */
  public function __construct(Client $guzzle, StateInterface $state, ConfigFactoryInterface $config_factory, $url) {
    $this->guzzle = $guzzle;
    $this->state = $state;
    $this->config = $config_factory->get('emergency_block.weather');
    $this->url = $url;
  }
  // ...
}

That's 3 services and one string. Of them, two are painfully obvious: The Guzzle Client and the StateInterface services. The class name is listed in 4 places, and the description of "this is the X service" is in 2. (And, to keep IDEs handy, we're listing the full class name in 3 places, but with different prefixing rules, but the short class name in 1.) Even I have a hard time defending that as developer-friendly. That's the part we're trying to figure out how to shorten.

I think that same concern would be relevant for PSR-5. If we assume that IDEs will eventually support whatever PSR-5 tells them to (which I think is likely), what do we tell them to do?

Allow me to provide a more refactored version as a way of example:

use Guzzle\Http\Client;
use Drupal\Core\KeyValueStore\StateInterface;
use Drupal\Core\Config\ConfigFactoryInterface;

class Weather 
{
  /** @var Client */
  protected $guzzle;

  /** @var StateInterface The state API store. */
  protected $state;

  /** @var string Base URl with which to request data. */
  protected $url;

  /** @var Config The configuration for this object */
  protected $config;

  /**
   * Initializes this object with its dependencies and extracts the 
   * configuration from the factory.
   *
   * @param Client                 $guzzle 
   *     The HTTP client used to connect to Weather services.
   * @param StateInterface         $state  The State API store.
   * @param ConfigFactoryInterface $config_factory 
   *     The factory from which to retrieve the configuration for this object.
   * @param string                 $url    
   *     A URL from which to request weather data.
   */
  public function __construct(
      Client $guzzle, 
      StateInterface $state, 
      ConfigFactoryInterface $config_factory, 
      $url
  ) {
    $this->guzzle = $guzzle;
    $this->state  = $state;
    $this->config = $config_factory->get('emergency_block.weather');
    $this->url    = $url;
  }
  // ...
}

I have done the following:

1. Removed the name of the current class from the summary and description
2. Collapsed all namespaces in the DocBlocks as these are imported using `uses`
3. Collapsed the properties DocBlocks to their single line short hand (important note: the description is now part of the `@var` and no longer of the entire DocBlock. When a DocBlock is used to document a single property I consider this a valid use case and eve phpDocumentor assumes that the description of the @var _is_ the property's description when no DocBlock description is present)
4. Rewritten the Constructor Summary to represent what it does and not what it is.

The previous Summary of the DocBlock is actually a good example of a 'useless' type of DocBlock; it didn't tell me what the constructor did except for constructing the object. But constructing an object is such a generic term that is does not.

There were two other question marks I had with this example:

1. If the Guzzle client was created beforehand, and the base url is known, why not register the base url as part of the client inside the constructor or even before passing it as a dependency? You can eliminate a dependency and superfluous docs that way.
2. What does this StateInterface do? The DocBlock does not provide information what it does in this object and now I have to read the rest of the code to determine its function.

I hope my wording was not too harsh, I was in a bit of a rush to write this.

Kind regards,

Mike

#35 won't work -- @var does not support a description, in PSR-5, our own Docs standards, or any other documentation scheme I've seen. It's only used for providing the type of a variable.

Also, I personally find complete one-line /** (something) */ doc blocks really really hard to scan, and our API docs standards do not allow them.

@jhodgdon I can guarantee you that `@var` supports a description. I know because I wrote both the documentation for phpDocumentor2 and PSR-5.

phpDocumentor2 docs: https://github.com/phpDocumentor/phpDocumentor2/blob/develop/docs/refere...
PSR-5 (@type supersedes @var): https://github.com/php-fig/fig-standards/pull/169/files#diff-9a10a11696d...

Gracious, I am not awake yet! Sorry about that.

I still don't think we want one-line /** */ doc blocks, though. Any other opinions?

So that's what I proposed in #6 right? (Not trying to say "I told you so!", but rather want to be clear what we're talking about here.)

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

Having been working on a Symfony project recently, and PHPStorm's defaults, I would support the following amendment:

- When an object property is a service and its role is self-evident from the class type, the description on the property docblock may be omitted.

(That is, putting "The config factory service" on all 500 times we have a $configFactory property, with a type specified on it, is needlessly redundant. Stop doing that. But still document properties that aren't just a service reference.)

Edit: So yes, I am +1 on comment 6.

RE #42 - "when an object property is a service" ... You mean to say "When a class member variable type is a service"? And ... does this really only apply to services? Also, #6 implies it should be for both class member variables and @param statements for functions.

Well, I don't think all properties should skip a description, just those that are blatantly self-evident and repetitive. "It's holding a service" was my best first stab at a better definition than "it's obvious, duh".

OK. Well we currently have policies that:
a) Every doc block needs at least a one-line description.
b) Every @param needs a description.

It sounds like you want to amend (a) to say "Exception: class member variable doc blocks where @var is used to give a class data type, and the variable name and class make the description redundant".

And (b) to say "Exception: if the @param is followed by a class data type, and the variable name and class make the description redundant".

.... My opinion, which I have probably stated above, is that both of these amendments will lead to bikeshedding on where to draw the line on what is redundant. I also don't see how it's really saving people a lot of time to avoid adding those lines of documentation.

Not quite. I'm suggesting only to modify A, with an exception of "if the class member variable is an injected service and is self evident from the type." There is much less room for bikeshedding there.

Also, in my experience with Drupal 8 and a Symfony project recently typing redundant human readable text over and over again does save a lot of annoyance, which means we're more likely to get people to do the parts that matter. PHPStorm also does the type for you, which makes it to ensure it gets done.

I think we are totally overthinking this. I think it is perfectly clear - and non-bikesheddable - what is redundant and what is not.

Example that is redundant:

@param ModuleHandlerInterface $module_handler
  The module handler.

It is absolutely impossible to gather any information from the description that the typehint does not reveal.

Similar example that is not redundant:

@param ModuleHandlerInterface $module_handler
  The module handler that is used to invoke hook_foo().

(Sadly, the latter example is rarely found in core but that's not for this issue.)

I think a sentence such as

For typehinted parameters the one-line description can be omitted if the typehint makes it sufficiently clear what needs to be passed.

with the above example as further explanation would be totally adequate.

I also find the current standard to be a burden during development, by the way.

Most documentation seems like a burden to most developers, as far as I can tell. Documentation that doesn't tell us anything is obviously not useful; it would be better to encourage people to write documentation that actually tells us something, rather than trying to eliminate documentation, IMO.

jhodgdon: I'm having an "Only Nixon could go to China" moment here. :-) 99% of the time I'm 100% with you on that: Lack of documentation is a bug, take the time to write it or go home.

But in this case... really, I cannot think of anything to say about a service-property 98% of the time that isn't some variation on "the X service. Because we're going to use it. You know, for things." Describing what specifically we're going to be using it for in the class is excessive most of the time, and is leaking implementation details out into places they do not belong. Put another way, even as an someone who is rather OCD about writing good documentation I can think of no way to "encourage people to write documentation that actually tells us something" more than what is already known from the class name. As you say, "documentation that doesn't tell us anything is obviously not useful". So let's skip places where there is no useful documentation to write.

For non-service properties, I agree, keep the docs.

OK... I do see your point. I just don't like policies/standards with lots of exceptions.

Also, the issue summary and previous comments have also included dropping the description for the @param lines on the constructor. Are you dropping that request to change the standards?

And can someone please write a coherent issue summary (someone who wants this to happen), with specific recommendations for which standards on which documentation pages need to be updated, and exactly how they should be? I don't really want this to happen, so am not really going to put effort into making it happen. Setting to Needs Work until there is a definite proposal.

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

As @jhodgdon said:

…can someone please write a coherent issue summary (someone who wants this to happen), with specific recommendations for which standards on which documentation pages need to be updated, and exactly how they should be?