Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard

There are a lot of changes in that MR. I only reviewed those in a migrate namespace.

There are a lot of changes in that MR.

Tell me about it, I manually changed them all ;)

Thanks for the partial review!
I think attacking it in chunks is the only way to keep your sanity.

This time I started at the top of the MR and worked down. The next file for me start at is core/lib/Drupal/Core/Image/ImageInterface.php.

Wow, this one will be nice to fix but will be tricky to drive home - there's a lot to discuss here I think. Great start though - and yes reviewing it is quite the effort, I will carry on later.

My next starting point is core/modules/comment/src/Plugin/views/sort/StatisticsLastCommentName.php

I think my comments are getting shorter and shorter - there are so many changes to review here.

I looked at the two unresolved issues. I did not review the resolution of my previous comments.

My next starting point is core/modules/media/tests/src/Functional/UrlResolverTest.php

I think my comments are getting shorter and shorter - there are so many changes to review here.

@quietone: I can't blame you, there are a lotof changes to review indeed.

Short comments are fine with me, with so many stuff to review we can skip the politeness of "Maybe we can use ...", or "Perhaps this fits better.".
Just whack in your proposed text, since so far, they were always better then my feeble attempts.

Thanks for sticking with this, it's a lot of work, but I really would like this sniff in to stop it from getting worse.

I finally made it to the end.

@Spokje You have made a lot of nice changes! Yes, would be good to get this in but it is 480K. That is a lot to ask of a committer in one issue.

I may not get back to this until the end of the week.

@quiteone:

I finally made it to the end.

🎉🥳

@Spokje You have made a lot of nice changes!

Which you changed to even better stuff ;)

Yes, would be good to get this in but it is 480K. That is a lot to ask of a committer in one issue.

Agreed, but I couldn't see a nice way of splitting this into smaller sub-issues without it taking a long time.
I hope having it in a MR makes reviewing (Thank you, @quiteone and @longwave) and commiting a bit easier due to the nice(?) GitLab GUI for changes.

I may not get back to this until the end of the week.

No problem, I'm happy the both of you went through it in the first place.

I am hoping to resolve all the current issues and then review again from top to bottom with a freshly rebased MR.

I am hoping to resolve all the current issues and then review again from top to bottom with a freshly rebased MR.

Thus spoketh @quietone in #24

Godspeed, my son! ;)

I've resolved a few comments where you agreed with me and made 2 changes you've suggested.

In the process also rebased the MR.

Trying to address the unresolved issues.

I intend to start reviewing this again from top to bottom as is said.

Brave man!

Starting from the top again. The next file to review is core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php.

Thanks @quietone, resolved all but one thread and rebased MR on 9.3.x

Next file for me is core/lib/Drupal/Core/Image/ImageInterface.php

I have not yet reviewed the changes from recent reviews.

Next starting point is core/modules/action

Next up is module/quickedit

Finished reviewing the remaining files. Phew! I keep seeing things to improve and had to be careful to stay in scope. I hope I succeeded.

Finished reviewing the remaining files. Phew! I keep seeing things to improve and had to be careful to stay in scope. I hope I succeeded.

Thanks @quietone, I know this is a "monster" to review, I think you did _very_ well :)

2 remaining issues.

@Spokje, thanks. thins is looking really good! I think we need another set of eyes (plus resolve the remaining unresolved) before having a committer look at this.

I do want to point out there are summary lines for tests here that start with 'Create ...'. Some of those got changed on my suggestion and perhaps are out of scope. If that is true, I hope they can stay as the change is an improvement. But not all the summary lines for tests that start with 'Create ...' have been changed. Along the way I became more concerned about scope and didn't comment on those. So, there are some inconsistencies in the changes here. Overall, though the additional coding standard is met and there are many improvements.

@quietone:

I think we need another set of eyes (plus resolve the remaining unresolved) before having a committer look at this.

Fully agreed.

[snipped] But not all the summary lines for tests that start with 'Create ...' have been changed. So, there are some inconsistencies in the changes here. Overall, though the additional coding standard is met and there are many improvements.

I see the phrase "many improvements", so I'm happy with a bit of scope-creep there.
Although I think a follow-up for getting _all_ the summary lines for tests that start with 'Create ...' changed is needed?

1 unresolved thread left.

Rerolled, resolved one of the threads from @quietone.

One more open thread, set to NR to get more eyes/brains/random body-parts on that one.

@skopje: Feel free to disagree, good arguments win the day. Also #3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed has landed, therefor you might get more fails from the testbot.

- Rebased MR on 9.4.x
- Merged latest commits

• Rebased MR on 9.4.x
• Merged latest commits of 9.4.x
• Fixed new violations of Drupal.Commenting.DocComment.ShortSingleLine
• Resolved threads opened by daffie (thanks @daffie for the Review)

Although there's still one open thread, putting this on NR to get more eyes/brains/random body parts on this issue.

The MR looks good to me. Just a couple of nitpicks.
I have created #3250482: The docblock of \Drupal\views\Plugin\views\cache\CachePluginBase::cacheSetMaxAge() is wrong.

Resolved all threads, thanks again @daffie for his eagle-eyed review.

All the changes look good to me.
The correct change is made to the file core/phpcs.xml.dist.
The IS is in order.
For me it is RTBC.

Added release note snippet to the IS.

#3250482: The docblock of \Drupal\views\Plugin\views\cache\CachePluginBase::cacheSetMaxAge() is wrong has landed. A small reroll is needed.

Resolved conflicts in CachePluginBase and ToolbarAdminMenuTest.

Back to RTBC.

We need to get a version of this that applies to 10.0.x as well - sorry.

Here is an MR for 10.0.x branch.

Changes made to /core/modules/field_ui/tests/src/Unit/FieldUiRouteEnhancerTest.php and core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php files in 9.4.x MR are ignored in this new MR as they no longer exist in 10.0.x branch.

Thanks!

Thanks everyone for all your work on this!

I discussed this issue with @quietone and have a couple thoughts:

First of all, in its current form, this is a disruptive (and easily disrupted) change set that would be best done as a scheduled beta target. Tagging accordingly. The next beta phase begins May 23.

Secondly, given the scope and nature of this change set, this is an issue where I would split addressing the coding standards rule into several logically scoped steps. Specific notes:

1. This patch cannot be reviewed programmatically, so it isn't necessarily a good fit for "one issue, one coding standards rule".
2. Classes, methods, and member variables all have different docblock standards, so this would be easier to review if the patch were split up into sub-issues by the type of docblock.
3. A single child issue of all docblocks that are fixed simply by adding a newline would be easily scannable regardless of size.
4. However, reviewing rewritten documentation is a much larger task. Many of the docblock summaries here have too little information after the update. (I can propose improvements for a lot of them, but holding off on that for now until we settle the scope and scheduling.)
5. Some of the new documentation also has some grammatical errors. I don't want to give an overwhelming amount of feedback on that either. It'd be more manageable for both reviewers and MR authors to give that feedback on smaller subsets at a time.
6. In general, the ideal size change set for reviewability is between 100-400 LOC. (Reference: Best practices for peer code review.) Since this patch is mostly English documentation rather than code, our brains can probably handle the larger end of that, but the current 351 files changes, 1124 insertions, 740 deletions is probably too much.
7. Edit: Meant to mention also that this in some cases requires reading the whole class etc. to ensure the docs changes are accurate, so that's another reason that smaller subsets of changes would be good.

I did have a think about whether it would be better to go ahead with the current MRs since so much work has gone into them already, versus spending more time splitting them up. I think using a git add -p or interactive merge would make it fairly doable to split this up without too much effort. Splitting it up might also help us avoid having to schedule the patch (and therefore having to spend more time later getting it to apply again).

Those are my thoughts for now, although I'll think more about it overnight and maybe I can help split it up quickly.

Thanks everyone! Truly impressive effort here.

Changing parent.

Making this one the final issue to be done when all the children are complete. This one will enable the sniff and fix any remaining errors.

Postponing until the sibling issues are complete.

Here is the patch that just enables the sniff.

#3268741: [meta] Fix Drupal.Commenting.DocComment.ShortSingleLine is not complete and per #55, this is blocked on all other children of #3268741: [meta] Fix Drupal.Commenting.DocComment.ShortSingleLine being completed first.

Hence removing beta target, since this unfortunately did not make it into 9.4.x in time. I think this will be a beta target again, but now for the next beta? Keeping the tag.

Also, it might be better to mark #3268838: Fix functions and test modules for Drupal.Commenting.DocComment.ShortSingleLine as a beta target for 9.5.x, since that'll likely get more eyes on the issues that are actually blocking this one? But … #3268741: [meta] Fix Drupal.Commenting.DocComment.ShortSingleLine is targeting 10.0.x, it never even targeted 9.4.x. Which is probably why that didn't happen?

Yes, removing beta target tag.

The sibling issues both landed so we can enable the sniff and fix any stragglers here.

Ah, you beat me to changing the status!

It looks it was decided to fix the class properties as well in this last issue. That was only 12 files and the patch is small, so that is reasonable.

A few nits:

1. +++ b/core/modules/editor/tests/src/Functional/Update/EditorAddLazyLoadImageFilterUpdateTest.php
   @@ -7,8 +7,7 @@
   + * Tests update of editor_file_reference with editor_image_lazy_load filter.
   

   This is difficult because of the length of the snake_case_words but "Tests adding editor_image_lazy_load filter to editor_file_reference." is a bit clearer?

2. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
   @@ -54,8 +54,7 @@ class EntityReferenceFormatterTest extends EntityKernelTestBase {
   +   * An entity that in not yet saved to its persistent storage.
   

   in -> is

3. +++ b/core/modules/migrate/src/Plugin/Migration.php
   @@ -205,8 +205,7 @@ class Migration extends PluginBase implements MigrationInterface, RequirementsIn
   +   * The source_row_status for current map row.
   

   "The source_row_status for the current map row."

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -87,16 +87,18 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
   +   * A list of field that need a unique value.
   

   field -> fields

@longwave, thanks for the review.

All comment changes look good to me.
The PHPCS rule has been enabled in the patch.
For me it is RTBC.

@daffie, thank you.

Adding patches for 9.5.x and 10.0.x.

We can only enable the sniff in a minor release, so I committed the change to phpcs.xml.dist to 10.1.x only and skipped it for the other branches.

Committed and pushed to 10.1.x, 10.0.x and 9.5.x. Thanks!

Whoops, committed 10.0.x and 9.5.x with the wrong commit message - reverted and re-committed with the right message.

Whoops, this is breaking testing on the 9.5.x branch right now. E.g. see https://www.drupal.org/pift-ci-job/2657088 (from #3356283: Fork laminas/laminas-diactoros):

There was 1 failure:

1) Drupal\Tests\Core\Theme\ClassyPreprocessUnchangedTest::testNoNewPreprocess
The file hash for classy.theme has changed. Any additions or changes to preprocess functions should be added to the themes that inherit Classy. 
If the changes to classy.theme are not changes to preprocess functions, update the hash in this test to: '1a5f162bc900c45957aaa89959bcb607' so it will pass.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'1a5f162bc900c45957aaa89959bcb607'
+'c42ff3a1291a258b42f0c44010cd28c7'

Since this patch touched classy.theme on the 9.5.x branch, we need to update core/tests/Drupal/Tests/Core/Theme/ClassyPreprocessUnchangedTest.php to match.

Not sure if this should be reverted and re-re-committed, or if we just want a hot-fix. Should the hot-fix be here or a separate issue?

Thanks!
-Derek

p.s. Oh wow, the 9.5.x test failed the same way above. See https://www.drupal.org/pift-ci-job/2655998...

Here's the hotfix for the current 9.5.x branch. Not sure if it should go here or a follow-up, but making it easier for whomever wants to move this forward.

Thanks!
-Derek

p.s. I swapped the order of the hash and the $hash since assertSame() wants the expected value first. The failure output was confusing.

Looks good to me.

My bad, I thought it was a random fail and didn't check properly. Pushed #75 as a hotfix to 9.5.x, many thanks for spotting and fixing this!

@dww and longwave, Thank you! I should have come back and checked on this.