Problem/Motivation

Similar issue to #2140961: Allow constructor methods to omit docblocks.

We don't use private very much in Drupal, but when we do it will usually be in a situation where code has been factored out of a protected method into a helper method that only makes sense with the specific implementation of the protected method - if you override the protected method, you can do so without the helpers.

An example is in #3593963: Field loading can hit the MySQL 61 table join limit if there are ~60 fields where the two new methods in that MR would nearly be closures (which don't need a docblock), but because they're long, they're in separate methods, but now those helper methods need massive docblocks because what were previously local variables now aren't. But documenting the variables doesn't help to understand the code because all of the logic is @internal.

Benefits

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

Three supporters required

  1. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  2. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  3. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)

Proposed changes

Create an MR with all proposed changes.

2. Repeat the above for each page or sub-page that needs to be changed.

Remaining tasks

  1. Create this issue in the Coding Standards queue, using the defined template
  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Discussed at a Core Committee meeting, if it impacts Drupal Core
  7. Final review by Coding Standards Committee
  8. Documentation updates
    1. Commit the MR
    2. Publish change record
    3. Remove 'Needs documentation edits' tag
  9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

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

Comments

catch created an issue.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

While the doc comment in your example (here, for easier reference) definitely looks horrible and is of disputable usefulness, our coding standards have to target the general case and with that in mind I don’t see what the private modifier really changes here. Since we’re an Open Source project the doc comments aren’t just for people who extend or use a certain module/library/API but also for the next people who will work on this code. Whether a method is protected or private should therefore not make much difference, as a first step.

I’d rather say what makes this doc comment potentially useless is that the method is called from just a single place, with variable names almost entirely unchanged, so it really has the character of a closure and can (or almost has to) be viewed in the context of its caller to make sense of it. (In that case, the private modifier is important of course, since it ensures we really only have that one caller to worry about.) Furthermore, both the one-line description and most parameter comments are more or less just human language versions of their respective names, adding almost no value. However, I think the description maybe should be expanded here to explain what the methods does, and the comment for $load_shared_table_fields is also useful to have. So maybe even here the doc comment makes sense to encapsulate logic and explain it without having to read the code – the very reason this is a separate method, not just inline code in loadFromDedicatedTables().

In conclusion, I would be strictly against a blanket permission for private methods to skip their doc blocks, at the very least we’d need some additional rules. But I’m not sure that that’s really worth the additional complexity, either, with us having so few private methods to begin with. I’m always more for simple, general rules with as few exceptions as possible, even if that means having a few pointless doc comments. And maybe having to do them will even sometimes lead to additional helpful information being added to a doc block that might have looked pointless at first.

catch’s picture

catch
he/him
Primary language English
commented

In conclusion, I would be strictly against a blanket permission for private methods to skip their doc blocks, at the very least we’d need some additional rules. But I’m not sure that that’s really worth the additional complexity, either, with us having so few private methods to begin with.

That's not really how we approach documentation in Drupal core though. At the moment if you skip adding documentation to the methods I introduced, you get a phpcs failure, so it's impossible not to. On the other hand, if you don't get a phpcs failure, a human reviewer can still ask for documentation where they think it's need.

For a corollary, we don't have any coding standard, much less phpcs rules, that say you need to add long inline comments where there is some tricky logic, but we have lots of long inline comments where something tricky happens because people ask for it.

However, I think the description maybe should be expanded here to explain what the methods does, and the comment for $load_shared_table_fields is also useful to have.

I think this would be better and more readable in an inline comment next to where they're called/used - but then we're duplicating the phpdoc with the current rules.

I’d rather say what makes this doc comment potentially useless is that the method is called from just a single place, with variable names almost entirely unchanged, so it really has the character of a closure and can (or almost has to) be viewed in the context of its caller to make sense of it.

100% agreed with this.