[meta] Many core class names violate naming standards

+1 for this issue

Demoting per IRC conversation. Doing an inventory of these would be good, as would git-blaming the names to see what justifications there are for the changes.

It sounds wise.

If I understand the new naming convention properly, there are potentially quite a large number of changes. Some of the easier examples:
Tar -> TarArchiver
Zip -> ZipArchiver

Com -> UuidCom
Pecl -> UuidPecl
Php -> UuidPhp

I'm a little less sure about the database names - I would think "Delete" falls under the "too ambiguous" clause of the naming convention. Is this change correct?

Database/Query/Delete -> DeleteQuery
Database/Driver/mysql/Delete -> DeleteQuery
(etc. for other drivers)

If I'm on the right track here, I'll be happy to go through and create a list based on jhodgdon's compiled class/interface names.

Com -> UuidCom
Pecl -> UuidPecl
Php -> UuidPhp

And actually it should be UUID and PHP and etc. See http://drupal.org/node/608152 #3 and #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName).

Thanks for the pointer on capitalizing the acronyms. UUID in particular is tricky though, since all of its backends are other acronyms - UUIDPHP and UUIDPECL are not terribly readable.

I see three options:
UUIDPHP, UUIDPECL <- In keeping with the standard, but I find it hard to parse visually.
UuidPHP, UuidPECL <- Make an exception to the standard
UUIDBackendPHP <- Use an extra word in between (they are backends, but there's likely a better term).

With that said, here's a list of the proposed changes. I aimed to be conservative. Transaction, Statement and Schema in particular I left as is - while not database-only terms, they have several derived class names (such as StatementEmpty, StatementPrefetch), so changing them would result in some unnecessarily long class names elsewhere.

I don't see what's wrong with UuidPecl, at least we don't have our file names yelling on us.

Definitely not a full list, agreed. Here's a more complete one, with some comments below.

NG appears to stand for "New Getter" - it is a set of temporary classes introduced in #1696640: Implement API to unify entity properties and fields. They will be removed before release, so I'm not worried about spelling out the abreviation too much.

Agreed with D) - FieldItem is better than just Item.

In the Core/Entity namespace, does "Controller" add information to the class names? For example, as far as I can tell, EntityForm is just as explicit about what the class does as EntityFormController.

Plugin -> PluginAnnotation. I was unfamiliar with "Annotations" in a programming context, so I didn't touch the names there. After looking over the code, I think I understand them a bit now.

DatabaseBackend -> DatabaseCache. As per the standard, "CacheBackend" doesn't provide any more information than "Cache" does alone.

MapperExceptionInterface, InvalidDecoratedMethod - These classes are unused. I will file a separate issue to remove them.

ContainerBuilder -> DICBuilder, RegisterKernelListenersPass -> DICKernelListenersPass - I think that DIC is a standard abreviation in use in the issue queue?

I don't understand the systems around EntityTranslation and EntityWrapper well enough to intelligently rename them. My best guess is that they should be EntityTranslationField and EntityWrapperFieldItem, but those are only guesses.

I believe that Field should be in the Field/ namespace, rather than Field/Type/, since it implements FieldInterface rather than FieldItemInterface. That's something for a seperate issue, if true.

Widget is hopefully unambiguous enough that it doesn't need to be changed to FieldWidget.

DIC stands for Dependency Injection Container. If the abbreviation isn't clear, I'm not sure how to name those classes concisely. InjectionContainerListenersPass is pretty unwieldy. Those two classes are very much internal - I can't think of a reason for contrib to extend or even reference them at all.

On a practical note, I'm starting to wonder how we'd handle this renaming as a patch. While in review/testing, it would have to be re-rolled quite often to chase head, and once it landed, every patch in the queue would have to be redone to accomidate it. And especially for some of the more confusing names, getting buy-in from people involved in that system will take time.

Basically, I'm not sure compiling a list of every change that should be made in one place is helpful. It might be better to open a separate issue for each component that needs to be updated - smaller patches and a smaller section of the community that needs to be involved will make actually implementing this change feasible.

I'll edit the issue summary and change this into a [meta] issue later.

I've started to create sub issues, one for each component affected. So far:
#1815982: Plugin class names violate naming standard
#1815988: UUID class names violate naming standard

The issue summary is also updated accordingly. More to come later. Anyone help pointing to existing issues I may have missed or creating the rest of the issues mentioned in the Summary would be very welcome.

If we intend to enforce the naming standard, it should be done as soon as possible.

Yes, but let's at least wait until after feature freeze before committing any of this. The pace at which we have been moving lately caused so many re-rolls already. A big change like this which would affect many files is going to make that even worse.

Anyways. This is really important and should indeed happen as soon as possible. But lets wait until after 1st of December.

Changing to task. It's not really a bug, or is it? :)

Agreed on waiting until after feature freeze. I saw it mentioned in another issue around coding standards, which is why I haven't moved any further on this.

I think Controller stands for a class that served a page request @_@ (not sure). In Drupal term that might be the page callback of hook_menu().

Yes, I am increasingly in pain with this myself. :/ I think we should be able to fix that after feature freeze (Dec 1st).

With the introduction of the new Feature Completion phase, I think we should leave this issue postponed until February 18. New features are still in progress, and I think it would still be counter-productive to force rerolls and confusion by changing core class names under their feet.

Differing opinions?

Tagging this so that we can revisit with the markup cleanup :)

read the issue... wrong kind of "class" names. sorry kids :/ untagging.

I had in my mind Feb 18th - the end of the "feature completion phase" - in mind for reopening this. On that day, we should have a 99% complete list of the classes that will be in 8.0 and can tackle them all at once, instead of having to keep updating the list as more features make it in.

It would certainly be possible to re-open now if someone wants to start work - I'll review any progress someone else makes, but don't want to start it myself until feature freeze.

Would it be correct to mark this meta-issue as Major when un-postponed, while leaving each issue it references Normal?

On that day, we should have a 99% complete list of the classes that will be in 8.0 and can tackle them all at once, instead of having to keep updating the list as more features make it in.

Well, I wouldn't go that far; there will still be refactoring and other tasks during feature freeze. :) However, I do agree we should wait until after Feb. 18.

Added issue summary and change into [META].

I added #1875996: Reconsider naming conventions for derivative classes to the summary. See also #1888702: Use configuration selection instead of derivatives for some blocks.

There are currently 880 classes in core that don't have Test in the name - just a few more than the 341 we had back in October. 121 of these are from Views.

I also noticed the file and class names involving "plugin_test" and similar - it seems to go against the convention used in other modules. "_test" is used ~100 times, while Test is used ~800 in filenames.

Documenting this here so it doesn't get lost in my bash history - this list was generated from inside the core directory using the command:
find modules/ lib/ -name '*.php' | xargs grep -o "^class [_a-Z]*" | grep -v "Test" >> classes.txt

plugin_test is a module, and that's our standard for namespaces. In fact, stuff like "argument_validator" is also in the namespace, not the class name, and isn't subject to this.
We're just evaluating the class name.

I've submitted a patch for #1815988: UUID class names violate naming standard - once that's reviewed and I know I'm on the right track, I'll create a few more for other subsystems.

@tim.plunket - Thank you, that is good to know. It would be issue-creep here even were it correct to change - I just noted it as something odd I'd seen while looking through the list of class names - and their namespaces, in order to figure out what ambiguous names should become.

• Changed issue summary to add Beta phase evaluation.
• While noting in comment #20 that jhodgdon says violation of coding standards is a bug, changed Category from bug to task after checking https://www.drupal.org/core/issue-category. Coding standards are not listed under either Bug or Task on that page, but a review of closed issues tagged "coding standards" showed the vast majority of such issues have been tagged as tasks, not bugs.
• Postponed issue to 8.1.x-dev per https://www.drupal.org/contributor-tasks/update-allowed-beta
• Removed Novice tag

Should this issue still be postponed? 8.1-5 has come and gone.

There is an issue to re-evaluate the naming standard. Let's keep this postponed on that, #2027221: [policy] Revisit class naming standards