Allow constructor methods to omit docblocks

+1 for a)... please!

💯 🙏

Why is this limited to constructors and magic methods? Most of useless comments belong to getters and setters.

There is one thing that may stop developers from omitting docblocks. Consistency. A class may look weird if some of its methods are documented while others are not. That's why I typically put dummy doc like "The object constructor" to methods.

Wondering, if it's a good idea to put something like {@selfdoc} there for cases when you want a docblock but don't have any useful documentation to put in it.

Why is this limited to constructors and magic methods? Most of useless comments belong to getters and setters.

Constructors specifically makes adopting 📌 Use PHP 8 constructor property promotion for existing code Needs work in existing code harder than it should be. Adding getters and setters here makes this a bit more complicated - not every method with get or set in it is a getter or setter, and not every getter or setter has the word get or set in it.

Given constructor property promotion though, I think we should widen this a bit to include elements of #2209735: [policy] Remove DI verbosity by removing unnecessary doc lines → .

Updating issue summary with a proposed resolution - based on option a, and moving back to needs review for a bit.

phpcs already allows this:

./core/scripts/dev/commit-code-check.sh --cached
1/1 ./modules/node/src/NodeForm.php 362.55ms
CSpell: Files checked: 1, Issues found: 0 in 0 files

CSpell: passed

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.

                                                                                
 [OK] No errors                                                                 
                                                                                


PHPStan: passed

----------------------------------------------------------------------------------------------------
Checking core/modules/node/src/NodeForm.php

PHPCS: core/modules/node/src/NodeForm.php passed
core/modules/node/src/NodeForm.php passed

So we only need a documentation change here.

@catch
What if I want to add a docblock, but I want to omit the first line?
This would be relevant if one of the parameters needs further explanation.

The original proposal would allow me to omit just the first line, but with the new title it is not fully clear. One possible interpretation is that if a docblock is added, it must contain all the required elements, including the first line description.

https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... → doesn't explicitly say that every method/function needs documentation, it is more about standards for documentation for ones that do.

So... actually there is no coding standards change here at all.

I think the end result is we need to add 'with the exception of magic methods like __construct()' to https://www.drupal.org/about/core/policies/core-change-policies/core-gat... →

The documentation has been updated https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . I think we can safely mark this as fixed.

Every function, constant, class, interface, class member (function, property, constant), and file must be documented, even private class members.

Under:
https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

I couldn't find that sentence anywhere despite scanning that page three times.

I think we should have a draft change record here with the proposed change.

https://www.drupal.org/docs/develop/standards/php/api-documentation-and- → ... doesn't explicitly say that every method/function needs documentation, it is more about standards for documentation for ones that do.

As xjm wrote.
Also this:
https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

All classes and all of their methods (including private methods) must be documented.

Not much else is said about methods, especially the first line description requirement is not explicitly mentioned for methods. But one could assume that most of the rules for functions and for general docblocks also apply to methods.

I did not find a rule in the page that explicitly requires every doc comment to have a summary. Maybe I missed something. Still I think one can end up assuming that it is required.

The first paragraph of a docblock is known as the summary.

For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."

Created the change notice https://www.drupal.org/node/3365111 →

I agree with @donquixote but let's clarify also in https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... → as in the community there was the impression of summary being mandatory.

I've added "On PHP magic methods the summary is optional." at the end of bullet point next to the one cited by @xjm:

Changed from

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc.

To

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc. On PHP magic methods the summary is optional.

On PHP magic methods the summary is optional.

Still this does not explicitly say that it is required in other cases.
The reader can assume that it is required by default, if there is a rule that makes it optional under specific conditions. But we should not rely on that.

First we need to understand if it's only missing from documentation that is required or it was always optional. Probably we'll never find that. There's a deep belief in the community that the summary is mandatory. Is it mandatory or not?

I will stop here because I feel we're losing time with an issue aiming to reduce the time lost in other places. There are also other coding standards like this opened for years with common sense proposals. Just coding standards orthodoxy which slows productivity and creativity,

Updated the change record to match the new issue summary a bit closer.

If we're just saying the entire docblock for magic methods can be omitted, then whether the summary is optional or not doesn't affect this issue at all (fwiw I think if the function has documentation, the summary is required, the only exception to that would be {@inheritdoc} + extra docs, which unfortunately API module doesn't support, where the + extra would be an addendum to what's already there).

If we're just saying the entire docblock for magic methods can be omitted, then whether the summary is optional or not doesn't affect this issue at all (fwiw I think if the function has documentation, the summary is required

I would be ok with the change.

However, this does not fully solve the problem stated in the original issue:
There are still constructors where we do have interesting things to say about the parameters, so we do want a docblock.
And for these constructors, the first line still has the same problems as stated in the issue summary:

Constructors usually have a one-line description of the format Constructs a ... object.. Besides the fact that this is stating the obvious (it technically cannot really do anything else), it also breaks {@inheritdoc} for constructors of child classes, as the documentation of those constructors will display that of their parents, which include the parent class name. Whereas a child class is also an instance of its parent class, this is quite odd and possibly confusing.

The most natural and clean way would have been to open a separate issue where we make the entire doc comment optional.
But we can also leave this one in its modified version, and open a follow-up to either standardize the summary line, or make it optional.

I am working on announcement for this issue. As part of that I am updating the IS to have a section that clearly shows the proposed documentation changes. Currently, that has the doc pages that refer to 'methods'. There are two docs that state the requirement for a docblock for 'functions', show below for reference. Does anyone think a change is needed to these as well?

Drupal API documentation standards (general) →
Every function, constant, class, interface, class member (function, property, constant), and file must be documented, even private class members.

In-code API documentation is added or updated →
All functions, classes, files, constants, etc. that are added or updated must be documented. (minimum requirements)

I did a wee test. I removed the doc block for an _constructor, enabled the Drupal.Commenting.FunctionComment.Missing and Drupal.Commenting.DocComment.MissingShort and ran the commit code checks. There was no error on the changed file. This confirms what catch said earlier in #31.

Added proposed text changes to the IS.

According to the current process, step 6, this should be tagged final discussion.

Somehow my changes to the IS were lost. I had to revert to the previous version. Adding tag back.

I think it's worth updating the two places in #44 too. The IS changes look good!

Is it mandatory that magic methods do not have documentation comments? If magic methods must not have documentation comments, what quoted in comment #44 should be changed to say that documentation comments are removed from the magic methods that are updated.

@apaderno I think we're just making it optional here. Sometimes we might want to document what we're doing in a magic method at a high level, I guess we might want to keep docs for constructors of value objects too.

I do think we should go through and remove all the constructor documentation from services, controllers etc. in core, but that's likely to happen at the same time as introducing property promotion to existing code.

I guess I misunderstood what comment #44 said. (Please disregard my previous comment.)

+1 on this. Less useless docs is great

Although out of scope I'm a bit confused why private methods are explicitly mentioned.

There was no further discussion of this change in the latest coding standard meeting. #48 and #50 support the change. I am adding tags per step 8 of the CS process on the project page.

Actually now I am no longer so sure about this change.

I think our main motivation here is redundant docs on __construct().
But for other magic methods, are the docs really redundant?

Some things I want to know when I see a magic method:

• Why does this class need to be serializable?
• Why do we use __sleep() over __serialize()?
• Which properties are being omitted? (This could also be answered by inline comments)
• Why do we use __get() instead of an explicit method? How do I find out which properties are supported?
• Why do we have __call(), and which methods are supported?

A good exercise would be a MR that removes docs on a number of magic methods in core, to evaluate the impact.
Not something we would merge (or not here at least), but as a proof of concept.
Ideally with separate commits to see separate good examples from bad examples.

And I don't think we should look at symfony. They have far too few docs overall.

@donquixote we're only making the documentation optional, not forbidden, so we'll still be able to ask for docs on magic methods if there's some explanation to do (the same way that we ask for inline comments a lot).

@catch That's right.
But I don't understand how magic methods (besides constructors) are special, compared to other methods.
We should find at least some examples (in core) where a doc on a magic method could or should have been omitted.

The audience for these requirements is not only core development, but also the contrib ecosystem and custom code developed for corporations or institutions that like to comply with official standards, but are not so easily convinced by subjective code quality arguments.

The formal requirements often result in redundant comments, but half the time they actually result in useful explanations that would otherwise be missing.

But, if we can find a significant number of magic methods where we don't need the doc comment, then I change my mind :)

Btw the search regex is function __([^c]|c[^o]), to get magic methods that are not constructors.

I assume appending .txt evades the testbot.

I used a regex to remove all the docs on magic methods.
Now we should find at least some of them that are redundant.

-  /**
-   * Output the PoItem as a string.
-   */
   public function __toString() {
     return $this->formatItem();
   }

I think this is a good candidate - magic method that's a direct wrapper of another method, and it's always going to produce a string at the end.

+++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
@@ -246,9 +246,6 @@ public static function loadByName($entity_type_id, $bundle, $field_name) {
     // recalculated.

This is also a bit redundant, although you could argue it should be replaced with the inline comment at the top of the function body.

I think there are 3 cases:

1. Existing doc comment is redundant (= does not add information), and no doc comment is needed.
2. Existing doc comment is redundant, and it should be replaced with a better doc comment.
3. Existing doc comment is sufficient, it adds information that is not easily available from the context.

-  /**
-   * Output the PoItem as a string.
-   */
   public function __toString() {
     return $this->formatItem();
   }

I think this is a good candidate - magic method that's a direct wrapper of another method, and it's always going to produce a string at the end.

To me this seems like an example of case 2.
What I really want to know when I see this method:

• Why does it need to be stringable? What is the typical context when it would be converted to string? E.g. to be used in a template?
• What kind of string can we expect? Plain text? Html?
  Perhaps this is clear if we know what the PoItem class does.

-  /**
-   * Implements the magic __sleep() method.
-   */
   public function __sleep() {
     // Only serialize necessary properties, excluding those that can be
     // recalculated.

This is also a bit redundant, although you could argue it should be replaced with the inline comment at the top of the function body.

I would want to know:

• Why does it need to be serializable? When do we expect it to be serialized, and how functional does it need to be when it is unserialized?
• What happens to the excluded properties? Are they restored on wakeup?

Actually, the "why" or "when" question is more important on magic methods, because you cannot use "find usages" to see when it is called.

I recall dated debate about docblocks for overriden methods, specifically about rare need to add something after @inheritdoc - mostly why override required

catch and I discussed the points raised by @donquixote. The points raised are valid but it is enough to enforce the doc blocks, especially after this proposal has gone through community approval. We decided that we should change this to be specific to constructors only. While that is a change to this decision it is narrowing the scope. Discussion on expanding the scope can be done in a separate issue.

I have updated the issue summary to specify constructors.

Reducing scope if fine. +1

I am ok with the new scope, and the change proposed here.

The issue summary needs to be updated, some of the "Current text" and "Proposed text" parts look confusing.
(Can we use a diff format or is this not possible / practical?)

Also, can we give advice on when a constructor should have a doc comment?
E.g. a constructor doc comment can be omitted, if:

• The constructor is public. (If not, it should be explained why it is protected or private)
• All parameters have native class or interface type hints (if not, the param comments should explain the format that is expected)
• All relevant information is available from the context and the signature.

On the other hand: "If a doc comment is added, it must be complete according to the requirements on this page."
(see below, we should create follow-up to relax this)

----------

To honor the original request, we should open follow-up issue(s) to discuss some questions:

Given a constructor where _some_ parameters need a doc comment, can we relax the requirements for that doc comment?

• First line comment:
  • Can the first line be simplified or omitted? E.g. "Constructor." instead of "Constructs a ...."? This would prevent that the class name is copied around to the wrong place, or left unchanged after a class rename.
• Parameter docs:
  (See #1512338: Revisit Coding Standard about parameters documentation → or maybe other issues)
  • Can some parameters be omitted in the doc comment, or do we have to list all?
    (To me it would feel awkward to omit any, but I could be spoiled by Drupal)
  • If we list all parameters, can parameter types for some parameters be omitted in the doc comment?
  • Can we omit parameter descriptions, if they are obvious?

-----

catch and I discussed the points raised by @donquixote

I only now realized there is a dedicated channel for coding standards in slack :)

Also, can we give advice on when a constructor should have a doc comment?

I think this is good advice, but I'm not sure if we should put that level of detail on this page, it would be too complex for a coder rule for example. We could add it to the change record though?

On the follow-up suggestions:

Can the first line be simplified or omitted? E.g. "Constructor." instead of "Constructs a ...."? This would prevent that the class name is copied around to the wrong place, or left unchanged after a class rename.

This would be an improvement, we'd have to modify the 'starts with a verb' rule, but that is over-applied. We could also consider omitting it altogether since it still won't add anything, but that would look weird maybe.

All parameters have native class or interface type hints (if not, the param comments should explain the format that is expected)

I think #1512338: Revisit Coding Standard about parameters documentation → is probably the right place for that discussion if we wanted to apply it in general, but if not it'd probably need a new issue for constructors. Also I wonder if whether a single string/int/bool/array type hint that's not self-documenting might be better documented as a class property (if it is one) - that becomes optional already due to property promotion but it can still be explicitly defined.

@catch, thanks for following up.

I have updated the coding standard pages and confirmed the change using the 'compare' option in the history.

Per step 10, I have removed the tag and set the issue to "Fixed".

Thanks everyone!

formatting fix

I'm not sure this change to "only constructors can be omitted" is what we want. In 📌 Use PHP 8 constructor property promotion for existing code Needs work and the proposals related to it, we want the constructor to become the canonical documentation for promoted properties. The change from this issue is at odds with that.

Furthermore, I also don't see the required coding standards rule updates issues attached to this before the d.o documentation got updated.

we want the constructor to become the canonical documentation for promoted properties. The change from this issue is at odds with that.

I think the idea is that a lot of the dependencies don't need a documentation at all, neither as constructor parameters nor on the property.

Personally I would prefer something where we start with separate conditional requirements for different parts of a doc comment (param, firstline description, return, other), and then say "If none of the individual parts of a doc comment are required, the entire doc comment becomes optional.". Similar to what I proposed for properties in #3376518-16: Allow omitting doxygen when type and variable name is enough → .

@xjm much of the discussion on 📌 Use PHP 8 constructor property promotion for existing code Needs work is about dropping the constructor docblock, that is why we held up making the change on this issue, so that we could remove the constructor docs at the same time as applying constructor property promotion across core.

Automatically closed - issue fixed for 2 weeks with no activity.

Opened 📌 Allow _construct() method to omit docblock Needs review .

This issue was finished before we had the new 9 step Coding Standards process finalised.
Coder issue 📌 Allow _construct() method to omit docblock Needs review to modify the sniff is ready for review.

I created this follow-up: #3457897: Simplify first line in constructor doc comments to "Constructor." →

Some constructors won't have their doc comment removed, and they need to keep a first line comment.
For these, I would like to see a simpler first line comment.

What I propose is what I already do in my own code.
Maybe others have different ideas.