Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- 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.
- Run
phpcbf . --sniffs=Drupal.Commenting.FileComment --standard=Drupal --extensions=php
from the core directory
Remaining tasks
commit the patch.
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff.txt | 557 bytes | klausi |
#34 | file-comment-2665992-34.patch | 2.65 MB | klausi |
Comments
Comment #2
alexpottLooks 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.
Comment #3
tstoecklerSeems that we need to add those @file-docblocks with proper descriptions first.
Comment #5
alexpott@tstoeckler yep this was just an exploratory patch - it won't be committed probably until 8.1.x RC
Comment #6
tstoecklerAhh, that's good to know, thanks.
Comment #7
alexpott@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.
Comment #8
xjmThe 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.
Comment #9
xjmPosted #2676816: Consolidate miscellaneous docs from class @file docblocks into the class docblocks. Still need a separate issue for the auto-generated proxy classes.
Comment #10
xjmRegarding #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.Comment #11
xjmPosted #2676822: Add @file docblocks where they are missing for non-classes, interfaces, or traits for #10.
Comment #12
xjmI totally missed that #2572643: Fix 'Drupal.Commenting.FileComment' coding standard was already committed to address that; closing as duplicate. Sorry.
Comment #13
xjmPosted #2676836: Update proxy class generator for new @file standard.
Comment #15
xjmWe 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).
Comment #16
xjmComment #17
heykarthikwithupatch did not apply, needs reroll.
Comment #18
heykarthikwithuCovers
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.
Comment #19
alexpott@heykarthikwithu the point of this issue to automate using the coder rule. This is not something that should be attempted by hand.
Comment #20
alexpottComment #21
alexpottSo 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]"
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.
Comment #23
alexpottCreated #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 standardComment #24
alexpottCreated #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
Comment #25
alexpottTagging as this is part of #2665992: @file is not required for classes, interfaces and traits
Comment #26
alexpottThe 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.
Comment #28
alexpottSo some of the error tests are dependency on line numbers... see interdiff.
Comment #29
alexpottRerolled 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.
Comment #30
alexpottAhh 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/"
Comment #31
klausiConfirmed 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.
Took 7 minutes :-)
Comment #32
klausiTagging.
Comment #34
klausiRerolled.
Comment #35
klausihiding obsolete files.
Comment #36
klausiTests passing, back to RTBC as my update was just a reroll.
Comment #37
alexpottI'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!
Comment #39
Wim LeersAnd there was much rejoicing! :)
Comment #40
neclimdulRejoicing and a couple patch rerolls :) still have that weird empty space at the top of files left over from CVS but baby steps.
Comment #41
xjmMight be worth a footnote in the release notes for updated coding standards.