[policy, no patch] Allow docblock to be removed when modifying constructor params

+1 Yes please! If we're changing the params, we would normally have to be updating the docs to match. In this case, removing the docs is better than "fixing" them.

+1 from me too, if it's in scope to add to the docblock, it's also in scope to remove it altogether given the new coding standard. I co-incidentally ran into this today on #3379885: Use MessagesCommand in BigPipe to remove special casing of the messages placeholder and took the approach here.

+1 from me. It seems sensible. I tried to review #3344389: Inject entity.repository service instead of using \Drupal::service('entity.repository') in non-tests which touches a number of constructor docblocks and wondered "Why?"

I think we've been doing this in practice quite a lot already.

I think we have the necessary support, but we probably need proposed text changes though.

Meant needs work

I don't know how to word something, it would have to be specifically about constructor documentation, because any other situation where it's like 'oh here's some extra code to remove/update to a new coding standard' can easily turn into a bad example of scope creep.

The main reason to do constructor promotion and documentation removals at once when changing one parameter is it's exactly the same line of code, or immediately adjacent, and just deletions. And because it avoids the deadening feeling of adding boilerplate you know shouldn't be there just to match existing boilerplate that should already not be there.

I studied the issue scope document last night and this morning. I think the most reasonable thing is to add another row to the "Examples of good and bad issue scope" table.

Let me know what you think of this.

#9 seems good, maybe with a link to the coding standards entry for constructors for context?

Edited #9 to add the docs link.

This fell way off my radar. The docs in #9 seem great. Thanks for putting those together! I think the Why column text is excellent, and helps people understand when this is a good idea, and when it isn't.

All 5 of us who have participated so far support this policy change, and no one has yet registered any opposition. That said, 5 people isn't exactly a huge sample size. 😅 But generally, the sense I get from being on the coding standards committee and following Drupal Slack + issues is that folks are revolting against boilerplate (rightfully), and this is a step in the right direction to being able to freely remove things that aren't adding value to our codebase. So I'm going to be bold and move this to RTBC, to see whatever other governance steps have to be taken to make this official.

This was linked to the old documentation, which is no longer maintained.
This is the new location: https://project.pages.drupalcode.org/coding_standards/php/documentation/..., it is still not required, so this is seems right to me. RTBC++

Personally, I'd rather go in the direction of allowing partial documentation in constructors' docblocks, i.e. only add a @param for those arguments of the constructor that would benefit from it.

Actually in a sense I think we will hit this when raising PHPStan to higher levels, it will require docblock description for iterable arguments for instance.

In classes that implement just DIC services in the constructor the 'no docblock' policy makes sense, as with autowiring the self-explainability of the arguments is evident. But not in other classes.

@mondrake that's for #3376518: Allow omitting @var, @param and @return tags or their descriptions if the variable name and type is enough which we still need to get agreed.

fwiw I've been actively encouraging this approach rather than updating boilerplate we'll then have to remove later, it's much easier to review deletions of the entire docblock than additions, so another +1.

I've allowed this in practice, although there's a bit of a judgment call. If it's one docblock that's being touched within the scope of a single API, that's one thing. But if it's dozens, that's a lot of extra code to review to ensure that there isn't any mismatch between what's being removed versus what's being updated by the MR. Because you have to read the whole class and check the member docs/hinting of all the params...

Do we need to amend the scope policy for this, versus just leaving it at the committer's discretion?

Edit: #9 is actually pretty good. Can we add it to the IS and maybe iterate on that?

Updated the proposed resolution with the change from #9.

Would you like a rector rule to delete all constructor documentation in a codebase? Just say the word.

word

well, in that case https://gist.github.com/chx/67e1e655da2b8313bf1c938a233dcfdd

I applied the rule here: #3565152: [rector] Remove constructor comments it's pretty clean!

Hang on there... I think we can only delete constructor documentation when the class has members documented and they're 1:1 with constructor params/services/etc. That won't be all constructors in the codebase, not by far.

I did it to start the discussion. Catch already pointed out we need to avoid removing value objects and timplunkett pointed out that we should keep instances of mixed.

I think we can only delete constructor documentation when the class has members documented and they're 1:1 with constructor params/services/etc.

not according to the coding standards:
All classes and all of their methods (including private methods but excluding constructors) must be documented.

That reads as we can remove all constructors. I think you might be mixing up with the other issue to allow removing parameter docs in general when they are one to one and typed with an object.

As xjm said in #17 this has been happening already so this is just clarifying the practice in documentation.

I have updated the Scope Guidelines, Examples of good and bad issue scope section with the text from the issue summary.

Thanks!

Re: #24:

You have a logic error actually. 😀

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

This does not mean that no constructors require documentation. It only means that not all constructors require documentation.