Problem/Motivation

There is a proposal to relax the @file standard for autoloaded files ie classes, traits and interfaces. See #2304909: Relax requirement for @file when using OO Class or Interface per file and #2665744: @file is not required for classes, interfaces and traits

Proposed resolution

Run phpcbf against core.

Steps to create patch:

  1. Make sure phpcs/phpcbf has access to the latest coder rule set ie. 8.x-2.x as the rule in 8.x-2.6 is not updated for the new standard.
  2. Run phpcbf . --sniffs=Drupal.Commenting.FileComment --standard=Drupal --extensions=php from the core directory

Remaining tasks

commit the patch.

CommentFileSizeAuthor
#34 interdiff.txt557 bytesklausi
#34 file-comment-2665992-34.patch2.65 MBklausi
#30 29-30-interdiff.txt1.54 KBalexpott
#30 2665992-3-30.patch2.65 MBalexpott
#29 2665992-3-29.patch2.65 MBalexpott
#29 28-29-interdiff.txt409 bytesalexpott
#28 26-28-interdiff.txt3.07 KBalexpott
#28 2665992-3-28.patch2.66 MBalexpott
#26 2665992-3-26.patch2.66 MBalexpott
#21 2665992-2-21.patch2.66 MBalexpott
#18 2665992-18.patch902.59 KBheykarthikwithu
#2 2665992-2.patch2.67 MBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2665992-2.patch2.67 MB

Looks like we need to run the current rule before #2665744: @file is not required for classes, interfaces and traits is applied against HEAD and fix that so there is less noise in this issue.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented
Status: Needs review » Needs work 
diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
index ffe27e0..ff4a596 100644
--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.inc
@@ -1,5 +1,9 @@
 <?php
 
+/**
+ * @file
+ */
+
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\DrupalKernel;
 use Drupal\Core\Config\BootstrapConfigStorageFactory;
diff --git a/core/includes/tablesort.inc b/core/includes/tablesort.inc
index 65b791f..b496e6f 100644
--- a/core/includes/tablesort.inc
+++ b/core/includes/tablesort.inc
@@ -1,5 +1,9 @@
 <?php
 
+/**
+ * @file
+ */
+
 use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Core\Url;
 use Drupal\Component\Utility\UrlHelper;

Seems that we need to add those @file-docblocks with proper descriptions first.

The last submitted patch, 2: 2665992-2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes

@tstoeckler yep this was just an exploratory patch - it won't be committed probably until 8.1.x RC

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at erdfisch commented

Ahh, that's good to know, thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2572643: Fix 'Drupal.Commenting.FileComment' coding standard

@tstoeckler here is the issue to apply the current @file doc rules to core #2572643: Fix 'Drupal.Commenting.FileComment' coding standard - we should get that one it before this one.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

The new sniff is working well, though the fixer is too aggressive and deletes other docs with the following files:
core/lib/Drupal/Core/Datetime/DateHelper.php
core/lib/Drupal/Core/ProxyClass/*
core/lib/Drupal/Core/Routing/UrlGeneratorTrait.php
core/lib/Drupal/Core/StreamWrapper/StreamWrapperInterface.php
core/lib/Drupal/Core/Template/TwigExtension.php
core/lib/Drupal/Core/Template/TwigNodeTrans.php
core/modules/block/src/Tests/BlockHookOperationTest.php
core/modules/book/src/ProxyClass/BookUninstallValidator.php
core/modules/field/src/Tests/Views/FieldTestBase.php
core/modules/filter/tests/filter_test_plugin/src/Plugin/Filter/FilterSparkles.php
core/modules/forum/src/ProxyClass/ForumUninstallValidator.php
core/modules/language/src/ProxyClass/LanguageConverter.php
core/modules/views_ui/src/ProxyClass/ParamConverter/ViewUIConverter.php
core/tests/Drupal/Tests/Component/Utility/NumberTest.php
core/tests/Drupal/Tests/Listeners/DrupalStandardsListener.php

I posted #2676540: Fixer should not delete elaborate @file doc comments, but I will also create a patch for the nonstandard core docblocks in the meanwhile. I'll post a separate issue for the proxy classes since that should be fixed in the generator thingy.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Posted #2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks. Still need a separate issue for the auto-generated proxy classes.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Regarding #3, I think we should only fix classes/traits/etc. in this issue (easy to do by scanning only --extensions=php). Since adding @file docblocks where they are missing requires writing actual new documentation, and since it's also in violation of an existing standard to not have them, that's another child issue we could start now.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Posted #2676822: Add @file docblocks where they are missing for non-classes, interfaces, or traits for #10.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I totally missed that #2572643: Fix 'Drupal.Commenting.FileComment' coding standard was already committed to address that; closing as duplicate. Sorry.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Posted #2676836: Update proxy class generator for new @file standard.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.2.x-dev » 8.1.x-dev

We have agreed to target this cleanup for the 8.1.x RC phase, so moving back to 8.1.x. The patch should be regenerated around April 6, since the target codebase will be much more stable then (and hopefully related bugs will be fixed by then).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +rc target
heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Assigned: Unassigned » heykarthikwithu
Issue tags: +Needs reroll

patch did not apply, needs reroll.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
CreditAttribution: heykarthikwithu as a volunteer and at Valuebound commented
Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2665992-18.patch902.59 KB

Covers core/lib/*, still needs to be removed few more.
Since its becoming very big, hoping this can be moved in one commit and other few in next commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@heykarthikwithu the point of this issue to automate using the coder rule. This is not something that should be attempted by hand.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2665992-2-21.patch2.66 MB

So I've run the rule against 8.2.x and generated a new patch... we still have some problems.

If you grep the resulting patch using git diff | grep ".\* [D-Z]"

- * Some parts of this code were derived from the MediaWiki project's UtfNormal
  * Defines a chained cache implementation for combining multiple cache backends.
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Batch\BatchStorage' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Config\ConfigInstaller' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Cron' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Entity\ContentUninstallValidator' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Extension\ModuleInstaller' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Extension\RequiredModuleUninstallValidator' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Field\FieldModuleUninstallValidator' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\File\MimeType\ExtensionMimeTypeGuesser' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\File\MimeType\MimeTypeGuesser' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Lock\DatabaseLockBackend' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Lock\PersistentDatabaseLockBackend' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\PageCache\ChainResponsePolicy' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\ParamConverter\AdminPathConfigEntityConverter' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\ParamConverter\MenuLinkPluginConverter' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Plugin\CachedDiscoveryClearer' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Render\BareHtmlPageRenderer' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Routing\MatcherDumper' "core/lib/Drupal/Core".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\Core\Routing\RouteBuilder' "core/lib/Drupal/Core".
  * Implements hook_page_top().
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\book\BookUninstallValidator' "core/modules/book/src".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\field\FieldUninstallValidator' "core/modules/field/src".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\filter\FilterUninstallValidator' "core/modules/filter/src".
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\forum\ForumUninstallValidator' "core/modules/forum/src".
- * Tests for forum.module.
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\language\LanguageConverter' "core/modules/language/src".
- * Merge into 'link' formatter once there is a #type like 'item' that
- * Tests for menu_ui language settings.
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\node\ParamConverter\NodePreviewConverter' "core/modules/node/src".
- * Search query extender and helper functions.
- * The overarching methodology of these tests is we need to compare a given
- * This file was generated via php core/scripts/generate-proxy-class.php 'Drupal\views_ui\ParamConverter\ViewUIConverter' "core/modules/views_ui/src".

So we still have stuff to fix. I think we should deal with the proxy class generation in a separate issue. As the generator will need to change to not add the docblock.

Status: Needs review » Needs work

The last submitted patch, 21: 2665992-2-21.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Created #2700363: Remove @file in auto-generated proxy classes and add ignore coding standards as these files follow symfony to address the proxy classes - we should try to get that one in asap to reduce noise here. - that's a dupe of #2676836: Update proxy class generator for new @file standard

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Created #2700367: Consolidate miscellaneous docs from class @file docblocks into the class docblocks (part2) to catch the newly introduced and missed stuff from #2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Tagging as this is part of #2665992: @file is not required for classes, interfaces and traits

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2665992-3-26.patch2.66 MB

The patch attached includes #2700367: Consolidate miscellaneous docs from class @file docblocks into the class docblocks (part2) and #2676836: Update proxy class generator for new @file standard.

Status: Needs review » Needs work

The last submitted patch, 26: 2665992-3-26.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2665992-3-28.patch2.66 MB
26-28-interdiff.txt3.07 KB

So some of the error tests are dependency on line numbers... see interdiff.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
28-29-interdiff.txt409 bytes
2665992-3-29.patch2.65 MB

Rerolled now dependencies are in and fixed a whitespace issue - not introduced by the patch or the CS fixer but made more obvious.

I've asked @klausi for an official release of coder so we can say core complies with 8.x-2.7 coder rather than HEAD.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2665992-3-30.patch2.65 MB
29-30-interdiff.txt1.54 KB

Ahh the fixer was fixing something out-of-scope and the merge in #29 was not quite right.

Grepping the attached patch for additions results in the expected changes...
grep "^+" 2665992-3-30.patch | grep -v "^+++ b/"

+      '@message' => 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 62 and defined',
+      $fatal_error['@message'] = 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 62';
+    $expected_line = 59;
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Confirmed that the patch only changes what it should change plus the line number Alex mentioned. A couple of new files have popped up in core in the meantime (at least on 8.2.x for me), but let's get this in as first step and then fix the rest as follow up.

time phpcbf . --sniffs=Drupal.Commenting.FileComment --standard=Drupal --extensions=php

Took 7 minutes :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: +DrupalBCDays

Tagging.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2665992-3-30.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
file-comment-2665992-34.patch2.65 MB
interdiff.txt557 bytes

Rerolled.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

hiding obsolete files.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community

Tests passing, back to RTBC as my update was just a reroll.

alexpott’s picture

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

I've grepped @klausi's patch with pattern in #30.

Given this applies to both 8.1.x and 8.2.x I'm going to commit this now since this will allow the branches to diverge without having to roll this patch again and again and again.

Committed 1842e12 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed bfde6d3 on 8.2.x 
    Issue #2665992 by alexpott, klausi, heykarthikwithu, xjm, tstoeckler: @...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

And there was much rejoicing! :)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Rejoicing and a couple patch rerolls :) still have that weird empty space at the top of files left over from CVS but baby steps.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +8.1.0 release notes

Might be worth a footnote in the release notes for updated coding standards.

Status: Fixed » Closed (fixed)

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