Naming standard for abstract/base classes

Thanks. I've rewritten the issue summary.

I HEAVILY prefer ThingAbstract. I've done this any time I've written an abstract thus far and it has served me well. Visually it's nice with it's proximity to interfaces, and yeah... this is definitely my preference.

Eclipse

"Abstract" is essentially hungarian notation, like the Interface suffix: It indicates something about the data type.

"Base" is a hint for how the thing is expected to be used: Whether abstract or not, the intent is that it is used as a "base" class and that you probably want to extend it to do anything useful.

I am not at the moment sure which one I'd prefer, other than preferring suffixes over prefixes so that classes appear together in file listings. They're also more discoverable via an IDE that way, as you can start typing "Fiel..." and get a list of Field API-related classes, base or otherwise. You're probably not going to start typing "Bas..." in the hopes that there's a Field API-related thing in there somewhere.

yeah, I'm happy as long as whatever gets used is a suffix.

My vote is in line with @yched's original proposal, ThingBase ("Base" suffix, abstract or not).

My reason for "Base" vs. "Abstract" is that during architectural design and further development, it's often clear whether a class will act as a base, but it's not necessarily 100% clear or guaranteed whether it is or has to be abstract and whether it will stay so. "Base" avoids nasty renames later on.

A more advanced example for this naming will be test framework base classes: (via #1541676: Convert Simpletest base test classes to PSR-0)

namespace Drupal\simpletest;

class TestBase;

class UnitTestBase extends TestBase;

class WebTestBase extends TestBase;

Speaking of, we should adjust existing core patches accordingly, and we also need to rename existing classes in core, once we have an agreement here.

So then what we're proposing is:

Abstract classes do NOT get any special naming.

Classes that "are intended for you to extend to make use of them rather than being used directly" get a "Base" suffix.

There is a lot of overlap in the above two categories, of course, but it technically still allows for non *Base abstract classes, no?

it technically still allows for non *Base abstract classes, no?

Yeah - I'd call that a feature. ;)

@yched: Don't think @Crell implied that, as we've been standardizing on SpecificThing in many PSR-0 conversions already. But yes, would be a good idea to formalize it in the standards as well.

sun: OK, so would I. I just wanted to make sure we knew that we were doing that.

yched: As of the last naming convention discussion, the shortname for classes is supposed to be "something that makes sense without the namespace in most cases". That sometimes means SystemThing, sometimes ThingSystem, depending on which makes more sense in context. (That is, DatabaseCondition, but ImageField.) It's imprecise, but the decision was that we'd rather code read well than have precise naming rules. So ThingWidget vs WidgetThing is left up to the Widget system to decide which made more sense in its case.

Is it appropriate to call this RTBC?

Yep, I think so. #1541676: Convert Simpletest base test classes to PSR-0 has already been updated for this.

Is this going to contain a patch to change all the existing abstract classes to use Base?

Ideally yes. However, we might want to wait for at least the kernel monster patch to land, as I guess that renaming all these classes will cause quite some headaches otherwise.

I don't see any disagreement on the idea here. What should we add to http://drupal.org/node/608152 ?

How about:
- Classes meant to be used as base implementations should have a suffix "Base", whether they are abstract or not.

And an example showing Base as good and Abstract as bad?

@jhodgdon Sounds good.

I suggest to not deal with renaming the test base classes ( there are tons of them, every second module has one or even multiple ones) in here but deal with that in separate issues. That would otherwise result in a huge page and we can clean up the names while doing the PSR-0 conversion.

#1495024: Convert the entity system to PSR-0 just happens to run into this:

/**
 * Defines a base entity class.
 *
 * Default implementation of EntityInterface.
 *
 * This class can be used as-is by simple entity types. Entity types requiring
 * special handling can extend the class.
 */
class Entity implements EntityInterface {

Quite the edge-case. Is this an EntityBase or not? :)

Ugh. :( It would be sad to make our classes less intuitively named just to follow a coding standard. :( Wonder if we can make some sort of exception for classes like Entity, Node, Comment...

If Entity is a class that is intended to be extended OR used directly, then it shouldn't get a suffix. *Base is for things that, really, why are you instantiating this directly? I suspect some of them should even be Traits in PHP 5.4. :-)

Given the last few comments, how about amending the proposed description in #16 to say [emphasis here shows new words added since #16]:

Classes that are only meant to be used as base implementations should have a suffix "Base", whether they are abstract or not.

And then we could show these examples:
- Entity (stands alone or as a base class, so no suffix) vs. EntityBase (bad)
- (what's a good example of an actual base class in D8 that should have the suffix?)

+1 to #12. As for a base class example, uh... Hopefully WSCCI and SCOTCH will have some for you eventually but we don't yet. Sorry. :-)

We now have TestBase, WebTestBase and UnitTestBase in, how about that for an example?

The simpletest base classes are an excellent example, since no one is going to use those directly. Good idea!

Excellent! So is everyone in agreement on the standard in #21?

Best as I can tell Entity is not ever used stand alone, it is ALWAYS extended. This based on some VERY quick grepping so I could be wrong, but Entity looks like a base class to me. It's got a lot of generic logic that should work 80% of the time for entity types out there, but most will probably need to provide 20% of their own code to get what they really need out of it.

If you can tack 'abstract' onto the front of the class and nothing in Drupal breaks... chances are it's an abstract (I tried to verify this with the test framework, but couldn't get through all the tests, still... you'd expect something catastrophic to happen if this weren't actually being used like an abstract already).

Eclipse

TL:DR; Entity class looks like a base to me, so we need a better definition of what is NOT a base.

ECK is using the base entity classes as final implementations. Of course, no idea whether that's going to change for D8.

However, even given that counter example, I'd still say that Entity should be EntityBase, because I think what matters is the author's intention - being:

You probably want to use this as base class for your own. Even though it's also possible to use it directly.

To that extent, the OP already stated:

A base class might be abstract or not. The "baseness" is a very important hint: When you want to implement your own thing, look up the at ThingInterface and notice there is a ThingBase to it. You learn it's probably a good idea to consider subclassing the base instead of starting fresh. Regardless of whether the base class is abstract or not.

Given #27, it sounds like you do not agree with the "only" wording in the proposed standard in #21. It sounds like you are suggesting that the wording should be "mainly"... right? In which case, should we use Node as the prime example of a class that should *not* have Base on it?

To be honest, I'm not very clear on what the decision was here. For the TypedData API currently being worked on, we decided against TypedDataBase (since we don't want confusion with "database"), but now in #1794212: Rename ItemBase to Item, are wondering whether to drop the Base suffix from other classes in the TypedData API or not. Suggestions there welcome. Thanks.

Well, the problem is that we wanted an easy to identify standard that would indicate when someone was looking at a class intended for use as a base class (again not always an abstract) so... I'm still generally ++ to the notion. It doesn't fix the compound word problem in general, but in specific to Typed Data, I would be totally ok with TypedDataApiBase (or similar). I suppose we could be more verbose and do BaseClass is our indicating suffix, but that just extends the convention in a way that should eventually make sense when people see it. I'm not sure there's a good answer here. I still think the principle is one we should embrace.

Eclipse

Updated issue summary.

Coding standards decisions are now supposed to be made by the TWG

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

This was discussed briefly at the CS meeting, #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC with myself and Björn Brala (bbrala). We only got as far as acknowledging that this is complex and will need further discussion.