Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
25.19 KB

Patch attached enables the Drupal.Commenting.FunctionComment sniff and changes the severity of all the other sub-sniff of the rule so that phpcs passes with the patch.

The patch here is based on the @anoopjohn's work in #2700661: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation (part 1)

alexpott’s picture

Cleaning it up a bit.

anoopjohn’s picture

I had earlier created this issue and had added this as a child of the part 1 issue #2707335: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation - Follow up issue for less simple fixes. Mark that issue as duplicate?

alexpott’s picture

@anoopjohn ah - sorry I didn't see that issue. My fault. Let's choose this one because the comments changes are more correct. You changes to phpcs.xml.dist on the other issue are great though. So if you could bring them into this issue that'd be fantastic.

alexpott’s picture

In #2707335-4: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation - Follow up issue for less simple fixes @anoopjohn asks

Should we exclude all rules sub sniffs [edit] in phpcs.xml or just exclude the ones that actually need fixes?

imo it makes to just exclude the ones that actually need fixes - less noise and better coding standards checking for the win!

anoopjohn’s picture

Status: Needs work » Needs review
FileSize
23.4 KB
6.68 KB

I have applied the exclusion rules as per #2707335-4: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation - Follow up issue for less simple fixes and have re-created the patch from #3 above. On rerunning the excluded rules I see that the number of sub sniffs that have to be excluded is more than the list I had listed along with the example shown. Not sure how I got the initial subset. I have made necessary changes and phpcs passes with the set of rules in phpcs.xml.dist now. The set of sub sniffs is still smaller than all the sub sniffs though.

I did a bit of grouping / mapping of the sniffs and sub-sniffs that fail and had created and shared a google spreadsheet with the details at #2571965-52: [meta] Fix PHP coding standards in core

Status: Needs review » Needs work

The last submitted patch, 7: 2707641-7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

The test fail has nothing to do with the @anoopjohn's changes.

andypost’s picture

Overall looks good

+++ b/core/lib/Drupal/Core/Render/Element/Button.php
@@ -72,10 +72,9 @@ public static function processButton(&$element, FormStateInterface $form_state,
    *   An associative array containing the properties of the element.
-   *   Properties used: #attributes, #button_type, #name, #value.
-   *
-   * The #button_type property accepts any value, though core themes have CSS that
-   * styles the following button_types appropriately: 'primary', 'danger'.
+   *   Properties used: #attributes, #button_type, #name, #value. The
+   *   #button_type property accepts any value, though core themes have CSS that
+   *   styles the following button_types appropriately: 'primary', 'danger'.

+++ b/core/lib/Drupal/Core/Render/Element/Date.php
@@ -76,11 +76,9 @@ public static function processDate(&$element, FormStateInterface $form_state, &$
-   *   #attributes, #id, #name, #type, #min, #max, #step, #value, #size.
-   *
-   * Note: The input "name" attribute needs to be sanitized before output, which
-   *       is currently done by initializing Drupal\Core\Template\Attribute with
-   *       all the attributes.
+   *   #attributes, #id, #name, #type, #min, #max, #step, #value, #size. The
+   *   #name property will be sanitized before output. This is currently done by
+   *   initializing Drupal\Core\Template\Attribute with all the attributes.

usually we describe properties at the class annotation in other elements

alexpott’s picture

@andypost this is not documenting the properties of the render element it is documenting what the pre-render callback is using as and as such is exactly how all the other render elements do it. See Checkbox::preRenderCheckbox() for example.

alexpott’s picture

rerolled.

+++ b/core/phpcs.xml.dist
@@ -34,6 +34,39 @@
+  <!-- The excluded sub-sniffs are to be fixed in core before Drupal.Commenting.FunctionComment passes fully.-->

I don't think this comment is necessary - this file is self-documenting enough.

andypost’s picture

+++ b/core/phpcs.xml.dist
@@ -18,6 +18,37 @@
+    <exclude name="Drupal.Commenting.FunctionComment.ReturnCommentIndentation"/>

subject of issue is exactly about this, why excluded?

alexpott’s picture

@andypost nope - it is about Drupal.Commenting.FunctionComment.ParamCommentIndentation

It is excluded to make these changes manageable. Once this in we can file to remove and fix each exclusion one by one.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Then it looks completed

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 638db4d and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed b13374f on 8.2.x
    Issue #2707641 by alexpott, anoopjohn: Ensure core compliance to Drupal....

Status: Fixed » Closed (fixed)

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