Problem/Motivation
\Drupal
still has an @file
docblock. Our coding standards were updated to remove these with 8.1.x.
According to @alexpott, the reason our coding standards checks (as well as the original conversion) have not picked this up is that the class has no namespace and we haven't ever solved how to get the rule to work for that.
Proposed resolution
Remove the docblock manually.
Update ApI documentation, @file: Documenting files to:
"The @file doc block MUST be present for all PHP files, with two exceptions: 1) the Drupal class file, and 2) files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block. "
Remaining tasks
Review
Commit
Comment | File | Size | Author |
---|---|---|---|
#14 | 2829453-14.patch | 349 bytes | quietone |
does-not-contain-drupal.patch | 363 bytes | xjm | |
Comments
Comment #2
xjmComment #3
cilefen CreditAttribution: cilefen commentedHow did we miss that?
Comment #4
catchThis actually fails Drupal CS because the class has no namespace, so can't be committed as is.
Comment #5
xjmlol. So it's not just that the fixer did not fix it; it's that Coder actually fails when it's correct. I looked for a Coder issue and did not find one, just #2828946: Still giving me warnings about file doc comments?.
I guess this is postponed on making Coder support it, though it might not be worth the effort.
Comment #6
xjmComment #7
catchIt's not just that, because the class isn't namespaced, the coding standards say it must have a @file docblock. https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
So really to change it, we need to relax the @file requirements in the coding standards - i.e. include any class where there's one file per class?
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedI wonder if anything has changed. Rerolled the patch.
Comment #15
Kristen PolThanks for the reroll.
1) Tests pass now.
2) Per #7, there was a question of whether it really should be removed. According to the docs:
So, now that the tests pass, if we want to keep this change, the documentation needs to be updated to reflect this change, since it only refers to
namespaced class/interface/trait
. Here's a version with theDrupal class file
explicitly referenced:Comment #16
Kristen PolAdded discussion item on the doc:
https://www.drupal.org/node/1354/discuss#comment-14049761
Comment #17
quietone CreditAttribution: quietone as a volunteer commented@Kristen Pol, thanks yes the docs need to be updated.
Comment #18
jhodgdonCan we discuss the docs wording a bit? Here's my suggestion:
Comment #19
Kristen PolThanks @jhodgdon. Looks good to me. I'll mark this RTBC as the docs change can happen when this gets committed.
Comment #20
jhodgdonI'll go ahead and update the page now. I don't think it's a problem to do it first.
Comment #22
catchNice to see this can go in now.
Committed 1c1dbe0 and pushed to 9.2.x. Thanks!