Part of #2909364: [meta] Fix 'Drupal.Commenting.VariableComment' coding standard.

See parent issue for instructions.

CommentFileSizeAuthor
#12 drupal-coding-standards-2909368-12.patch11.36 KBmfernea
#7 fix-2909368-7.patch10.56 KBreflie
#3 drupal-coding-standards-2909368-3.patch11.82 KBmfernea
#2 drupal-coding-standards-2909368-2.patch11.82 KBmfernea

Comments

mfernea created an issue. See original summary.

mfernea’s picture

mfernea commented
Status: Active » Needs review
Issue tags: +Novice
StatusFileSize
new drupal-coding-standards-2909368-2.patch11.82 KB

Here is the patch.

mfernea’s picture

mfernea commented
StatusFileSize
new drupal-coding-standards-2909368-3.patch11.82 KB

I updated the modifications to phpcs.xml.dist.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Issue tags: +Vienna2017
zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Issue tags: +Needs reroll

Sorry but patch #3 does not apply and needs to be rerolled

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Status: Needs review » Needs work
reflie’s picture

reflie commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new fix-2909368-7.patch10.56 KB

Here is reroll.

andriyun’s picture

andriyun commented

Seems like we have to move this issue to postponed until at least issue #2909372: Enable 'Drupal.Commenting.VariableComment.MissingVar' coding standard will not be fixed.
Patch in comment https://www.drupal.org/node/2909372#comment-12290589 introduce new fails

--- a/core/lib/Drupal/Core/Cache/Context/LanguagesCacheContext.php
+++ b/core/lib/Drupal/Core/Cache/Context/LanguagesCacheContext.php
@@ -13,7 +13,7 @@ class LanguagesCacheContext implements CalculatedCacheContextInterface {
   /**
    * The language manager.
    *
-   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   * @var \Drupal\Core\Language\LanguageManagerInterface $language_manager
    */
   protected $languageManager;

which don't visible for Drupal.Commenting.VariableComment.InlineVariableName now

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Status: Needs review » Postponed
Related issues: +#2909372: Enable 'Drupal.Commenting.VariableComment.MissingVar' coding standard

Sounds valid!

mfernea’s picture

mfernea commented
Status: Postponed » Needs review

Please let me disagree. :)
This patch is a lot simpler than the one at #2909372: Enable 'Drupal.Commenting.VariableComment.MissingVar' coding standard. Indeed, there might by some extra work to do on that ticket if this one gets merged, but I don't see this as a big problem.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Status: Needs review » Needs work

Changes to phpcs.xml.dist must be included in the patch, otherwise LGTM

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2909368-12.patch11.36 KB

Changes included.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Status: Needs review » Reviewed & tested by the community

Perfect. Patch applies and phpcs gives no error. We still must wait for DrupalCI to complete test build but it looks RTBC to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Added @zaporylie to credit list for reviews that ensured phpcs.xml.dist was included in the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b85856f417 to 8.5.x and f2fddd2f23 to 8.4.x. Thanks!

As this only changed code comments backported to 8.4.x to keep code in sync to make bug fixing easier.

  • alexpott committed b85856f on 8.5.x 
    Issue #2909368 by mfernea, reflie, zaporylie: Fix 'Drupal.Commenting....

Status: Fixed » Closed (fixed)

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