Problem/Motivation

The changes required for 'Drupal.Commenting.DocComment.ShortSingleLine' are too large and need to be split into child issues.

Proposed resolution

Fixes for sniff 'Drupal.Commenting.DocComment.ShortSingleLine' that are just adding new lines.

Remaining tasks

Patch
Review - Instructions for adding the sniff and running the test are in the Issue Summary of #3123060: Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard
Commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3268746-5.patch11.75 KBquietone
#5 interdiff-2-5.txt472 bytesquietone
#2 3268746-2.patch12.53 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
StatusFileSize
new12.53 KB

And here is a patch.

xjm’s picture

+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultiple.php
@@ -94,6 +94,7 @@ public static function multipleValidate($element, FormStateInterface $form_state
   /**
    * {@inheritdoc}
+   *
    * Used in \Drupal\field\Tests\EntityReference\EntityReferenceAdminTest::testAvailableFormatters().
    */

This is the only zinger here. {@inheritdoc} cannot coexist with any other docs. So I would split this out of the patch, and we can decide whether to delete the second line or to replace the inheritdoc with a full docblock.

I also note that a lot of the one line summaries are missing the infamous "s" (the initial verb is supposed to be in the indicative third person). However, fine with scoping that to a followup since that problem likely exists many other places in core as well. So the rest of this patch seems fine once the above hunk is moved out.

xjm’s picture

Status: Needs review » Needs work
quietone’s picture

StatusFileSize
new472 bytes
new11.75 KB

Ah, yes the infamous 's'. If a sniff is helpful with that I'd wait for a sniff. If not, then I would wait until all the existing sniffs are fixed. Otherwise, it feels like chasing one's tail.

Followup made for the 'zinger' #3269000: fix docblock of TestFieldWidgetMultiple::isApplicable which is removed from this patch.

quietone’s picture

Status: Needs work » Needs review
shashwat purav’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #5 is applied successfully to 10.0.x branch.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

@Shashwat Purav, thank you for reviewing this issue!

The automated testing infrastructure tells us whether the change set still applies, so we do not need people to review that. It is also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

Also see the issue credit guidelines for more information on which kinds of contributions are credited.

xjm’s picture

Version: 10.0.x-dev » 9.3.x-dev
Status: Needs review » Reviewed & tested by the community

Good to go. This should be backportable all the way to 9.3.x. (We would not backport the rule, just the cleanups where they apply.)

xjm’s picture

Title: Fix newlines for 'Drupal.Commenting.DocComment.ShortSingleLine' » Fix missing newlines for 'Drupal.Commenting.DocComment.ShortSingleLine'

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3268746-5.patch, failed testing. View results

quietone’s picture

Status: Needs work » Reviewed & tested by the community

random fail on Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove, retesting.

restoring rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3268746-5.patch, failed testing. View results

spokje’s picture

Status: Needs work » Reviewed & tested by the community

Random fail (again) on Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove, re-RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3268746-5.patch, failed testing. View results

spokje’s picture

Status: Needs work » Reviewed & tested by the community
quietone’s picture

Version: 9.3.x-dev » 10.0.x-dev

Changing the version and starting tests for 9.5.x

alexpott’s picture

Version: 10.0.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed b6a3c63309 to 10.0.x and 37f6b990ef to 9.5.x and 49ce2e6d6e to 9.4.x and 7177a7cbe1 to 9.3.x. Thanks!

Backported to 9.3.x since this is only documentation.

  • alexpott committed b6a3c63 on 10.0.x
    Issue #3268746 by quietone, xjm: Fix missing newlines for 'Drupal....

  • alexpott committed 37f6b99 on 9.5.x
    Issue #3268746 by quietone, xjm: Fix missing newlines for 'Drupal....

  • alexpott committed 49ce2e6 on 9.4.x
    Issue #3268746 by quietone, xjm: Fix missing newlines for 'Drupal....

  • alexpott committed 7177a7c on 9.3.x
    Issue #3268746 by quietone, xjm: Fix missing newlines for 'Drupal....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.