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

Proposed resolution

Change the FileCommentSniff to handle this.

Remaining tasks

CommentFileSizeAuthor
#5 2665744.5.patch6.52 KBalexpott
#5 4-5-interdiff.txt3.88 KBalexpott
#4 2665744.4.patch2.63 KBalexpott
#4 2-4-interdiff.txt1.42 KBalexpott
#2 2665744.2.patch2.54 KBalexpott
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
Issue tags: +Needs tests
FileSize
2665744.2.patch2.54 KB
alexpott’s picture

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

One issue here is that apart from working out if the file is a class, interface or trait - how can we know if it is autoloadable? This sniff and fixer change will work great for Drupal 8 but previous versions of Drupal it won't work so good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2-4-interdiff.txt1.42 KB
2665744.4.patch2.63 KB

Ah of course - we can check for a namespace. This means core/lib/Drupal.php won't be caught be this though... so maybe we can just add something explicit for this because it is the only non namespaced autoloadable file in D8.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: -Needs tests
FileSize
4-5-interdiff.txt3.88 KB
2665744.5.patch6.52 KB

Here's tests

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work

Looks good, but tests are failing. ClassDeclarationUnitTest is one of the weird edge cases where we should not delete any comments with the fixer. It mixes different namespaces, so an @file docblock is totally relevant in such a case and should not trigger an error.

  • klausi committed 2e24160 on 8.x-2.x authored by alexpott 
    Issue #2665744 by alexpott, klausi: @file doc is not required for...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Fixed

Committed an updated version of the patch and fixed the test fails. This should be a good first step, please test the fixer with core. Let's open new issues for the problems that come up with it.

xjm’s picture

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

I tested the new sniff and it seems to be working well. I posted one followup issue:
#2676540: Fixer should not delete elaborate @file doc comments

@klausi, can you clarify about the test failures? Is that resolved or?

klausi’s picture

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

yes, as posted above I have fixed the phpunit test fails with the commit.

Thanks for the follow-up!

Status: Fixed » Closed (fixed)

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

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

Was the intention here to not require these, or to not allow them? The behavior seems to be the later, which is a heck of a thing to introduce in a point release. See https://travis-ci.org/jhedstrom/DrupalDriver/jobs/121744938#L360

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

Disregard #12, I'm fully in agreement with the change, and it's a simple enough fix. I just freaked out when I saw the wall of failures :)