Allow omitting doxygen when type and variable name is enough

I agree, this would be a huge improvement. I wonder if we can extract the information from the type on the generation of the api pages (for example: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21AccountS...), so that we can still fill in some description there.

Yes, please. It's not only a core+contrib issue. Probably, most of the projects are just applying Drupal coding standards and they spend quite a lot of time to fix these kind of CS issues. When a var is correctly typed and named what's the point to add a description saying the same? It's only adding noise to code.

Why is this limited to class properties? Methods suffer from the same problem.

  /**
   * Gets the entity type manager.
   *
   * @return \Drupal\Core\Entity\EntityTypeManagerInterface
   *   The entity type manager.
   */
  public function getEntityTypeManager(): EntityTypeManagerInterface;

One thing that stopped me from doing this is inconsistency. Docblocks are used non only for the purpose of documentation but also for visual separations properties and methods. Consider the following example.

protected Foo $foo;

/**
 * Bar description.
 */
protected Bar $bar;

protected Baz $baz;

The code style looks a bit ugly. The vertical rhythm is broken which brings some visual dissonance.

On my custom projects I ended up adding {@selfdoc} tag to code that has no need for any documentation.

@Chi, this issue will probably already not be super easy to land, let's not try to increase scope here. Can you open a new issue for methods please?

@borisson_ I think that can wait till this issue is resolved.

I'm against this in its current form, because I think the text is slightly too vague and open for discussions. I feel that most discussion about the duplication of these docblocks is held by very experienced Drupal developers who are easily able to see the patterns and duplications. If I had to explain this rule to someone new to Drupal they would not be able to determine whether "that variable/property/method is not specific to the instance."

I also think that the current phrasing of the proposal is very hard to test with automatic tools. That means that either the rule will exist, but anyone using coder must type out a comment anyway; or coder must stop enforcing comments altogether which leads to worse documentation.

If people really want this then my proposal would be to scope this to "Docblocks may be omitted for class properties that hold references to Drupal services" since the documentation for those services is expected to be on the service implementation. I could conceive of a way of testing that with PHPStan, but I'm not sure if coder can be so smart.

My personal preference would be to focus more on adopting constructor promotion which i think makes the discussion in this issue moot and shifts it to "Should we document the constructor" (I'll reserve my answer for an issue focused on that).

/**
 * {@selfdoc}
 */
protected Bar $bar;

This is at least a hint that the developer has explicitly chosen not to provide documentation, as opposed to just 'forgetting' it, so it might help @Kingdutch's objection. I also like the idea of the API module trying to backfill these - just parsing out the classes that are referenced and linking to them would help maybe? It's always slightly annoying to me that the use statements get removed from the method declarations, so that when you view a method on its own you can't see what it's referencing - I don't use an IDE so I end up on api.drupal.org a fair amount still.

Both for automated tools and unambiguous wording:

Take the name of a property or function parameter. Convert it to CamelCase and append Interface after. If this is the type of the property or function parameter then the doxygen is optional.

This rule is narrower than the issue summary, however it covers most cases. I think adding selfdoc beats the readability purpose.

This seems hard, if not impossible to perfectly automate with a standard.

I'm in support of removing the requirement for doc blocks entirely for these vars (and other things but not for this issue) and leaving it up to the review process to determine whether things need extra documentation. This is just like we do for inline code comments.

Could you clarify why do you think this is hard to automate? One part is str_replace('_', '', ucwords($string, '_')) . 'Interface' and PhpParser certainly has the type of params. If necessary, I can lend a hand coding this for phpstan , probably would need a little mentoring mostly in how to fit it but II do not see any problems in the actual functionality.

What if we base this on whether a property has an object (class/interface) type hint?

AccountSwitcher::$accountStack

This is actually an array. So yes, I want it documented.

AccountSwitcher::$currentUser

This is actually an object, so I don't need it documented, _if_ instead we insert the proper native type declaration.

Interestingly, this is currently wrong:

  /**
   * The current user service.
   *
   * @var \Drupal\Core\Session\AccountProxyInterface
   */
  protected $currentUser = [];
[..]
  public function __construct(AccountProxyInterface $current_user, [..]) {
    $this->currentUser = $current_user;

There is no reason why this should be initialized as empty array.

/**
 * {@selfdoc}
 */
protected Bar $bar;

I like it, as a way to avoid the visual inconsistency.

What if we base this on whether a property has a (trivial) object (class/interface) type declaration?

Something like this:

• Native type declaration is strongly recommended for properties.
• Constructor property promotion is strongly recommended when possible.
• The readonly modifier is strongly recommended.
• Doc type for properties is optional, _unless_ one of the following:
  • Native type is missing or incomplete, e.g. for compatibility reasons or because limitations in native type system: Add a complete doc type, possibly with advanced syntax.
  • Native type contains array: Document array shape with advanced syntax. Optionally use @phpstan-var instead of @var (or is it phpstan-type?), if phpcs gets confused. In the latter case, @var becomes optional.
  • Native type contains callable: Document the signature and return with advanced syntax. Optionally use @phpstan-var instead of @var (or is it phpstan-type?), if phpcs gets confused. In the latter case, @var becomes optional.
• Doc description for properties is optional, _unless_ one of the following:
  • Property is not readonly, or not initialized in constructor: Explain how or when it changes. (TBD)
  • Type (doc or native) contains/allows...
    • NULL; Explain when.
    • union operator: Explain when one or the other type applies.
    • intersection operator: Explain why, or what it means.
    • array: Explain what the keys and values represent.
    • callable: Explain when it is called and what is expected of it.
    • string: Explain format and purpose of the string. E.g. if it is url, does it contain the port? Is it valid html, json, yml, something else?
    • integer or number: Explain what we are counting or measuring.
    • anything other than a class or interface name: ...
• If doc description and doc type are empty, then the entire doc comment can be omitted or replaced with @selfdoc (TBD).

> then the entire doc comment can be omitted

That's the entire point. There's no point if we are going through all this to end with another form of pointless clutter.

Could you clarify why do you think this is hard to automate?

Because I don't think your suggestion covers all bases, it'll still lead to having to document some things with useless doc blocks, no? You even said that it's narrower in scope than the IS.

Take the name of a property or function parameter. Convert it to CamelCase and append Interface after. If this is the type of the property or function parameter then the doxygen is optional.

my proposal does not cover all bases no but it is a step forward. Do not let perfect be an enemy of good. This would lead to a removal of a lot of pointless doxygen. There might be some odd cases like AccountProxyInterface $currentUser or ImageToolkitInterface $toolkit still having pointless doxygen but that's fine. We can tackle that in the next issue. This particular proposal is

1. Narrow.
2. Not ambiguous
3. Easy to code

so it should be easier to accept than a complete overhaul of our doxygen rules.

Easy to code

Maybe I misunderstand, but here is where I see the problem:

It can be easy to code to detect whether an existing doc comment is pointless. E.g. "Current user" for a $currentUser parameter.

However, we also need to detect whether a doc comment is required if it is currently missing.
E.g. if we have "AccountProxyInterface $currentUser" without doc comment, how can we detect whether a doc comment must be added?

Also, the first check cannot distinguish between a lazy doc comment that should be improved vs a redundant doc comment that can be dropped.

But maybe I completely misunderstand the proposal.

(and I want to refer to #16 again which _can_ be formalized)

Adding related that may be a duplicate, I haven't checked.

> E.g. if we have "AccountProxyInterface $currentUser" without doc comment, how can we detect whether a doc comment must be added?

"AccountProxyInterface" !== "CurrentUserInterface" =====> needs doxygen.

"AccountProxyInterface" !== "CurrentUserInterface" =====> needs doxygen.

Ok, I almost thought I got it.
From that comment alone, if the property name (or parameter name) is a 1:1 transformation of the class or interface name (more precisely, the short name) from the type declaration, then the doc comment can be omitted.

I assume this would only apply for properties with a single type declaration, and only for object types. E.g. string $string or array $array does not count.

So compared to #16, it is less permissive.
In #16 I don't make any requirement for the property name.

But in the issue summary:

On the other hand AccountSwitcher::$currentUser does not need doxygen, because in any object where AccountProxyInterface $currentUser is a property we know what it is: the currrent user.

This contradicts my above understanding of your proposal. Here you say that for AccountProxyInterface $currentUser the doc comment _can_ be omitted, because it is used elsewhere.

+1000 overall, but I haven’t read every detail of every proposal, yet. @selfdoc seems like a reasonable solution for ensuring we thought about it, visual rhythm, etc.

+1 on every single word in the IS.

Mixing up properties with and without docblock is not a problem in other projects, e.g. https://github.com/doctrine/dbal/blob/3.6.x/src/Connection.php

-1 on the use of {@selfdoc} - Drupalism?

But the proposed resolution in the IS (as currently written) is not something we could get any of our tools to accurately enforce.

Allow omitting doxygen when the doxygen for that variable/property/method is not specific to the instance.

It relies on human discernment to decide what’s ” specific to the instance” or not, by way of a few annotated examples. How could any layer of our automated tools enforce this? We need something more specific than that, or our tools can’t enforce any such doc blocks, and our code will tend to degrade over time as things which actually do need docs sneak in.

You could argue that occasionally missing docs is better than the hordes of boilerplate docs we have now, and you might be right. but I’d rather adopt one of the proposals that’s enforceable by automation.

Partial reduction in clutter and noise, if we go the Drupalism route, can we use 1 line?

/// {@selfdoc}
protected Bar $bar;

We are putting a lot of effort maintaining our coding standards. Just another drupalism. Let's get off the island (again) and adopt one the popular PSR-* standards and focus on things that really matter (even, I know, I'll miss the 2 space indent and inline opening curly brackets)

Some of the PSR standards (cache, for example) are not fit for purpose, and for me the PHP-FIG organisation itself is also not fit for purpose - most of the member organisations including Drupal and Symfony have left some time ago.

At the moment, they have a coding style standard: https://www.php-fig.org/psr/psr-12/, but the documentation standard is still in draft: https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md.

So we can't adopt it even if we wanted to.

@catch there is also a proposal for PHPDoc tags (PSR-19). I submitted a request there yesterday.
https://github.com/php-fig/fig-standards/pull/1307

@Chi as we can see from the responses to your comment they're hostile to doing anything actually useful. People are welcome to try to make the PSRs work for us, but I gave up on that a decade ago and we should not adopt them for the sake of it. The Laminas debacle has shown that when you get off the island sometimes your raft falls apart.

/// {@selfdoc}
protected Bar $bar;

Seems like this would conflict with all properties needing to be documented with C-style comments. Also for me it wouldn't help the vertical rhythm issue compared to not having docs.

A few notes on @selfdoc

1. I think @selfdoc should only be used to replace:
a. A short description of the docblock when it duplicates function/property name
b. Tags that describe a signature (@pararm, @return)

That means all other contents of a docblock should stay with @selfdoc
Example:

/**
 * Gets the entity type manager.
 *
 * @return \Drupal\Core\Entity\EntityTypeManagerInterface
 *
 * @deprecated
 */
public function getEntityTypeManger(): EntityTypeManagerInterface {
  return $this->entityTypeManager;
}

This example needs to be replaced as follows.

/**
 * {@selfdoc}
 *
 * @deprecated
 */
public function getEntityTypeManger(): EntityTypeManagerInterface {
  return $this->entityTypeManager;
}

2. @selfdoc should not be mandatory. That means it's up to developers decide whether to use it or just omit the whole docblock. Actually, not sure about this.

3. If we want to align format of this tag with @inheritdoc as it described in PSR-19, it should be in came case and have two variations — w/wo curly braces.
However, currently Drupal always use inline variation of @inheritdoc (with curly brances) and it's not camelized.

Another case of duplicated docblocks is single method services and controllers.

/**
 * Generates URL.
 */
class UrlGenerator {

  /**
    * Generates URL.
    */
  public function generateUrl(): Url {
    // ...
  }

}

This probably deserves own issue.

The issue has stalled so I am narrowing to my proposal in #12. It does less than the original but it is easy to automate and unambiguous.

Once 📌 Use PHP 8 constructor property promotion for existing code Needs work is done, the property creation will happen in the constructor itself and there'll be no other property definition at all (see the before/after in the issue summary), I think it would be good for the issue summary to use something less service-y as an example where we still have similar boilerplate.

The issue summary already says "property or function parameter". It should be function/method parameter, indeed. Looking at the IS of the issue linked:

  public function __construct(
    protected EntityTypeManagerInterface $entityTypeManager, // no doxygen needed
    protected ConfigFactoryInterface $configFactory, // no doxygen needed
    protected TypedConfigManagerInterface $typedConfigManager, // no doxygen needed
    protected TranslationInterface $stringTranslation, // this still needs doxygen
    protected StorageInterface $activeStorage, // this still needs doxygen 
    protected EventDispatcherInterface $eventDispatcher, // no doxygen needed
    protected EntityRepositoryInterface $entityRepository, // no doxygen needed
    protected ExtensionPathResolver $extensionPathResolver, // perhaps it's worth extending the rule so no doxygen would be needed.
  ) {}

Out of eight, five is covered, a sixth could be covered by a minor modification of making the Interface append match the type:

Take the name of a property or method/function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then doxygen is optional.

I added this new rule to the IS.

This also strongly suggests TranslationInterface should be called StringTranslationInterface. That's a separate issue, of course.

When https://www.drupal.org/project/drupal/issues/3294266 📌 Allow plugin service wiring via constructor parameter attributes Needs work gets in, we could add here or in a separate issue: don't mandate doxygen on constructor parameters which are known to be a service. PHP 8 constructor property promotion this will also take care of properties: they'll be gone in this case anyways.

Updated to use the new template for coding standard.

I think the proposed standard is fine, but 90% or more of what it would affect, is going to be covered by constructor property promotion and #2140961: Allow constructor methods to omit docblocks → .

We still have 📌 Use PHP 8 constructor property promotion for existing code Needs work open, so core is full of the current boilerplate, but it would be useful to see an example that's not covered by that issue to see what would remain in core that this would affect.

Sure.

In EntityFormInterface you can find:

  /**
   * Sets the entity type manager for this form.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   *
   * @return $this
   */
  public function setEntityTypeManager(EntityTypeManagerInterface $entity_type_manager);

The entire doxygen is completely superflous although the proposal would only drop @param.

In AccountSwitcherInterface you have

   * @param \Drupal\Core\Session\AccountInterface $account
   *   The account to switch to.

and also here the @return $this could be dropped in another proposal with a :static return type, same as with the previous one.

In Drupal\block_content\Routing\RouteSubscriber you can find

   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type.

In FieldTypePluginManagerInterface:

   * @param \Drupal\Core\Field\FieldStorageDefinitionInterface $storage_definition
   *   The field storage definition.

and so on.

quietone → credited Dane Powell → .

quietone → credited drunken monkey → .

quietone → credited euphoric_mv → .

quietone → credited joachim → .

quietone → credited pfrenssen → .

quietone → credited slootjes → .

quietone → credited TravisCarden → .

quietone → credited Wim Leers → .

Closed #2938784: Do not require descriptions in variable comments → as a duplicate, adding credit.

I support this.

I support this.

I support this.

I support this

The current example in the issue summary looks like a property definition for something that would be constructor property promoted now and hence not need a property definition at all, this is already covered by existing coding standards.

It would be good to see examples, ideally real ones from core, where this rule would be applied that aren't about constructors.

I think \Drupal\Core\Messenger\MessengerTrait might be a good example.

Re #56: see #41. Someone needs to update the summary to use some of those as the examples…

Totally support this, now that it’s focused on something we can automate.

Perfect, thanks @chx

I'm setting to NW because of "Needs followup" tag.

That tag was added in #9 as a request to keep the scope limited to class properties. Life moved on. The vast majority of class properties will be auto-documented via constructor property promotion. The whole scope of this issue now is for method params and any non-promoted class members. No follow up needed. Removing tag and restoring status.

Scratch that, moving to RTBC so our committee can review this at our next meeting. I don’t believe anything else is needed for that to be the next step.

Sorry I have one more question/comment about this:

If a method has one, interface, parameter, then it is easy to drop the entire doxygen.

However if a method has two parameters, one that's an interface and one that's a bool, then it would be strange to leave the first parameter undocumented and only document the second.

So do we need to add explicit language that doxygen can only be dropped when every parameter is self-documenting?

It might also be easier to automate.

I would argue that "omit the doxygen" is unclear terminology.
There is no "doxygen" in php, and even outside php, I don't think "doxygen" means a specific part of the doc comment, but rather the mechanism to generate a documentation from doc comments.
For "omit the doxygen" it is not clear whether we omit the param description, the entire `@param` tag, or the entire doc comment.

So instead we should say "omit the param description" or "omit the param tag" or "omit the entire doc comment".

So do we need to add explicit language that doxygen can only be dropped when every parameter is self-documenting?

Absolutely.
Something like this:
- A function doc must have either _all_ `@param` tags or none of them.
- A param doc description can be omitted if the type is a single class or interface, and the parameter name is a derivative of the type name.
- A param doc tag is "trivial" if it only replicates the native parameter type.
- If all parameters are "trivial", then all of them can be omitted.
- If all parts of the doc comment are trivial, then the entire doc comment can be omitted.

Problem/Motivation

This is a followup to #3238192: Allow omitting @var for strictly typed class properties → . That has this code snippet:

  /**
    * Where one can order a soda.
    */
  protected Bar $baz;

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

  /**
   * Sets the entity type manager for this form.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   *
   * @return $this
   */
  public function setEntityTypeManager(EntityTypeManagerInterface $entity_type_manager);

The entire doxygen is completely superflous although the proposal would only drop @param. We should strive dropping it in its entirety by typing the return then dropping the return and then finally the text.

Or in Drupal\block_content\Routing\RouteSubscriber::childRoutes you can find

   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type.

This is a waste, completely pointless and it is actually harder to read. Because

EntityTypeInterface $entity_type

already totally tells you everything there is.

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Replace 99% of the current doxygen with typing the properties. Rejoice as the amount of boilerplate collapses to a fifth.

Three supporters required

1. @pfrenssen →
2. @nilsdestoop →
3. @claudiu.cristea →

Proposed changes

Drupal API documentation standards for functions →

Each parameter of a function must be documented with a @param tag (see exceptions below).

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

@var: Class property data types →

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible.

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible. It is also permissible to completely omit the doxygen if the name of the variable -- append the string Interface as needed -- equals the the type.

Remaining tasks

2. /li>
4. Review by the Coding Standards Committee
5. Coding Standards Committee takes action as required
6. Tagged with 'Needs documentation edits' if Core is not affected
7. Discussed by the Core Committer Committee, if it impacts Drupal Core
8. Documentation updates
   1. Edit all pages
   2. Publish change record
   3. Remove 'Needs documentation edits' tag
9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a fuller explanation of these steps see the Coding Standards project page →

(ignore the previous comment)

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

The `setEntityTypeManager()` example is interesting.
The `@param` doc and its description are redundant, because we already have the native type declaration and the first line of the doc comment.
The `@return $this` is _not_ redundant.
The method doc description looks a bit redundant, but actually, I might want more information:
This method needs to be called before the class can be used.
In fact this method reveals a design flaw, where we don't have automatic dependency injection for this class, and instead there are various places where the method is called explicitly.

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

Again "doxygen is optional" can be confusing. What exactly do we omit? The description, or the entire `@param` tag, or the entire doc comment?

I think the "take the name of a function parameter..." logic is a placeholder for "The parameter is a service".
Usually, if it is not a service, then we should assume the parameter to play a specific role in the class, which deserves to be described.

Btw, to see how a lack of documentation looks like, we only need to look at the symfony codebase.
I find this painful to read :)

The `@return $this` is _not_ redundant.

It would be redundant if the method had a return type though wouldn't it?

It would be redundant if the method had a return type though wouldn't it?

The only native return type could be `*(): static`, which is less specific than `@return $this`.
E.g. `): static` would still be correct on a wither method that returns a clone. `@return $this` must not return a clone, it must return the object itself.

then it would be strange to leave the first parameter undocumented and only document the second

I think it's ok to only document parameters that need clarification. That's quite typical case when parameter type is extended via psalm/phpstan.

I think it's ok to only document parameters that need clarification. That's quite typical case when parameter type is extended via psalm/phpstan.

It would look quite alien and unexpected in a Drupal codebase, to have param docs for 3 out of 5 parameters of a method.
Having the complete list of parameters gives a sense of symmetry and completeness.
Also it is easier for an IDE to generate the doc comment, and warn if it is incomplete.

But maybe it is just a question of familiarity.
Do you know of 3rd party code with incomplete parameter docs?
(finding such code is the first step, we might still decide that we don't like it)

Maybe we should create an MR with example doc removals.

It would look quite alien and unexpected in a Drupal codebase, to have param docs for 3 out of 5 parameters of a method.

Even if all of these parameters are self documented you might want to extend documentation for just one of them. Typical use case is clarifying types for arrays. Like Some/Type[] instead of array. With "all or nothing" policy you will likely just ignore this need.

Do you know of 3rd party code with incomplete parameter docs?

Example from typhoon library.

The scope of this issue is a bit unclear. Initially it was about @var tag, but in comments mostly reference @param tag.

It think it should apply to @return tag as well.

Maybe I'm missing something but I can't see anything in https://github.com/typhoon-php/typhoon/blob/0.3.x/src/Reflection/Metadat... which only documents some and not all parameters on the same method?

@catch all other classes in that project follow that pattern.
For example https://github.com/typhoon-php/typhoon/blob/0.3.x/src/Reflection/Metadat...

::fromFileContents() has @param tag for $file parameter only because it need extra clarification for Psalm (non-empty-string). All other parameters in that class have no tags. Also note missing @return tags as the return type is clear from typehint.

::fromFileContents() has @param tag for $file parameter only because it needs extra clarification for Psalm (non-empty-string). All other parameters in that class have no tags. Also note missing @return tags as the return type is clear from typehint.

Interesting example.
For the $contents parameter, the comment description should explain that, if it is provided, the actual file contents are ignored, but the $file is still passed to FileChangeDetector. Maybe in less words.
The ChangeDetector::changed() could have a doc description saying "TRUE, if the resource has changed since the detector was created.".
Also the classes could have doc descriptions.

In this example the code is actually relatively simple, so we can make sense of it without comments.
And tbh there is a risk that a poorly written description is more misleading than helpful. I am already unhappy with my proposed comment above.
Still, in general I tend towards having doc descriptions on most symbols over not having them.

We could have a rule that says, more of less:

If any @param statements are required, all must be documented but the descriptions are optional for self-documenting cases.

Eg

/**
 * Does the important and weird thing.
 *
 * @param EntityTypeManager $entity_type_manager
 * @param bool $is_special
 *   TRUE if the special recursively cached access checking 
 *   on even numbered entities is necessary, otherwise FALSE. 
protected function doImportantWeirdness(…): void {
   …
}

Forgive typos / formatting, I’m typing this comment on my phone while boarding a flight. But hopefully it’s “clear”. 🤓😆

Also, re end of #66, I think this is definitely not just about services.

for example, this is a common parameter for which this documentation would be redundant / pointless:

* @param EntityInterface $entity
*   The Entity to operate on.

Or UserInterface $user, etc.

#77 looks OK to me visually, doesn't have the 'spot the difference' problem. Would be good to know if phpcs supports it via configuration. I guess it would have to be something like 'docblock is optional. If there is a docblock, all parameters must be documented' and then make the parameter description optional in phpcs and it's up to humans to decide which need a description?