Problem/Motivation

For PSR-0 class files (where a PHP file contains just a single class definition), it seems a bit redundant to have an @file documentation line that is pretty much the same as the class definition. So, we need a standard for a better way to document such files.

Proposed resolution

Proposed by sun in comment #38 -- our new standard would use the following documentation for PSR-0 class/interface files at the top:

/**
 * @file
 * Contains \Drupal\Core\Config\StorageInterface.
 */

Then the file would continue with the rest of the documentation for the class or interface.

Remaining tasks

1. Agree upon a standard [done].

2. Update the coding standards page http://drupal.org/node/1354 with this standard. [done]

3. Core patch to update core files to use this standard.
This is the Novice task. What needs to be done:

a) Identify class files (files with a .php extension, in the Drupal namespace -- not vendor files) that have incorrect file doc blocks. This can be done by looking at api.drupal.org (as described in comment #72) or by using a script.

The correct file doc block should say something like:

/**
 * @file
 * Contains \Drupal\Core\Config\StorageInterface.
 */

In this case, Drupal\Core\Config would be the namespace from the "namespace" line in the class file (a few lines down), and StorageInterface would be the name of the class (file name without the .php extension).

The problems could be:
- No @file doc block at all.
- Doc block line doesn't start with the word "Contains" and end with .
- Class name or namespace is incorrect or missing.
- Namespace doesn't start with \.

b) Make a patch to fix the problems.

User interface changes

None.

API changes

New standard for API documentation of namespaced files to replace the language for "files containing just one class" on http://drupal.org/node/1354#files

Original report by catch

See discussion from http://drupal.org/node/1323120#comment-5424112 down.

Also related to #1323082: Establish standards for namespace usage [no patch] and others.

Since PSR-4 is class per file, the @file docblock is a bit redundant, but our coding standard currently specifiy that every single file should have one. The reason given for this is http://api.drupal.org/api/drupal/files. However it also duplicates the docblock for the class itself, so seems a bit redundant.

Files: 
CommentFileSizeAuthor
#116 drupal-file-documentation-1392754-116.patch700.51 KBjustAChris
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,215 pass(es). View
#115 drupal-file-documentation-1392754-115.patch699.98 KBjustAChris
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,129 pass(es). View
#113 interdiff-1392754-109-113.txt4.04 KBjustAChris
#113 drupal-file-documentation-1392754-113.patch701.46 KBjustAChris
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,103 pass(es). View
#110 interdiff-1392754-109-110.txt3.04 KBjustAChris
#110 drupal-file-documentation-1392754-110.patch700.46 KBjustAChris
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,028 pass(es). View
#109 drupal-file-documentation-1392754-109-do-not-test.patch698.92 KBjustAChris
#104 drupal-file-documentation-1392754-104.patch701.23 KBtss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,841 pass(es). View
#99 drupal-file-documentation-1392754-99.patch700.52 KBtss
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-documentation-1392754-99.patch. Unable to apply patch. See the log in the details link for more information. View
#94 drupal-file-documentation-1392754-94.patch710.24 KBtss
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-documentation-1392754-94.patch. Unable to apply patch. See the log in the details link for more information. View
#89 drupal-file-documentation-1392754-89.patch710.42 KBtss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,081 pass(es). View
#87 drupal-file-documentation-1392754-87.patch708.94 KBtss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,050 pass(es). View
#78 drupal-file-documentation-1392754-78.patch654.68 KBtss
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,679 pass(es). View
#80 filesNeedUpdate.txt47.02 KBjustAChris
#80 docstandards.txt4.23 KBjustAChris

Comments

Crell’s picture

With PSR-0, we're going to end up with quite a bit more files. In most of those cases, I cannot think of what I'd even put in a @file declaration that isn't a direct copy and paste from the class's docblock.

I see three options:

1) Tell developers "well just copy and paste it, and remember to update both."

2) Since api.module is our own custom parser, adjust it to auto-detect a PSR-0-ish file and fill in some default text, like "contains the FooBar class".

3) Since api.module is our own custom parser, adjust it to auto-detect a PSR-0-ish file and fill in the first line of the class description. Basically automating option 1.

Personally I like option 3, but that would be work for drumm and I hate to give him more work without him chiming in here. :-)

catch’s picture

I'd been pondering something like #3, so if we can persuade Drumm that sounds like a good option. It may also work for non-Drupal code if it has a docblock for the class.

webchick’s picture

Issue tags: +coding standards

Tagging so the right people see this.

bojanz’s picture

#1 would be redundant. So definitely #2 or #3.

jhodgdon’s picture

You can also persuade jhodgdon that #3 is a good example (I'm co-maintaining API module now). Especially if accompanied by a patch for the API module that does it. What is PSR-0?

We would also need to put this in http://drupal.org/node/1354 (doxygen standards) as a special case.

webchick’s picture

Component: base system » documentation
jhodgdon’s picture

Component: documentation » base system

As a note, I would think the @file would be something like:

@file
Definition of the XYZSomething class.

Not a cut/paste of the first line of the class itself, which is hopefully not that...

jhodgdon’s picture

Component: base system » documentation

oops, cross-post. :)

Crell’s picture

PSR-0 is the namespace and file organization standard that we have adopted, as have most other PHP projects. The key point here is that it mandates one-class-per-file, even if the class is small, so The @file line isn't going to really tell you anything useful beyond what's already in the class's docblock.

If we want the @file to just be that string all the time, couldn't that be automated? (That's essentially option 2 from #1.)

webchick’s picture

#2 and #3 are Drupal-specific magic. Most developers are going to be concerned with what's in their IDE. It's for this reason I'd prefer us to be consistent and just use @file everywhere. Because otherwise, documenting this rule goes like:

"Use @file declarations at the top of all files, unless they are a PSR-0 format class file. What's PSR-0? Well it's a common standard for splitting up object-oriented PHP code so that it autoloads properly. How do you know if you're in a PSR-0 file? Well, if it's a file with a name like FooBarBaz.php that's in /includes/something and it contains just a single class. Or it could also maybe be in a module folder. Somewhere."

Seriously, how do you propose to document this?

jhodgdon’s picture

I'm happy to leave the standard saying we should have an @file for each file. Is it a lot of trouble to add one that says:

/**
 * Definition of the FooBarBaz class.
 */
jhodgdon’s picture

which I might add, seems pretty easy to maintain...

webchick’s picture

#11 sounds good to me, too.

bojanz’s picture

#11 sounds okay.

I'd also like to point out that #10 makes 1) a bit more complicated than it is. By the time D8 ships, we'll want every class to be PSR0 loaded (and having a class per file is already common in many parts of contrib), so it would be enough to say "if this file has a class, no need for a @file docblock".

webchick’s picture

Yes, we could simplify to "If the file contains only _one_ class, don't do @file". Maybe. But that's still not accurate, because a .test file, for example, could only have one class and is not PSR-0, and should still have @file.

So consistency FTW, imo. :)

catch’s picture

All test files should eventually have one class and be PSR-0 though, we don't want to keep a separate autoloader around just for tests.

Crell’s picture

Amen to #16. I'll be happy if we can take the registry out back and shoot it.

So I can live with either:

1) If a file has only one class, @file is optional.
or:
2) If a file has only one class, the @file should simply be "Definition of $classname".

#1 is slightly less work for me, but I won't make a fuss either way.

webchick’s picture

Title: @file for PSR-0 classes, or not » @file for PSR-0 classes, or not (no patch)
Status: Active » Reviewed & tested by the community

Let's go with #2 then, cos that matches our existing standards, doesn't require work to API module, and doesn't require explanation, really. Cool?

jhodgdon’s picture

Do you want me to add a note to to 1354 about that?

webchick’s picture

Yeah, sounds good.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

http://drupal.org/node/1354#files

Seems like the standard has been defined, agreed upon, and recorded. Marking this issue fixed, since I think this issue was just about defining the standard?

Status: Fixed » Closed (fixed)

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

merlinofchaos’s picture

Status: Closed (fixed) » Active

I want to re-open this just to make sure one item gets discussed:

The use of "Definition of" is inconsistent with our class/function/method documentation where we require a third person verb.

It seems like it really should be "Defines" which is shorter (yay) but also more consistent with the rest of our standards. I bring this up because I actually got caught on this trying to write documentation to our standards and little inconsistencies like this are annoying when our standards are as extensive as they are.

jhodgdon’s picture

Hmmm... It's true that our standards for functions and classes specify 3rd-person verbs. But our standards for files don't even say they need to start with a verb:
http://drupal.org/node/1354#files

merlinofchaos’s picture

No, but they do say that they can, and if they do they should be of the 3rd person variety.

Which means that it's really just a semantic difference, and there's two things we could be consistent with:

1) Many @file directives don't start with a verb. We could be consistent with that. (Except that some do).

Or:

2) We already use "Defines..." a lot in other places, because classes and functions must start with a verb.

jhodgdon’s picture

So... What exactly are you advocating we should change on
http://drupal.org/node/1354#files

neclimdul’s picture

/**
 * @file
 * Definition of NameOfTheClass.
 */

should be

/**
 * @file
 * Defines NameOfTheClass.
 */

or better yet the de-facto standard seems to actually be

/**
 * @file
 * Defines My\Namespace\NameOfTheClass.
 */
jhodgdon’s picture

RE #27 - the "de-facto standard" seems to currently be "Definition of ...". I don't see a single instance of "Defines Drupal\" in core currently. They are all "Definition of Drupal\".

Crell’s picture

Yes, it looks like merlin is suggesting we change that, like we changed "Implementation of hook_foo" to "Implements hook_foo" for Drupal 7.

FTR I have no strong preference either way. I can see the value of the consistency Earl is suggesting, but I don't care enough either way to push for either one. Just let me know what I should do. :-)

neclimdul’s picture

No, the de-facto standard is put the namespace in the comment. The change is to use Defines instead of Definition of. The alternative I see is to convert Implements back to Implementation of to be consistent with class definitions which I don't see happening :)

jhodgdon’s picture

RE #30 - we are talking about apples (file documentation headers, which tell what a file contains) vs. oranges (function documentation headers, which tell what a function does). In the function case, we use verbs (with very few exceptions). In the file case, it's not so clear to me that verbs are the best thing in all cases.

I had some trouble finding other existing standards for what to put in file headers, but I found these:
http://www.cs.cmu.edu/~410/doc/doxygen.html (from a Carnegie Mellon CS class -- they use 3rd person verbs for functions, and non-verbs for files)
https://sites.google.com/site/zoidbergengine/documentation/code-standards (uses descriptive text for files, not verbs)
http://www.slac.stanford.edu/exp/glast/wb/prod/pages/advanced_doxygen/us... (uses "Declaration of..." in .h header files, and "Definition of..." in .c files for the file description)

If anyone can find cases where people are advocating using verbs to start file descriptions, I'm open to the idea, but I don't think that practice is generally followed in the industry.

neclimdul’s picture

Let me just start with, I'm not sure what the right solution actually is. I actually don't really care either I just want to make sure if I'm going to be using a strict standard it makes sense to someone and we thought about it before I train myself to type it. :)

Those are some useful links but I think the examples given are a bit hard to compare and I took something different from them. They all are descriptive descriptions of what's contained in the file and therefore not verbs. This is in line with the @file line from other files that don't contain a class. We have chosen in class files though to just regurgitate the class and namespace(I actually think is a bit weird). The sentence we've chosen is also very action oriented and not descriptive which is I think what lead to the correlation drawn in the OP.

I also think we're more explicit in our documentation standards then the industry possibly to a fault. That is to say, my experience elsewhere has been "be descriptive" and that's all that matters. I think this is going to make it rather difficult to compare ourselves to the industry.

Between the two options, my gut feeling is there's not really a identifiable reason for the inconsistency and since we're so explicit and tied to our consistency, we should be consistent. But if there is a reason then ok lets keep it the way it is.

jhodgdon’s picture

Actually, the last link (http://www.slac.stanford.edu/exp/glast/wb/prod/pages/advanced_doxygen/us...) is exactly analagous to our situation. This is for C++ classes, where there is a declaration (.h) file and a definition file, and they use "Declaration/Definition of [classname]" as the brief file description.

Which is exactly the same as our situation, aside from the language (C++ vs. PHP), and so our current standard
Definition of [classname].
would stand as it is.

Again, I do not think there is any reason to change the standard that we have. It seems to be consistent with the general idea of "describe what is in a file", and with the few other standards I was able to find for the style of the one-line file description comment. Functions "do" something. Files "contain" something. Verbs are appropriate for "doing" things. Descriptive text is appropriate for describing what something contains.

webchick’s picture

Yeah, I think the distinction is there is that functions imply verbs; files are nouns. So describing the function in "actiony" words makes sense, vs. describing files in "descriptivy" words makes sense.

Or at least maybe that's why I never found this difference awkward.

merlinofchaos’s picture

Okay, there are some flaws with the logic.

1) The system we aren't using isn't really active. All we are doing is implying a subject. If we were using active "Do something" then we would have a standard that says "Do something." In fact, that is explicitly forbidden. Our standard is "Does something".

2) The reason we do this is that there is basically an implied [This class] or [This function] does something.

3) Classes are closer to files than functions in how active they are, anyway, and we explicitly still use the third person implied subject. But classes aren't any more active than files; they contain properties and methods.

Note: I think the only reason I'm arguing this is that our standards have become kind of strict lately as neclimdul said, if we're going to be that strict we should be thoughtful about it. I, personally, am struggling with the coding standards because there's so much strictness that's new and watching xjm write really long "This is wrong this is wrong this is wrong" reviews is hiliting, to me, what feel like inconsistencies in the standard that impedes my ability to do stuff because I have to memorize a document that is, in fact, too big for me to memorize.

neclimdul’s picture

Just to summarize my arguments for the change:

  1. "Definition of" still sounds actiony to me we're using a different form. Its also not a full sentence which is an exception to our full sentence rule.
  2. I think the argument that somehow a file doesn't do actiony things is incorrect. The are actually executed where classes actually statically define something and don't explicitly perform any actions. The methods on the class are where all the action happens.
  3. I think we should encourage a better file descriptions rather then repeating information already contained in the file.
  4. I think consistency is more valuable then some questionable reasoning about how functions, classes, and files differ in what they do.

However since there's a general sense of "I don't care to change it" and "it makes sense to me" overwhelming the comments I guess we just keep it?

xjm’s picture

I think "Defines" might be a little more consistent, but I don't have a strong opinion. :)

sun’s picture

I somewhat agree with the noun vs. verb argument. But I also agree with the argument that it is not really consistent. :)

And while I agree with the consistency argument, I'd find two "Defines ..." a bit confusing. I could see developers getting easily confused on as to where to document the class/interface (which should happen on the class' phpDoc block, not the file's).

Perhaps we just need a different verb?


/**
 * @file
 * Contains Drupal\Core\Config\StorageInterface.
 */

namespace Drupal\Core\Config;

/**
 * Defines the shared interface for all configuration storage controllers.
 */
interface StorageInterface {
}

?


Also, FWIW, I originally was rather against the @file docblock in PSR-0 files - but meanwhile, having that readily at hand turned out to have a practical benefit of being able to copy + paste the entire PSR-0 class name (e.g., for running tests on the command line). So thanks + hooray for adding it! :-D

jhodgdon’s picture

I think the proposal in #38 is good. Thanks for thinking of it!

xjm’s picture

Status: Active » Needs review

#38 is fine by me.

tim.plunkett’s picture

Another +1 to #38

neclimdul’s picture

Its better for sure. +1

merlinofchaos’s picture

Also happy with #38.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Ok, sounds like consensus.

jhodgdon’s picture

Title: @file for PSR-0 classes, or not (no patch) » [policy - then patch] Decide on documentation standards for @file for PSR-0 class file

Consensus++!!

I will update the issue summary and then leave this for a few days at RTBC, in case there are further comments, then update the coding standards page. Then we can work on a patch for 8.x core.

jhodgdon’s picture

(summary is updated)

Crell’s picture

My concern is that I don't know if "Defines" will make sense for all classes. It should for interfaces, but not necessarily for classes. And a common prefix of "Defines an object that..." for every class seems excessive.

I'm totally cool with "Contains" for the @file declaration, though.

jhodgdon’s picture

RE #47 -- Our standard for classes is usually something like "Represents a ...".
http://drupal.org/node/1354#classes

tim.plunkett’s picture

How about

@file -> Contains
interface -> Defines
class -> Represents

Or, just codify @file -> Contains, and have the rest be the standard pattern. The issue title only mentions @file explicitly

jhodgdon’s picture

We already have at least the suggestion of interface/class at http://drupal.org/node/1354#classes

So yes, let's just plan to add the @file part to
http://drupal.org/node/1354#files
replacing what is currently there for "files defining one class".

neclimdul’s picture

How about the class doc we just suggest the tense and let the developer describe it the same way we do with functions. I actually assumed that was what we'd agreed upon.

Crell’s picture

+1 to #50.

Crell’s picture

Issue summary: View changes

update with latest proposal

jhodgdon’s picture

I have just edited the issue summary to make it clear the only place we should change on node/1354 is the @file section (#50). Everyone OK with that clarification? Based on the past few comments, Crell, tim.plunkett, myself, and I think neclimdul are +1 on that. Any dissents?

sun’s picture

I wanted to leave more or less exactly the same comment as @neclimdul -- for classes/interfaces, the standards can only rule the tense, but not the content.

I'd mark this RTBC, but it is already, so I think it's ready for execution. :)

Crell’s picture

No dissent. Let's do.

jhodgdon’s picture

OK, this "Contains" idea has been around long enough, with no dissent, that I think we can consider it adopted. So I've just edited
http://drupal.org/node/1354#files
(very small edit: "Definition of " --> "Contains" -- http://drupal.org/node/1354/revisions/view/2109666/2204862).

Note that since we are working on a standard for namespaces in documentation on #1487760: [policy, no patch] Decide on documentation standards for namespaced items, I specifically did not put the namespace into the example. When that issue is resolved, we can update the example.

So, the next thing we would need is a patch. Should we make that a separate issue or do it here?

jhodgdon’s picture

Title: [policy - then patch] Decide on documentation standards for @file for PSR-0 class file » Comply with new documentation standards for @file for PSR-0 class files
Status: Reviewed & tested by the community » Active

I guess we can just do the patch here? Should be a fairly simple search-and-replace...

jhodgdon’s picture

Issue summary: View changes

updated proposal

tim.plunkett’s picture

Since rolling this patch is trivial, can we postpone that on #1627350-29: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName)? That will not be as easy to reroll...

msonnabaum’s picture

Just wanted to throw my 2 cents in here since no one seems to be arguing against this.

Adding @file to PSR-0 classes is insane.

The @file comment tells me that a class exists in a file. This isn't useful, because PSR-0 already tells me what class is in the file based on how the file is named. PSR-0 also ensures that only one definition of a class can appear in that file, so I can't see how there could possibly be an edge case where an @file comment would be at all useful or informative.

Standards are great and all, but I think it's important to make exceptions when our standards clearly make no sense.

xjm’s picture

If the API module can still parse and document the files, #59 seems fine to me. PSR-0 already has its own simple rules that folks will need to understand anyway.

jhodgdon’s picture

The API module will not display any documentation for a file if there is no @file, so I am against #59. It will leave lots of blank spaces on pages like
http://api.drupal.org/api/drupal/files/8

I think we should stick with the standard that everyone had agreed to on July 6th at this point...

msonnabaum’s picture

Surely we could fix the API module? It doesn't seem sensible to me that we'd make a decision about how we document our code based on what api module is capable of at any given time.

jhodgdon’s picture

How do you propose fixing the API module exactly? I am not sure what logic it could use to tell that something is a PSR-0 class file.

So... No one objected to this until today, and this issue has been around since January. Is it really so difficult to add a few lines to the top of the file, so that we have a consistent standard saying "all files need an @file"?

chx’s picture

You can give a good guess by looking at the namespace and if the last two steps are the directories and the filename matches then that's it.

See core/vendor/doctrine/common/lib/Doctrine/Common/EventArgs.php -- the class is EventArgs, the namespace is Doctrine\Common.

webchick’s picture

I too prefer "All files need a @file." It's much easier to document and understand, and in addition to API module other IDEs and stuff parse it as well and they don't special-case PSR-0. Feel free to follow whatever conventions you want in your own contributed modules though.

msonnabaum’s picture

I wouldn't lump in other IDEs with api module. I'd be surprised if we could find a single php project that uses @file for any type of class-per-file layout.

This is a Drupalism and it doesn't make sense.

msonnabaum’s picture

Just to be clear, I'm not trying to block this issue, I'm just pointing out that it makes no sense and the only argument for doing it seems to be that we don't want to bother with changing api module.

If that's the case and we still want to do this, then I won't fight it.

Crell’s picture

I think the other argument is fewer conditionals in our coding standards. (I agree that and api.module are both relatively minor issues, but I don't greatly care one way or another.)

jhodgdon’s picture

So. We decided on this coding/docs standard a while back... Who actually now is in favor of changing it? Can I see a show of hands? If there is not a groundswell, I would like to propose leaving it as it is. We're talking about adding roughly 3 lines of documentation at the top of each file...

jhodgdon’s picture

Issue summary: View changes

update status

Crell’s picture

Title: Comply with new documentation standards for @file for PSR-0 class files » Comply with new documentation standards for @file for namespaced class files
Assigned: Unassigned » jhodgdon

3 years later, I think we ended up going with the manual approach. Can we close this? I vote yes.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

Well... We could use this issue to fix the remaining files that still do not comply with our docs standards, since the issue title is "Comply with standards".

If you go to api.drupal.org for Drupal 8, click on Files, and filter to Location Contains .php , Namespace contains Drupal (so that Vendor files are excluded), you'll see that there are quite a few there that say "Definition of" instead of "Contains", at least one that just says "Contains" without the class name on the first page, and a few where the right column doesn't turn into a link (indicating the class name isn't correct).

It seems like it wouldn't be too hard to identify and fix these with a script, on this issue?

Crell’s picture

Issue summary: View changes

Script or a Novice tag, yes. Sounds good to me.

jhodgdon’s picture

Issue summary: View changes
Issue tags: +Novice

OK, let's put on the Novice tag and see what happens. The task is at least pretty straightforward and clear. I'll add some notes to the issue summary.

tss’s picture

I am currently working on this.

justAChris’s picture

I'd started a bash script to check the docblock summaries, so can help review any patch that comes about.

tss’s picture

Just as note: Traits does not become links on the api documentation pages, even though they follow the standard.

tss’s picture

FileSize
654.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,679 pass(es). View

I'm done updating the code. Here's what I did (for reference)

  • Cloned Drupal core and switched to branch 8.0.x.
  • Went to https://api.drupal.org/api/drupal/files/8 and filtered by Location=.php and Namespace=Drupal
  • From here I went through all the pages and looked for:
    • Missing links in fourth column.
    • Missing text in fourth column.
    • Any other text than "Contains \[namespace]\[classname]".
  • Then I grep'ed for files still containing Definition of and Contains of still in the Drupal namespace, which showed an additional 200+ files.

Additional notes

1) Some files did not get parsed correctly (i.e. links are missing in the fourth columns on the API documentation site.)

These might have failed because of the { on the namespace line:

  • core/modules/aggregator/tests/src/Unit/Plugin/AggregatorPluginSettingsBaseTest.php
  • core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
  • core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
  • core/lib/Drupal/Core/Entity/Query/ConditionInterface.php
  • core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
  • core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
  • core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
  • core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
  • core/modules/views/tests/src/Unit/EntityViewsDataTest.php
  • core/tests/Drupal/Tests/Core/Form/FormTestBase.php
  • core/tests/Drupal/Tests/Core/Plugin/Discovery/HookDiscoveryTest.php
  • core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
  • core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php
  • core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
  • core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php
  • core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
  • core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
  • core/modules/views/tests/src/Unit/Plugin/Block/ViewsBlockTest.php

These might have failed because of an additional unnamed namespace block:

  • core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
  • core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php

2) It appears that traits can not become links on the API documentation pages, despite the fact that they are following the coding standards.

3) The file core/modules/datetime/src/Plugin/views/filter/Date.php does not exist in my code base, so I could not fix it. (Last commit: 9f4d5e84fe213c546579d0d00db7385b69fa8470)

4) I tried to find files where the @file doc block was not ending in a full stop (.), but I haven't gotten the regex to work. This is not an issue, since these files will still be parsed correctly on the documentation site (i.e. become links).
If this needs to be changed before closing this ticket I would like some help with finding the files in question. My current grep is this: grep -r "* Contains \\\\Drupal.*" . | grep -v "\." (I'm on xUbuntu.)

tss’s picture

Status: Active » Needs review
justAChris’s picture

FileSize
4.23 KB
47.02 KB

Great work! There's still a few that don't match the suggested form. Mainly, it's the dot at the end of the summary, like in
/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php

A few with incorrect namespaces or classnames, like:
/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.php

Some missing the docblock:
/core/lib/Drupal/Component/Diff/Diff.php

I've attached a summary and the bash script that reviews the summary line of the docblock, there is some code to edit the files, but don't completely trust it yet. The script was tested on OS X 10.10 only, so may need tweaks on other environments. Change txt -> sh for usage

justAChris’s picture

Status: Needs review » Needs work
tss’s picture

Thanks for your feedback. I will try to look at the files you listed tomorrow.

I have a few more notes regarding the list of files that failed.

The file core/lib/Drupal/Core/Entity/Query/ConditionInterface.php does actucally work, I just missed a part of the namespace. This has been corrected and will be in a new patch.

The other files that does not get parsed correctly are all due to having an additional namespace declaration. Some of them have it in the beginning of the file and others at the end. They are all test files.

The three files below only have one namespace declaration and could be updated to not enclose the class in brackets. Would this be considered part of this task? (i.e. should I make this change?)

  • core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
  • core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
  • core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
jhodgdon’s picture

Regarding traits, really they aren't linked? Can you give an example?

tss’s picture

jhodgdon: Have a look here: https://api.drupal.org/api/drupal/files/8?object_name=.php&namespace=Dru.... There are only a few links in the fourth column, but they are for test classes.

tss’s picture

Using AllowedTagsXssTrait as an example:

- The filename is AllowedTagsXssTrait.php, which is the same as the class name.
- The namespace is Drupal\Core\Field, which is the same as in the doc block.
- The file doc block starts with double stars followed by @file.
- Backslash is used as separator.

This is according to the coding standards.

/**
 * @file
 * Contains \Drupal\Core\Field\AllowedTagsXssTrait.
 */

namespace Drupal\Core\Field;

use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Component\Utility\Xss;

/**
 * Useful methods when dealing with displaying allowed tags.
 */
trait AllowedTagsXssTrait {
...
jhodgdon’s picture

Thanks for that information! Looks like a bug in the API module. Filed:
#2502377: Trait links not being made
for that.

tss’s picture

Status: Needs work » Needs review
FileSize
708.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,050 pass(es). View

@justachris:

I have updated my local git repository, so the latest commit is now 7d8eb15a2d138b00d4f8bbde09186b584ff3bcc0.

I have updated the files listed in filesNeedUpdate.txt in comment #80, however I have some questions about four files:

1) ./drupal/core/lib/Drupal/Core/Archiver/ArchiveTar.php contains a comment line with vim settings. I have added the @file doc in the next code block and you script does not complain about it. The API module might. For now I will leave the vim settings comment.

2) ./drupal/core/modules/datetime/src/Plugin/views/filter/Date.php does not exist in my codebase even after a git pull. Which code base are you working on?

3) ./drupal/core/modules/migrate_drupal/tests/src/Unit/source/d6/CommentSourceWithHighwaterTest.php: The filename has a lowercase w in water, while the class name is with an uppercase. Which should I update? The filename or the class name? I'm leaning towards the filename, but I would like confirmation.

4) ./drupal/core/modules/system/core.api.php does not contain a class or namespace, so it is not relevant.

I ran the script locally after my changes and found some additional files. These has also been updated with the exception of the file in sites/default/files, since it is not part of the repository.

  • ./drupal8novice/core/lib/Drupal/Core/Database/InvalidQueryException.php : Reason : wrong/no classname or namespace, original: * Contains \Drupal\Core\Database\IntegrityConstraintViolationException., expected: * Contains \Drupal\Core\Database\InvalidQueryException.
  • ./drupal8novice/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php : Reason : wrong/no classname or namespace, original: * Contains \Drupal\conneg_test\Tests\ContentNegotiationRoutingTest., expected: * Contains \Drupal\system\Tests\Routing\ContentNegotiationRoutingTest.
  • ./drupal8novice/core/modules/system/tests/modules/conneg_test/src/Controller/TestController.php : Reason : wrong/no classname or namespace, original: * Contains \Drupal\conneg_test\Controller\Test., expected: * Contains \Drupal\conneg_test\Controller\TestController.
  • ./drupal8novice/core/modules/system/tests/modules/accept_header_routing_test/tests/Unit/AcceptHeaderMatcherTest.php : Reason : wrong/no classname or namespace, original: * Contains \Drupal\Tests\accept_header_routing_test\Unit\Routing\AcceptHeaderMatcherTest., expected: * Contains \Drupal\Tests\accept_header_routing_teste\Unit\Routing\AcceptHeaderMatcherTest.
  • ./drupal8novice/sites/default/files/php/service_container/service_container_prod/ffde4f1f3c4ff8ac7386a430403fe0a2316af0ec9e5319a24b251b8c3366b732.php : Missing or invalid @file doc block:
jhodgdon’s picture

Great!

Answers to your questions:

1) ArchiveTar has a LOT of problems. Let's just leave that one out of this patch and move on.

2) I don't see that date plugin class either.

3) Given the other comments in that class file, I think it should be HighWater also.

4) Any files that end in .api.php are NOT class files and should not be affected by this patch. Sorry for not mentioning that in the issue summary.

tss’s picture

FileSize
710.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,081 pass(es). View

Awesome, here's the final (?) patch. :-)

- Ignored ArchiveTar, Date, and core.api.php as previously mentioned.
- Renamed CommentSourceWithHighwaterTest.php to CommentSourceWithHighWaterTest.php (capital w) using git mv.

justAChris’s picture

2. Sorry about Plugin/views/filter/Date.php, it is indeed not there, I forgot to clean my local copy after a pull.

3. Agree that it should be HighWater in the docblock, matching the classname. Ideally, the filename should be exactly the same as the classname.

4. Again sorry, the api.php file should have been excluded since it doesn't use a namespace or declare a class. I messed up the regex:

namespace=$(grep -E '(^\*)*namespace Drupal\\.*' $i | sed 's/namespace //g;s/;//g;s/{//g;s/ //g' )
should be:
namespace=$(grep -E '^[^\*]*namespace Drupal\\.*' $i | sed 's/namespace //g;s/;//g;s/{//g;s/ //g' )
or simply:
namespace=$(grep -E '^namespace Drupal\\.*' $i | sed 's/namespace //g;s/;//g;s/{//g;s/ //g' )

Note: the ./drupal8novice/sites/default/files/php/service_container/service_container_prod note seems like you were possibly working with an installed copy since its not part of the code base, but don't see it in the patch and the patch applies cleanly, so it's ok.

The script no longer finds any issues, reviewing manually.

justAChris’s picture

Status: Needs review » Needs work

Two things, one very minor:

1. On diff --git a/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php b/core/lib/Drupal/Component/Plugin/FallbackPluginManagerInterface.php
* Contains \Drupal\Component\Plugin\FallbackPluginManagerInterface.php.
The training .php should be dropped

2. On diff --git a/core/lib/Drupal/Core/Database/Driver/sqlite/Select.php b/core/lib/Drupal/Core/Database/Driver/sqlite/Select.php
please review the trailing newline, the diff doesn't make sense for the update of this scope:
-}
\ No newline at end of file
+}

Note that the latest patch does not apply cleanly on OS X because of its file case in-sensitivity in conjunction with the HighWater file rename (maybe the ordering of the diff - addition then subtraction). It applied cleanly on Linux though.

jhodgdon’s picture

Just wanted to say... Great teamwork tss and justAChris on this! I haven't been looking at the patches because you seem to have it well in hand. Thanks!

jhodgdon’s picture

I think this will make sure both tss and justAChris get credit for this. Both should definitely be credited.

tss’s picture

FileSize
710.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-documentation-1392754-94.patch. Unable to apply patch. See the log in the details link for more information. View

@justAChris
New patch with the changes mentioned in #91. Thanks for the feedback.

The newline change was done automatically by sublime text, so I didn't even notice it, but I agree it is beyond the scope of this ticket.

@jhodgdon
Thanks for the appreciation. :-)

tss’s picture

Status: Needs work » Needs review
justAChris’s picture

Status: Needs review » Reviewed & tested by the community

Can't find any additional issues, I'm satisfied, updating status to RTBC.
@jhodgdon - hoping i'm not jumping the gun on this status change, but don't want to require another person to have to scroll through a long patch. Extra eyes are always welcome though

  1. I've verified that the patch only updates the docblock, and in some cases adds a docblock if missing. The one exception is core/modules/migrate_drupal/tests/src/Unit/source/d6/CommentSourceWithHighWaterTest.php, filename was renamed for case correctness.
  2. Minimal functional testing done since these updates only affect the docblock - applied patch and performed install

Repeating note that patch does not apply cleanly on OS X because of its file case in-sensitivity in conjunction with the HighWater file rename, but is ok on Linux.

Additionally, if the following patch is committed first, this patch will likely fail since files being updated are removed in that issue:
#2499835: Remove broken Fake DB driver

cilefen’s picture

Another issue may land before this one so at worst we will need a small follow-up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: drupal-file-documentation-1392754-94.patch, failed testing.

tss’s picture

Status: Needs work » Needs review
FileSize
700.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-documentation-1392754-99.patch. Unable to apply patch. See the log in the details link for more information. View

New patch.

justAChris’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to rtbc per notes in #96

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: drupal-file-documentation-1392754-99.patch, failed testing.

justAChris’s picture

Issue tags: +Needs reroll

In addition to reroll, the docblock on the following file should be updated, summary doesn't match the namespace anymore (or new file):
/core/modules/views/tests/modules/views_test_formatter/src/Plugin/Field/FieldFormatter/AttachmentTestFormatter.php

The last submitted patch, 99: drupal-file-documentation-1392754-99.patch, failed testing.

tss’s picture

Status: Needs work » Needs review
FileSize
701.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,841 pass(es). View

New patch.

Updated for the files mentioned in the QA log, the file mentioned in #102 and one additional file:

- core/modules/system/src/Tests/Update/UpdateSchemaTest.php

justAChris’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Sorry, I'm pretty sure that patch in #104 removes the previously mentioned highwater file:
/core/modules/migrate_drupal/tests/src/Unit/source/d6/CommentSourceWithHighwaterTest.php

I think that file rename can be cleaner in the patch as well with additional git configuration:
https://www.drupal.org/node/1542048, under "Optimize diffs for renamed and copied files"

cilefen’s picture

@tss @justAChris Considering the size of this patch in terms of the some ~1500 files affected, any commit on any day could force a reroll. It may be a good idea to get the attention of a core committer in order to plan a moment to get this in.

Crell’s picture

Issue tags: +avoid commit

Tagging according to #106.

Crell’s picture

Issue tags: -avoid commit +Avoid commit conflicts

Damnit, Drupal.org!

justAChris’s picture

Re-roll of #104
Still needs work to resolve file deletion in 104 as well as updates to the following new/moved files:
core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
core/lib/Drupal/Core/Cache/Context/ThemeCacheContext.php
core/tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php

justAChris’s picture

Status: Needs work » Needs review
FileSize
700.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,028 pass(es). View
3.04 KB

Moving this along since the patch will get stale very quickly.

Added updates to files mentioned in #109
Updated the remove on /core/modules/migrate_drupal/tests/src/Unit/source/d6/CommentSourceWithHighwaterTest.php to a rename

Not comfortable testing my own patch updates, so going to need someone else to review. I've provided an interdiff off of the previous re-roll and still approve of work up to #105 (with noted exception in that comment)

tss’s picture

@justAChris: Thanks for helping along with the patch.

@core committers: I would very much like to have scheduled a merge or something with one of you as suggested by @cilefen in #106. It doesn't make sense for me to keep rolling patches until it happens to be accepted at just the right time.

When would be a good time for you? I have time this coming weekend between Saturday 12:00 GMT (noon) and Sunday 21:00 GMT. (i.e. I can work late at night if it works better for you). Otherwise it can be weekdays next week from 16:00 GMT to 21:00 GMT.

jhodgdon’s picture

tss: Can you review the latest patch?

justAChris’s picture

FileSize
701.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,103 pass(es). View
4.04 KB

Updating Patch, one from 110 still applies cleanly, but there are two new errors in the documentation that have been resolved.
Included interdiff off of 109.

justAChris’s picture

Status: Needs review » Needs work

Patch in #113 no longer applies, conflicted with commits that occurred today, so needs a re-roll. Goal is to have the patch re-rolled and reviewed tomorrow. Core committer is aware of the patch due to conversation on IRC today, so we may have support to get this committed tomorrow if we can get it to RTBC.

justAChris’s picture

Status: Needs work » Needs review
FileSize
699.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,129 pass(es). View

Re-rolled patch

justAChris’s picture

FileSize
700.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,215 pass(es). View

Another re-roll

tss’s picture

I will see if I can get time to look at this tonight.

tss’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #116 is ok.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Avoid commit conflicts

There is never going to be a good time to commit this patch. There are only docs changes and therefore it passes the beta criteria for what is allowable. We will probably need to re-run the scripts before release but then there will be far less to change and change less disruption to the rtbc queue. Committed 1f53649 and pushed to 8.0.x. Thanks!

  • alexpott committed 1f53649 on 8.0.x
    Issue #1392754 by tss, justAChris, jhodgdon: Comply with new...
jhodgdon’s picture

THANK YOU @alexpott for committing this, and @tss and @justAChris for all of your scripts, rerolls, and careful attention!

Status: Fixed » Closed (fixed)

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