Allow constructor methods to omit docblocks

OK. What alternative would you suggest?

Are you saying you don't want a one-line description at all, or that you want to modify what it says?

Updating title and tags according to the actual proposal here, which is a coding standards change proposal.

Xano: if you want this to get proper consideration, perhaps you could make a real issue summary (using the template) outlining your reasoning, and then set this to "needs review"? Since you are the one who is passionate about it, you will need to be the one to push it forward. I am generally not in favor of adding more exceptions to our standards, which are already very complicated to understand.

To avoid exceptions in the coding standards while avoiding the inheritance problem, we could just use a generic "Constructs the object." summary for all constructors?

There is nothing preventing people from using #7.

I just worked in the sprint at DrupalCon on #1924818: Things that are not really factories should not be documented as factories and I raised my opinion there about not having constructor documentation at all, except of parameters descriptions unless the constructor actually includes a logic that needs documenting, which is not normally the case. I think that not having that one line comment helps at least in terms on not having to maintain unnecessary documentation, a practical example would be that the mentioned issue would not have been there in the first place.

I have just updated the issue summary here with the two basic proposals (adopting Xano's I think preferable wording for (b)). So basically the options are (this is in the summary now):

-----------
a) Do not require the one-line summary for magic methods.

b) Encourage people to use a generic one-line summary like "Constructs a new class instance." to avoid most of the problems.

Both ideas fix the problems with inheritance, when the class name changes, copy/paste problems, etc.

The advantage of (a) is that it doesn't require restating the obvious.

The advantages of (b) are code sniffers etc. will not flag the doc block as non-compliant, and it doesn't require us to have yet another exception in our already-extensive coding/documentation standards.
---------

I vote for (b). Any other thoughts?

As we have a proposal...

Coding standards changes are now part of the TWG

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

I'll sponsor this change.

I'll sponsor this change.

Proposal a) or b)?

I vote for b) myself.
We should then maybe also provide pre-defined first lines for other magic methods in some central place.

a)

The Technical Working Group reviewed this issue and before this can be tagged for final discussion we feel that we should attempt to come to a specific recommendation to propose to the Drupal community as a whole. If we can’t come to a consensus the TWG is happy to mediate but we feel that this issue's participants should try to come to more of a quorum than a 2/1 split.

As additional data points, the Zend framework appears to go with proposal B with respect to the __construct() method and Symfony and PHPUnit appear to go with proposal A with respect to the __sleep() method.

a)

Let's try to be a bit less redundant in our standards. The first line adds 0 value.

I vote for a) as well.

The only concern I have with (a) is that it looks visually incomplete if some methods don't have a docblock. (-> scannability)
Also, this makes it harder to spot methods where the docblock was forgotten by the developer.

If a magic method already has a docblock with @param and/or @return, we don't need additional human-language description. Unless there is something relevant to say.

If the docblock would otherwise be empty, I would like to see something in there that justifies the docblock's existence, but makes it immediately obvious to readers that it does not contain any relevant information.

/**
 * -
 */
function __wakeup() {..}

But seeing that this is really awkward, I think I could live with having no docblock at all in these cases.

@drunken monkey (#16)

I vote for b) myself.
We should then maybe also provide pre-defined first lines for other magic methods in some central place.

The problem with (b) is that we really would have to come up with pre-defined text for all the magic methods, which developers need to look up, and reviewers need to verify. Sounds like yet another waste of time.

@tizzo (#18):

As additional data points, the Zend framework appears to go with proposal B with respect to the __construct() method and Symfony and PHPUnit appear to go with proposal A with respect to the __sleep() method.

Interesting! So we could have one predefined first line for __construct(), but not do anything for the other methods.
I am not really convinced this is a good idea, but it is interesting.
Btw, __sleep() actually has a return type (array). But __wakeup() has neither parameter nor return value, so it is a better example.

Totally agree with this. Would be great if someone could explain why someone would need the "Constructs the object." comment in the first place. I'm also addressing this more broader in this issue.

The problem with (b) is that we really would have to come up with pre-defined text for all the magic methods, which developers need to look up, and reviewers need to verify. Sounds like yet another waste of time.

Coming up with the pre-defined text doesn't seem like that big a deal. I'll do that in ten minutes, if that's the issue, and we can then just iterate if things aren't clear enough.
Anyways: The proposal says nothing about forcing people to use this, it says "encourage": i.e., we provide these default doc blocks for the developers' comfort, so that they don't have to think about them, but it's nothing a reviewer is expected to check. If someone wants to provide their own doc block for a magic method (or doesn't know about the pre-made ones) that's totally fine, too. (As I see the proposal – I might be mistaken.) We just want to make it as easy as possible to follow the standard of documenting even those magic methods. (And encourage "Constructs a new class instance." instead of something that mentions the actual class name and would be inaccurate when inherited in a child class. Unless I'm mistaken the docs actually suggested such a wording ("Constructs a CLASS object."), but I can't find it anymore, so either I'm mistaken or it got removed already.)

@ #22: It's explained multiple times here already: less exceptions are generally better, it looks "cleaner" if it's not allowed to skip the doc block for some methods (everything else has them) and easier to spot methods that "illegally" miss the doc block, plus some developers might actually not be 100% familiar with all magic methods.

@ #23 What is the advantage of having standard dock block descriptions if they don't add anything useful like "Constructs a CLASS object."? Any developer should be able to see this because the name of the method is "__construct". I don't see a reason to provide all these useless comments at all. This can easily be solved by simply not requiring a description for every method and there are no exceptions anymore too.

+1 on a), would be DRY.

Go look at what Symfony does as of late.

+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 #3278431: Use PHP 8 constructor property promotion for existing code 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.

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 probably changed to say that documentation comments should be 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 naming it .diff instead of .patch 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.

 
diff --git a/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
index 54c08284f4..fb91a394bd 100644
--- a/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
+++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
@@ -246,9 +246,6 @@ public static function loadByName($entity_type_id, $bundle, $field_name) {
     return \Drupal::entityTypeManager()->getStorage('base_field_override')->load($entity_type_id . '.' . $bundle . '.' . $field_name);
   }
 
-  /**
-   * 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 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 #3278431: Use PHP 8 constructor property promotion for existing code 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 #3278431: Use PHP 8 constructor property promotion for existing code 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.

Opened #3400560: Allow _construct() method to omit docblock.

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