[policy] Revisit class naming standards

I'd say we have to change the standard. Ignoring the namespace would lead to unacceptable long class names.

Please, let's allow namespaces to be used when determining what a class does.

1) We duplicate names when directly subclassing:
Drupal\Core\HttpKernel;
Symfony\Component\HttpKernel\HttpKernel;

2) We have Views field plugins and the Field entity type:
Drupal\field\Plugin\views\field\Field;
Drupal\field\Plugin\Core\Entity\Field;

If we try to enforce the policy as written, we'll quickly end up with http://en.wikipedia.org/wiki/Hungarian_notation

I thought the reason for the existing standard is so that when you're looking at code, you can actually understand what it's doing without having to refer to the namespace for context.

Here's an example from user.module.

At the top of the file:

use Drupal\entity\Plugin\Core\Entity\EntityDisplay;

Hundreds of lines further down:

/**
 * Implements hook_user_view().
 */
function user_user_view(UserInterface $account, EntityDisplay $display) {
....
}

Now if we assume that EntityDisplay were to be renamed to Display, the code becomes harder to understand, doesn't it?

By the way, are there really so many examples of code not following the current standard?

While looking for the above example, I definitely found some, but the majority seemed to already be using a class name that was descriptive on its own. I wasn't really looking through the entirety of the D8 codebase, though.

A good example is "DeleteForm".

\Drupal\forum\Form\DeleteForm
\Drupal\path\Form\DeleteForm

The Form\DeleteForm is redundant enough, we don't need Path and Forum tacked on the front.

New proposal:

Classes and interfaces should have names that explain their responsiblities in the context of the namespace, so the namespace information does not have to be repeated.
Notes:
Exception for Drupal 8.x: due to the way database classes are loaded, do not include the database engine name (MySQL, etc.) in engine-specific database class names.
// This rule can be explained by the namespace rule above.
// Not needed anymore: Exception for test classes: Test classes only need to be unambiguous within the context of the module they are testing.
Unit tests should follow the same namespace path so Drupal\name\Example\Foo should have Drupal\Tests\name\Example\FooTest as test class.

I'm very much in favor of this change. Having the same bloated names as in Drupal 7 in addition to namespaces seems insane. If no-one disagrees, it seems we should just be able to implement this?
My proposal for the phrasing (I agree with Jennifer, the exception has become unnecessary now):

Class, interface and trait names should be unambigous in the context of their namespace, and not duplicate the information of the namespace as far as possible.

#13 seems fine to me.

I agree that not everything needs to be repeated in the class name, but I would prefer if the main noun of the class name would describe what the thing is, and not just how it is different from other things in the same namespace.

E.g. Drupal\node\Plugin\Condition\NodeType should rather be Drupal\node\Plugin\Condition\NodeTypeCondition, because it is not in fact a node type, but a condition based on node types.

Drupal\zoo\Animal\Frog\Green should rather be Drupal\zoo\Animal\Frog\GreenFrog, because the object is a frog, not "a green".

OK, then what is needed further to implement this proposed change? Are there any more votes against? Do we need to get more votes?
It seems to me that, with a Beta release now out, a lot of module maintainers will start porting their modules, and it would be great if we had the coding standards up to date for that.

If this change is adopted, could someone clarify how the five examples currently given in the table at https://www.drupal.org/node/608152#naming would be changed (if at all?). It is not obvious to me what they would look like under the new standard.

I am also wondering if there is some confusion over what the wording of the current standard actually means and how it should be interpreted. When I asked for an example in #5 above of a class in Drupal 8 that doesn't follow the current standard, #6 suggested "\Drupal\forum\Form\DeleteForm", but to me that looks like it's following the current standard exactly :) Is it possible we just need more clarity in the wording of the current standard, rather than trying to change it? I haven't done an exhaustive examination of the D8 code but my impression is still that there are more places that follow the current standard than places that don't (although there are enough of each that it's a problem either way).

For clarity and posterity, here are the 5 examples from https://www.drupal.org/node/608152#naming as they are currently written:

Thanks... So, I'm not sure the wording in #13 really makes those examples clear? (When it says "not duplicate the information of the namespace as far as possible" that at least implies that you can use some common sense to duplicate information when you think it makes sense to, and I'd certainly do that myself for some of those examples.)

I'm also really not convinced that #13 is being used in Drupal core currently. I just ran this to list all core classes (excluding tests and non-Drupal code):

grep -RE -e "^(class|interface)" core/ | grep -v vendor\/ | grep -v Test

I attached the results. Skimming through them I see plenty that follow #13 but plenty that follow the current standard instead. I'm not even going to try to venture a guess as to which is more prevalent :) Unfortunately it does not look like this will be consistent in Drupal 8 no matter what.

The best solution I can think of is to distinguish a bit between interfaces and classes, and be more strict about interfaces because those are the ones that are by far the most likely to be used as type hints in function parameters (where #4 is a big problem if they don't have standalone names). Basically I consider #13 to be particularly problematic for interfaces.

Something like:

Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity.

If a class is not likely to be used outside of its namespace, it is acceptable to have a shorter name that is unambiguous within the context of the namespace only. Interface names, however, should always be globally unambiguous.

This is not perfect but would at least cut down on the number of violations in the core codebase.

c) We have written standards that Drupal 8 blatantly violates.

We already have plenty of code that violates the standards.
E.g. missing docblocks, etc.
We do want to fix this, but it is taking really long time.

I think it would be desirable to rename the most confusing examples. E.g. the "Select" and "String" classes you mention really would benefit from a rename.

But if some of our code base does not comply, we will survive it.
We could even state in the standards that (currently) not all of core complies.

The main goal here, imo, is to increase readability and DX. If not for all of our code, then at least for the new stuff we write.
Consistency is also a nice goal to pursue, but it is less important than readability.

When I asked for an example in #5 above of a class in Drupal 8 that doesn't follow the current standard, #6 suggested "\Drupal\forum\Form\DeleteForm", but to me that looks like it's following the current standard exactly :)

No, it isn't. ForumDeleteForm would be following the current standards – there are many DeleteForm classes in D8 (as #6 explicitly says).

The problem is probably that this also doesn't follow the proposed new standard, since the "Form" part would be redundant. So basically, complying with either standard would need a lot of renaming, which is unlikely to happen now, after the first Beta releases.
But I think the proposed new standard in #13 would still make more of the current class names acceptable, especially since there is a bit of room for interpretation with "as far as possible". At least it would not be as widely violated as the current standard.

Of course, the best would have been if people had just adhered to the coding standards while working on D8, but obviously that didn't happen and cannot be changed now. Now I think it makes sense to ammend the standards to more or less reflect the practice in Core, and thereby give clear guidelines to contrib module maintainers who want their code to conform to the standards, but avoid extraordinarily verbose class names that are also in stark contrast to Core's.

As for type hinting: in most cases, these will have a doc comment a few lines above them, which always has the FQN for reference, which should make this clear. For hooks, it's of course a bit more complicated, but we don't have that many of those anymore anyways, and the extra click probably won't hurt too much either. (Apart from the fact that we probably won't rename anything, or at least not much, anyways, so neither standard is going to make a difference there, at least for Core hooks.)

No, it isn't. ForumDeleteForm would be following the current standards – there are many DeleteForm classes in D8 (as #6 explicitly says).

Where are you seeing anything in the current standards that says a class name can't repeat more than one place in the codebase? All I see is that they are supposed to be understandable and not ambiguous, but that's not the same as saying they can never repeat (identical words aren't always ambigious if the difference is clear from the context). It also says that repeating the last part of the namespace ("DeleteForm") is good to remove ambiguity but the examples imply that repeating more than that ("ForumDeleteForm") is bad.

Anyway, I guess this proves my point above - we might need more clarity in the wording of the current standard first and foremost :)

For hooks, it's of course a bit more complicated, but we don't have that many of those anymore anyways, and the extra click probably won't hurt too much either. (Apart from the fact that we probably won't rename anything, or at least not much, anyways, so neither standard is going to make a difference there, at least for Core hooks.)

There are 265 hooks in Drupal 8 core right now (grep -R function\ hook_ core/ | grep \\.api\\.php | wc). And "extra click" only works if you are reading the documentation in the right place, e.g. api.drupal.org :) But the good news is that most hooks tend to take interfaces as function parameters rather than regular classes (for example the user_user_view() example I gave in #4 now looks different; it's actually EntityViewDisplayInterface) and most interfaces in Drupal core already seem to be pretty un-ambiguously named. I think if we changed the standard to focus on those kinds of situations there is not much that would need to be renamed.

Where are you seeing anything in the current standards that says a class name can't repeat more than one place in the codebase? All I see is that they are supposed to be understandable and not ambiguous, but that's not the same as saying they can never repeat (identical words aren't always ambigious if the difference is clear from the context). It also says that repeating the last part of the namespace ("DeleteForm") is good to remove ambiguity but the examples imply that repeating more than that ("ForumDeleteForm") is bad.

Anyway, I guess this proves my point above - we might need more clarity in the wording of the current standard first and foremost :)

OK, I'm not a native speaker, so maybe I really misinterpreted this then – though #6 looks like Tim Plunkett also sees it my way. In my eyes, two classes having the same name is the very definition of "ambiguous". Saying that the context makes them unambiguous could also be used as an argument against the "Form" suffix.

But yes, we can agree that the current standards aren't very clear to begin with – especially the examples.

And "extra click" only works if you are reading the documentation in the right place, e.g. api.drupal.org :)

Or in an IDE. And those two will probably account for 95+% of D8 documentation reading.

most interfaces in Drupal core already seem to be pretty un-ambiguously named. I think if we changed the standard to focus on those kinds of situations there is not much that would need to be renamed.

But then we'd lose the nice correlation between a lot of classes and their interfaces. I'm also thinking of contrib here, and prefixing each and every class (or just interface) there with the module name would just lead to extreme bloat. Especially if you then have to use the FQNs in all comments, which would duplicate nearly the complete name.

In my eyes, two classes having the same name is the very definition of "ambiguous".

I think the difference here is: The class name alone should be "practically unambiguous, one eye blind." or "unambiguous in most of the places where we expect it to be used" whereas class + namespace should be universally unambiguous even if used outside of Drupal.

So it is just a more sloppy interpretation of ambiguous.

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

Also marking for final discussion as this active issue received a lot of attention but needs a nudge to get finalized one way or the other.

I think there's consensus among the participants here on the problem/motivation part of the issue summary, but I don't think there's enough buy-in on the proposed resolution to warrant wider announcement for "final" discussion yet. I'd like to see a proposed resolution that addresses the concerns raised by David_Rothstein, so untagging for now.

Good points, Alex.

Marking "Needs work" for a solution that answers @David_Rothstein's issues.

I don't think "addressing David's concerns" is really possible in the context of the proposed resolution, since his proposal goes in an entirely different direction. As far as I understand, he just wants to clarify the current standards, while #13 proposes a completely different one.

As Jennifer explains in #22, there are three options:

a) We write down the standard that Drupal 8 is actually using. See #13.
b) We enforce our current written standards, which would involve renaming most classes in Drupal 8.
c) We have written standards that Drupal 8 blatantly violates.

#13 tries to implement b) – adapting the standards to make most of Core (though by far not all) comply with them.
#23 stays with c) – though you have to say that a lot of Core violates all kinds of coding standards, so this would hardly make it a no-go.

Of course, with just five people weighing in on the proposals, it's hard to decide anything, so I'd say what this issue foremost needs are a few more people stating their opinion. If David is the only one unhappy with #13, I don't think that should keep us from implementing it. (Or donquixote as well, from how it looks, though he proposes yet another solution, something between b) and c) – see #24.)
Also, Sebastian's and Tim's comment from the start of the discussion seem to also support #13 (but an explicit statement might still help).

Maybe just putting this up for "final discussion" would help get those additional voices, but of course I can also understand if you first want more consensus here.

I tried to keep the previous comment as opinion-free as possible to make it a good summary for new people coming to this issue to weigh in.

However, looking through the existing classes in Core (new list version attached), I can't really see how it currently conforms to any of the discussed coding standards to any significant degree – there is a complete mixture of "duplicate anything except 'Drupal'", "use just a single word, completely meaningless without the namespace" and anything in between. Forms, controllers and entities, for example, are pretty good at being unambiguous (and, thus, complying to the current standards) while almost all plugin class names are utterly meaningless without their namespace.

So, if we really want Core to more or less comply with coding standards, we'd probably have to look at this in much more detail and draft a new standard that would be pretty complicated (e.g., keeping most of the old, but introducing some more exceptions).

I don't really think that's desirable, either (drafting a complicated standard just to fit the current chaos in Core), and I concurr with donquixote that we will survive Core violating one more coding standard. What I chiefly want to achieve in this issue is to have a meaningful, easily understandable and above all practical standard for contrib.
The current one, if applied as I see it (or even if adapted as suggested by David_Rothstein) would lead to absurdly long class names in contrib, as they (or at the very least the interfaces) would have to always include the module name, and probably a lot of the namespace.

I quite like donquixote's proposal #24 of keeping class names ambiguous and short, but demanding that they "make sense" by ending in a word that really describes what they do (without necessarily making them unambiguous). If we drop the "make the standard to match Core" requirement, the only real downside I'd see here is that this wouldn't be the easiest-to-follow standard and we'd have to take care to phrase it properly. It would make the names slightly longer, but not nearly as long as the current standard in a lot of cases, and it would give us names which probably let developers identify the class in most cases without looking at the FQN.
What I really don't like about my proposal #13 is exactly that: that we end up with minimalistic names which become meaningless without the namespace, and thus probably a bit harder to parse at a glance when coding. (I'd just prefer it a lot to the current standard, and thought until now that it aligned better with what Core does.)
Also, with my proposal, the name of some plugin classes becomes hard to determine. (E.g., Search API defines a single Views cache plugin – what should that be called? The complete information is already in the namespace.)

Maybe some good suggestions/standards regarding import aliases could solve that, though. Might make things more confusing/unstandardized after all, though.

So, to summarize all proposed resolutions (I hope):

1. Keep the current standards: class names should be unambiguous and should describe the class completely without referring to the namespace.
2. #23: Adapt the current standard to let "primarily internally used" classes be more ambiguous – never interfaces, though. Would probably need a good explanation and/or set of examples.
3. #13: The opposite: let class names be as short as possible and only be unambiguous (and, sometimes, make sense) in combination with their namespace.
4. #24 (no proposed phrasing yet): #13 with the addition that, where necessary, a word describing what the class does should be attached, so that the names "make sense".

("Class" always meaning interfaces and traits, too.)
Core doesn't really conform to any of those, but practically uses a mix of "all of the above".

(Sincerest apologies for the wall of text!)

Following up on what I proposed in #16 and #24 and #30, maybe we could add (to be discussed):

• If we *already know* something is going to have the same class name as something else in core, or in a popular contrib module or library, we try to find a different name. E.g. if I would want to provide an alternative db layer in contrib (for which reason whatsoever) I might try to use different class names than those in core dbtng. E.g. MySelectQuery as opposed to just SelectQuery.
  This is controversial - we could say that the different namespace is sufficient.
• Naming is not an exact science. There is still human judgement involved, and sometimes there is no "perfect" name.