Reasoning: In files just containing a single class/interface/trait and following PSR-0/4, the @file doc block adds absolutely no value – it's information can just as well be obtained from the file path or from the file name and the initial namespace statement (which, without the @file doc block, is the first line in the file anyways).

Proposed change: In the documentation section about the @file comment, change the following:

Drupal sample for files containing one class/interface:

/**
 * @file
 * Contains \Fully\Qualified\Namespace\And\NameOfTheClass.
 */

to this:

The @file doc block MUST be present for all PHP files, with one exception: 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.


Original report:

The following is relatively trivial, but worth remarking on.

The current Drupal API documentation standard for @file states:

@file: Documenting files

The @file tag is placed in a docblock to indicate it is documentation for the file as a whole.

If @file is missing Coder reports:

'ERROR | Missing file doc comment'

I appreciate the sense in "the file as a whole" for procedural files with possibly many functions.

But when one is using strictly OO development with strictly one Class or Interface per file, the above "the file as a whole" is not very useful if one is documenting the Class or Interface in the docblock header of the class anyway. Repeating the 1st line of Class header in the @file section breaks the Don't Repeat Yourself (DRY) principle and leads to maintenance problems (if the purpose or statement of purpose of a class changes it has to be changed in 2 spots).

[OLD: I find myself simply putting 'class XYZ' in the @file block thus:]

EDIT: I find myself simply putting 'NameOfClass' in the @file block thus:

/**
 * @file
 * BlockVisibilityKind
 */

namespace Drupal\ooe\Block;

/**
 * Encapsulates the possible visibility status options 
 * of blocks as referenced in hook_block_info().
 * 
 * UML: 
 * @link http://drupal7demo.webel.com.au/node/1151 BlockVisibilityKind @endlink
 * 
 * @author darrenkelly
 */
class BlockVisibilityKind {

  /**
  * 'Show on all pages except listed pages. 
  * 'pages' lists the paths where the block should not be shown.''
  */
  const NOTLISTED = BLOCK_VISIBILITY_NOTLISTED;

  /**
  * 'Show only on listed pages. 
   * 'pages' lists the paths where the block should be shown.'
  */
  const LISTED = BLOCK_VISIBILITY_LISTED;

  /**
   * 'Use custom PHP code to determine visibility. 
   * 'pages' gives the PHP code to use. '
   */
  const PHP = BLOCK_VISIBILITY_PHP;
}

When the API docs are generated I find this acceptable (it merely states that the file contains the named class, and the description of the class appears on the generated API page anyway).

Please relax the rule so that one does not need to have a redundant @file docblock if it is clear that the file is dedicated to a single Class or Interface.

Comments

klausi’s picture

Component: Review/Rules » Coder Sniffer
Status: Active » Closed (works as designed)

Again, this would require a change in our coding standards, which should be discussed in a core issue first.

klausi’s picture

Project: Coder » Drupal core
Version: 7.x-2.2 » 8.x-dev
Component: Coder Sniffer » base system
Category: Feature request » Task
Status: Closed (works as designed) » Active
Issue tags: +coding standards

Actually, I can just move this issue to core. You can do so with your other coding standard discussion issues, too.

webel’s picture

@klausi wrote:

I can just move this issue to core.

OK, but I am not sure that 8.x-dev is the right target. I want my changes to apply ultimately as well to the 7.x series of Coder (the coding standards docs do not seem to be constrained to particular Drupal version so clearly). It says at https://www.drupal.org/coding-standards#naming:

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version.

Ultimately, what matter to me is what Coder (per version) reports on.

webel’s picture

Issue summary: View changes
webel’s picture

Closely related. The Druoal coding standard currently says:

Drupal sample for files containing one class/interface:

/**
 * @file
 * Contains \Fully\Qualified\Namespace\And\NameOfTheClass.
 */

I don't find this adds any value, it is redundant, it is fragile against change, and it just makes for more work.

Firstly, the word 'Contains' does nothing, files do contain things, yes.

Putting the Namespace in there is fragile against change. Even IDEs like NetBeans and Eclipse with powerful refactoring don't catch it if the packaging is changed.

It also does not add anything of value to the generated docs, the API module extracts nicely the correct namespace already.

Putting it a 2nd time in text does not add any value, all it does is add another error-prone text based liability.

I would get rid of the entire @file requirement in the case where a file has one class or interface (but keep the requirement that the class or interface have a docblock header).

For now, all I am using is this (enough to keep Coder quiet without having to introduce fragile, inconvenient and redundant content):

/** 
 * @file
 * NameOfClassOrInterface
 */
dawehner’s picture

I don't find this adds any value, it is redundant, it is fragile against change, and it just makes for more work.

Note: This @file header is used on api.drupal.org, so you might should add similar kind of information on the api module.

jhodgdon’s picture

Project: Drupal core » Drupal Technical Working Group
Version: 8.0.x-dev »
Component: base system » Code

Coding standards changes are now part of the TWG

tizzo’s picture

Project: Drupal Technical Working Group » Coding Standards
jhodgdon’s picture

Component: Code » Coding Standards

I vote that we should do this. Under PSR-4, there is zero information being provided by that @file doc block, and they're often incorrect anyway.

The only reason we have them (and it's probably my fault) is that when we first started doing PSR-0 a long while back, we had no process for changing coding standards and so we had this standard saying "All files need @file" and we enforced it. It maybe kind of sort of made sense at the time, as we had a few class files and it was all new... Now, I think it makes no sense.

So we would want to amend our @file standards to say something like:
https://www.drupal.org/node/1354#file -- add this at the end:

However, files that contain exactly one 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.

dawehner’s picture

I would personally so much support that given that it adds nearly no value.
The namespace is always without the @file doc the first part of the file anyway.

dawehner’s picture

Status: Active » Reviewed & tested by the community
Issue tags: +needs announcement for final discussion

Let's follow the super effective workflow.

drunken monkey’s picture

Issue summary: View changes

Before announcing a final discussion I think we should have a clear proposal for the change (with exact wording) in the issue summary.

I've now added a proposal there, please discuss or edit as appropriate. (I don't think just adding a paragraph, as Jennifer proposes in #9, is the right way, since then we'd have an almost completely inapplicable example for how to document classes there.)
(Btw, are we now using RFC 2119 or not?)

dawehner’s picture

IMHO we should swap the logic around, the common case is already that a file contains one class/interface/trait only, so what about doing the following:

The @file doc block SHOULD NOT have a @file documentation block for PHP files following the PSR-4 or similar standard, so files which contain exactly one class/interface/trait.
Wim Leers’s picture

Dear lord, yes please! Useless boilerplate that does more harm (extra work, and wrong copy/pasting when outdated) than good.

klausi’s picture

@dawehner: your sentence does not make fully sense, a file block cannot have a file block. But I agree, the sentence can be simpler.

dawehner’s picture

Oh yeah you are absolutely right. let's change it to:

The file SHOULD NOT have a @file documentation block for PHP files following the PSR-4 or similar standard, so files which contain exactly one class/interface/trait.
xjm’s picture

Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs review

The last time we discussed this, in a previous issue, we decided to retain the file docblock since it has a handy copy-pastable line with the fully namespaced class (among other standards), but made some changes to reduce redundancy between that and the class docblock. The original issue report for this issue did not take into account that recommended format.

Edit: NB: I am neither for nor against this proposal; I just wanted to make sure the change wasn't rushed through without taking into account the past discussion. It'd be good to find and review the previous issue.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Edit: NB: I am neither for nor against this proposal; I just wanted to make sure the change wasn't rushed through without taking into account the past discussion. It'd be good to find and review the previous issue.

Well, there has been days where IDEs didn't had a feature to copy those references, these days are gone, so yes at least myself took that into account.

pfrenssen’s picture

Just out of interest and to save me some keystrokes: how exactly can you copy the fully namespaced version of the class in modern IDE's like PHPStorm?

I'm +1 for this change BTW, the convenience of being able to copy/paste it doesn't outweigh the fact that it is redundant information.

tim.plunkett’s picture

command-shift-option-c works in my default Mac PHPStorm binding. You have to be on the class name itself. If you're on a method name, it appends :: and that.

dawehner’s picture

Just out of interest and to save me some keystrokes: how exactly can you copy the fully namespaced version of the class in modern IDE's like PHPStorm?

Look out for the copy reference command.

tizzo’s picture

The coding standards committee believe this issue is ready for final discussion and are announcing and tagging it appropriately. It is worth noting that while the language of this issue’s title suggests relaxing the policy to make the docblock optional the wording of the proposal makes removal mandatory. This needs further consensus before ratification.

ksenzee’s picture

I strongly support this proposal. Every time I see the @file block in one of these files I want to facepalm. And yes, let's make removal mandatory.

chx’s picture

I would relax and remove the "exactly one". Why? Because test classes which contain a second class but those are only there because we are pre-PHP7 and don't have anonymous classes. Otherwise fully agreed.

mglaman’s picture

While working on porting contrib and matching coding standards, it feels very redundant to have to write out the fully qualified class name in the file block. All for this change.

Crell’s picture

The wording is a little odd; the "one exception" applies to the majority of our codebase now. :-) But +100 on dropping that docblock for class files, Amen and pass the sweet potatoes!

I also agree with the caveat chx notes in #24. Perhaps call that out specifically?

pfrenssen’s picture

Issue summary: View changes

Well the way I read it the statement is still valid:

files that contain exactly one namespaced class/interface/trait

Even if you have a dozen anonymous classes, then you might still have exactly one namespaced class :D

Joking aside, I agree with #24 and @26 that the wording is too strong, having multiple classes in namespaced code is very uncommon, stressing 'exactly one' just puts a lot of emphasis on it, it would make everyone reading this pause for a moment to imagine use cases for multiple classes.

Updated the wording s/exactly one/a/.

xjm’s picture

It might be worth clarifying in the proposal that this is for files that contain only one class, interface, or trait? Edit: Sorry, I fail at reading; this is somewhat the previous couple comments were about. Or what would the standard be for a file that uses a namespace { } or declares another class, for example a phpunit test that declares a helper class after the test in the same file?

As an aside, if this standard is adopted, updating core to be compliant would be a massive patch since all the hundreds of classes in core already follow the current standard. Per the https://www.drupal.org/core/d8-allowed-changes#minor we should update core for new coding standards in minor versions. The ideal time to apply it to core would be during the release candidate phase of 8.1.x, which begins March 16. Edit: Also, if it is adopted, let's pretty please update coder before it's applied to core.

xjm’s picture

Oh and as I mentioned, these standards were previously discussed in earlier issues:

It'd be good to document why those lengthy previous discussions are being overturned, that is, why their conclusions were wrong or no longer relevant.

Another aside: from past-xjm in #1487760-88: [policy, no patch] Decide on documentation standards for namespaced items, November 2012:

Furthermore, it's already in 1500 classes in core. Can we just stop changing that line already? (It was changed from "Definition of" to "Contains" four months ago for no compelling reason and only about 10% of classes are correct there at this point.)

;)

pfrenssen’s picture

I think the main reason is that back in 2012 when this was first discussed we were still at the start of our OOP journey. Currently we have a collective experience of writing almost 6000 classes and we've come to the insight that this docblock is just meaningless boilerplate which doesn't add any value, but does cost time to write, review and maintain.

alexpott’s picture

Can we get issues created for:

  1. coder which includes a sniff to find unnecessary @file docblocks and the ability to fix it.
  2. Applying the fix and the coder rule to Drupal 8 core so at least core obeys our own rules.
klausi’s picture

Yes, we should do that once this is fixed and the coding standards document was updated.

alexpott’s picture

@klausi I was wondering if we should try to synchronise #31.1 and acceptance of this issue then we can ensure that the sniff and fix can be created and we can also ensure that #31.2 won't take long to implement. I think shortening the disconnect between a new standard and core following it 100% is paramount for new coding standards actaully becoming a standard.

xjm’s picture

I also think a coder issue containing the exact formulation of the new rule makes sense to file alongside this issue, because it illustrates the standard we would adopt. Then if the standard is adopted, we can immediately commit the patch to coder and add it to the blacklist in the core phpcs.xml.dist file. As stated in #28, we would patch core during the minor RC phase.

Edit: In keeping with:

All changes that can be tested with coder/phpcs are advised to come with a patch on the the coding-standards project issue (to avoid a flood of patches for not-yet-ratified standards to the coder queue)

:)

tizzo’s picture

Title: Please relax requirement for @file when using OO Class or Interface per file: 'ERROR | Missing file doc comment' » Relax requirement for @file when using OO Class or Interface per file
Project: Coding Standards » Drupal core
Version: » 8.1.x-dev
Component: Coding Standards » documentation
Issue tags: -final discussion +Approved Coding Standard, +Needs Documentation

The Coding Standards Committee agrees that the community has reached sufficient consensus on this issue and is tagging the issue appropriately. The following update to the coding standards is being recommended and is being moved to the Drupal Core queue for core committer approval per step 7 in the coding standards process. We leave it to the core and coder module committers to coordinate the implementation.

The @file doc block MUST be present for all PHP files, with one exception: 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.

xjm’s picture

Thanks @tizzo! As a Drupal 8 core release manager, I am okay with this change, provided a coder rule is implemented to codify it and a patch provided to add it to the core exclusion list. That way, we can make Drupal 8 core compliant in the next minor release. So leaving untagged for now pending that.

Edit: I also double-checked that @jhodgdon, the Documentation Topic Coordinator, also already commented in favor of this change. Thanks @jhodgdon.

catch’s picture

+1 from me. Do we just need to open the issues against coder + the follow-up to make core comply to mark this fixed?

jhodgdon’s picture

+1 from Docs maintainer too.

alexpott’s picture

alexpott’s picture

alexpott’s picture

So looking at #2665992: @file is not required for classes, interfaces and traits and #2665744: @file is not required for classes, interfaces and traits I think we should also say that autoloaded classes must begin

<?php

namespace Foo/Bar;
alexpott’s picture

Also we need to get core passing the current filedoc rule... we close but we have a problem with things in ./core/scripts - see #2572643: Fix 'Drupal.Commenting.FileComment' coding standard

jhodgdon’s picture

Project: Drupal core » Coding Standards
Version: 8.1.x-dev »
Component: documentation » Coding Standards

Uh, wait... So I think this issue was moved to the Core queue to get committer approval, and the standard has not been formally adopted and documented yet.

So I think we should move this issue back to the Coding Standards queue and say it is approved by the core committers, to trigger off the next part of the process, and to leave the issue there for posterity?

drunken monkey’s picture

I guess someone with the necessary permissions also now needs to actually edit the documentation to reflect this change? Then, from what I understand, we can mark this as "Fixed" and continue work just in the Coder and Core issues.

In any case, thanks to everyone in this issue! Great to finally see coding standards changes happening again.

xjm’s picture

@drunken monkey, Thanks! This issue needs committer signoff before we update the docs, and @alexpott and I have said we'll sign off once the coder rule is settled (but not before since as expected it turns out to need some refining).

xjm’s picture

#2665744: @file is not required for classes, interfaces and traits is committed now. I tested locally and this sniff behaved as expected. The fixer is a little overagressive, so I filed #2676540: Fixer should not delete elaborate @file doc comments. #2665992: @file is not required for classes, interfaces and traits is the issue to fix this in core, which also has a few blockers, but I think it's okay to move forward now.

We will make core compliant with this standard during the RC phase of 8.1.x which starts April 6.

Thanks especially to @alexpott for getting this unblocked.

dawehner’s picture

Its so great to see this!

klausi’s picture

Status: Reviewed & tested by the community » Fixed

* Changed the coding standards page as proposed in the issue summary https://www.drupal.org/coding-standards/docs#file
* Released Coder with a sniff to complain and fix @file blocks in class files
* Removed all the @file doc blocks in Drupal core at #2665992: @file is not required for classes, interfaces and traits

I guess we are done here?

jhodgdon’s picture

klausi++

pfrenssen’s picture

Great, thanks!

Wim Leers’s picture

Hurray! :) And there was much rejoicing! Best of all: #2665992: @file is not required for classes, interfaces and traits already landed too, so Drupal 8.2 effectively has gotten rid of this already!

Status: Fixed » Closed (fixed)

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

joachim’s picture

The downside of losing this is that there's no longer the fully qualified class name anywhere. It's quite handy for copy-pasting to an import statement somewhere else.

ksenzee’s picture

An IDE can do that for you quite easily (in PhpStorm, right click on the class name, and the first option is "Copy Reference"). I think we're past the point of optimizing for people coding in plain text editors, for good or ill.