Comply with new documentation standards for @file for namespaced class files

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. :-)

Tagging so the right people see this.

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

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.

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...

oops, cross-post. :)

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.)

#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?

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.
 */

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

#11 sounds good to me, too.

#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".

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. :)

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.

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?

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

Yeah, sounds good.

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?

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.

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

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.

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

/**
 * @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.
 */

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\".

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. :-)

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 :)

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.

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.

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.

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.

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.

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?

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

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

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

#38 is fine by me.

Another +1 to #38

Its better for sure. +1

Also happy with #38.

Ok, sounds like consensus.

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.

(summary is updated)

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.

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

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

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".

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.

+1 to #50.

update with latest proposal

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?

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. :)

No dissent. Let's do.

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?

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

updated proposal

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...

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.

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.

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...

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.

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"?

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.

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.

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.

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.

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.)

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...

update status

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

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?

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

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.

I am currently working on this.

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

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

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.)

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

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

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

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.

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 {
...

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

@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:

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.

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.

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.

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.

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!

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

@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. :-)

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

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

New patch.

Setting back to rtbc per notes in #96

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

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

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"

@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.

Tagging according to #106.

Damnit, Drupal.org!

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

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)

@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.

tss: Can you review the latest patch?

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.

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.

Re-rolled patch

Another re-roll

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

The patch in #116 is ok.

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!

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