Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity

The important part to me was the sibling relationship between Entity and Configurable (or ContentEntity and ConfigEntity), not necessarily the naming.
So I'm glad this is retained.

This is also highly preferred over the approach of "let's try to convince everyone that Entity now means also non-content". It implies that we were only focusing on a specific subset of entities.

So, in theory, I'm +1 on this.

Hah, I have more technical reasons for reverting this: #1761048: Revert StorableInterface/EntityInterface separation

Marked #1761048: Revert StorableInterface/EntityInterface separation as a duplicate.

As I mentioned in the original Configurable thread at #1668820-80: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc), I too struggle with the term "Storable" as something that's a) unique to Drupal and not found in other systems and b) an abstraction which doesn't actually gain us anything, unlike the abstraction of "Entity" in the first place.

So I'd definitely support this change, but we should certainly get a few more opinions because the "Storable" method had pretty broad buy-in at DrupalCon.

This suggestion makes sense to me... basically we are saying we have a base class of entity (with which we create instances of things)... Drupal has two special types of things - Content and Configuration - which need to be separated. This is right... and basically the whole point of CMI.

I agree with timplunkett, that the sibling relationship will help people to understand things. What I am not sure about, however, is using ContentEntity. For example, using ContentEntity for users. This to me would be equally confusing?

I agree with this idea. I'm still trying to explain to people what the D7 concept of 'Entity' means, so we have some education to do, but we should get the new terminology set before too many other people get used to what these terms mean in D7.

At the very least, the new terms reflect the fact that content and configuration have a common parent that they inherit from, and that's useful.

I think it would also be good to figure out where this terminology will be defined so we can flesh out the specifics.

I think the proposal is a step forward and brings us more clarity.

My concern is whether the above usage of the word Entity matches how the rest of the world uses the word. The concept of entities in software engineering goes back 35 years and exists in many systems, so let's not have Drupal deviate from accepted definitions.

Exactly - I've nothing to add here :-)

These terminologies are exactly what I came up with after that discussion in Munich, and all the people who I've talked to about this topic seemed to agree that it's a good idea.. so a big +1 from me as well :)

Ok great! Who wants to write a patch? :)

Bumping this to major, because I need at least the Storable stuff to be reverted ASAP for #1552396: Convert vocabularies into configuration.

The Storable* stuff was accepted as a temporary workaround, to address the concern of possibly needing to enhance the Entity[Interface] for "content"-specific things in the future, which might be incompatible with Configurables.

The only reason we changed the entity system to make Entity inherit from Storable is that the other way around (a ContentEntity extending Entity) would require plenty of renames and API changes all over the place (including documentation).

A few of us (namely @fago and me, I believe) still argue that there is no need at all to separate ContentEntity from Entity. That is, because pretty much everything in the entity system's architecture is optional and can be controlled per entity type. It is a generic system to manage data and things. It makes no assumptions or whatsoever on as to what kind or type of thing is being managed (and what business logic is layered on top of it).

The only concern that came up was that some modules want to display all entity types as select options to the user, in which case only "content" entities should be exposed. The obvious counter-argument against that is that Vocabularies and some other contrib entity types (like Flags) are "falsely" exposed as "content entities" today already, so they need to be filtered out, and so the argument is kinda moot, because it attempts to add a false dichotomy to a system that doesn't make a differentiation by design.

In the end though, that would be a simple matter of declaring meta-data for entity types; e.g.:

function taxonomy_entity_info() {
  $types['taxonomy_term'] = array(
    'tags' => array('content'),
  );
  $types['taxonomy_vocabulary'] = array(
    'tags' => array('configuration'),
  );
}

During the course of the Configurables discussion, we concluded that there are entity types, which can be both content and configuration, depending on a site's use-case. For example, the Taxonomy Access Control contrib module uses taxonomy terms in a way that rather maps to configuration than to content.

Furthermore, things like Flags and Users are hardly content. Users create content, and a Flag primarily represents a relation/reference, but not "content". These type of things are just simply entities. They might share some similarities with things that are content, but bending a full "content" notion onto them is wrong.

Thus, the meta-data declaration above tries to account for the fact that things can be classified into a lot more than content and configuration. In the end, however, it would probably make more sense to find more specific declarations that resemble actual business logic so that modules are able to extract the entity types they're looking for in a more granular way; e.g., "has own page", "has access", "is bundle", etc.

To get to the point: One of those declarations could even be "is storable" — copying over from #1761048: Revert StorableInterface/EntityInterface separation:

The bottom line is:

1. An entity is a defined thing.
2. A storable is undefined.

You may

3. Subclass entity for all more specific configuration things.
4. Subclass entity for all more specific content things.

But

5. There is no more abstract thing than a thing.
6. A thing may be storable, but if you want to classify by that, then storable things are a subclass of things. Not the other way around.

In other words:

• Entity » Storable
  Entity » Content
  Entity » Configurable
  Entity » Storable » Configurable

  ...are all valid inheritance chains.

• Storable » Entity

  ...is not.

However, it makes absolutely no sense to classify things by "storable", since all the entity types we're having and talking about are storable. But if you have an entity type that is not storable, then you specify:

  'controller class' => 'Drupal\...\NonStorableController',

I hope this provides sufficient architectural reasoning for why I would like to see the Storable* stuff just being reverted, without replacement, and without adding a bogus "ContentEntity" differentiation.

That is, because if we need to distill entity types for different use-cases, then that differentiation has to be done on a different level/layer (e.g., metadata). That differentiation would only exist for higher business logic, but has no impact code-wise.

Though I agree that the separation between content and configurable feels a false one (we are in the minority on this), trying to back out of "ContentEntity" being a sibling to "ConfigurableEntity," or trying to re-jigger the class hierarchy to include storable a level lower, is going to stall this discussion (and all of its various dependencies) for another N weeks. I would recommend doing the simplest thing that could possibly work, which is what Alex outlined in the OP. The decision to make those content and configurables siblings is what got buy-in during that configurable discussion, so we should stay with it. As we get more stuff done, we could revisit this "sibling" relationship again after feature freeze and see if the proposal in #12 makes sense.

The essential change; reverts Storable* to Entity*.

The renaming is quite complex to re-roll, so the code lives in the platform sandbox' entity-rename-1761040-sun branch. @tim.plunkett and many others have access to that sandbox already; if anyone needs access, just add them.

Fixed bogus type-hints in ConfigStorageController.

Removed bogus use statement.

ContentEntity and ConfigEntity aren't in that patch at all, and ContentInterface isn't used anywhere.

Yes. I don't want to rename Configurable. And I don't want to introduce a "ContentEntity." See #12.

If we really need to make use of the ContentInterface, then I'd strongly suggest to do it in a bare minimum fashion; i.e.:

1) make Node + Comment + Term (but not User) implement ContentInterface instead of EntityInterface, but

2) leave the rest of the system alone.

The real differences between entity types will happen on a per-entity-type basis. Just like it is today already with:

Node extends Entity
Comment extends Entity
Term extends Entity

...and likewise, with entity-specific controllers:

NodeStorageController extends EntityStorageController
...

Btw, there's a bug in the issue summary:

But, in a cursory Google search, I haven't yet found systems that call their configuration objects entities. According to my understanding, applications tend to use the word "entity" to refer to just the things managed by the application rather than all the behind-the-scenes things that make the application work.

This is a bit misleading, because we are not calling config objects entities. Config objects are and remain as configuration objects/files and exist behind the scene. Configurables, however, are things being managed by the application, so they perfectly match the definition of entities.

In the end, however, it would probably make more sense to find more specific declarations that resemble actual business logic so that modules are able to extract the entity types they're looking for in a more granular way; e.g., "has own page", "has access", "is bundle", etc.

Exactly. E.g., your module wants to display an entity? Get entity types that implement the entity render sub system.

>ContentEntity and ConfigEntity aren't in that patch at all, and ContentInterface isn't used anywhere.

I agree with sun that there is no point in having an empty ContentEntity class as base - interfaces are what counts anyway.

If we really need to make use of the ContentInterface, then I'd strongly suggest to do it in a bare minimum fashion; i.e.:
1) make Node + Comment + Term (but not User) implement ContentInterface instead of EntityInterface, but
2) leave the rest of the system alone.

Agreed - I've never though of relation entities or user accounts as typical site content. Anyway, I'm not sure what the implications of the "Content" stamp should be, but let's have it now for the sake of moving on and restoring a sane terminology and code. We can remove the interface before release if we figure out it's useless.

@name:
ContentInterface sounds a bit ambiguous, let's better use ContentEntityInterface.

Rerolled and updated the patch to do so.

#21 appears to be a new topic/discussion for me. Can we move that into a separate issue?

I think that #20 addresses everyone's concerns, so RTBC it is.

Let's get some reviews from people who don't buy into the sun/fago/webchick-think.

Here is a patch closer to what is described in the OP.

The only thing I'm now unsure of is whether the configurable code should be moved wholesale to Drupal\entity. But if sun or fago are happy with this as is, then I'm satisfied.

In the phrase "configurable entity", isn't configurable already an adjective?

But I agree, before it implied that other entities couldn't be configured :)

Caught the last couple stragglers.

I think we're good to go here! I checked with dawehner and he thinks this is fine as well.

Noooooooooooooooooooooooooooooooooooooooooooooo. Thanks for attention. Config is not an entity. That's why we did the Entity-Storable separation. We love PHP. Really. Really. OK. Let's do this.

Thanks.

Configurable vs. ConfigEntity will cause quite some havoc on its own. (custom/unique noun vs. entity)

However, if this is what it takes to get the insanity/regression of Storable* fixed, then be it.

This patch will break a couple of other config system patches that are in the queue right now, as well as contributed projects that are chasing HEAD (let's please stop presuming VDC would be alone), but I'm happy to re-roll and adjust those in exchange for a quick commit and the essential reversion here.

There are also some variable/comment renames that I don't fully agree with, but those can be fixed in separate follow-up issues/patches.

Well this certainly results in about 1,000x more readability/understandability of the code, so if this is good by both sun and the VDC folks, it's good by me! I really never liked the word "Configurable" as a noun anyway, despite having been the one to suggest it originally. ;P

Committed and pushed to 8.x. Thanks!

Great!

We missed removing the Storable StorageControllerInterface though, follow-up at #1772102: Remove StorageControllerInterface in favour of EntityStorageControllerInterface

Now we have these, we need to figure out how contrib modules are supposed to attach stuff to them, see #1783346: Determine how to identify and deploy related sets of configuration.

This should have a change notification, probably referencing the earlier change notice for Entity.

So http://drupal.org/node/1400186 is the other change notification that should be updated.

+++ b/core/modules/comment/lib/Drupal/comment/Comment.phpundefined
@@ -7,12 +7,13 @@
-class Comment extends Entity {
+class Comment extends Entity implements ContentEntityInterface {

+++ b/core/modules/config/lib/Drupal/config/ConfigEntityBase.phpundefined
@@ -2,23 +2,23 @@
-abstract class ConfigurableBase extends StorableBase implements ConfigurableInterface {
+abstract class ConfigEntityBase extends Entity implements ConfigEntityInterface {

+++ b/core/modules/config/lib/Drupal/config/ConfigEntityInterface.phpundefined
@@ -0,0 +1,25 @@
+interface ConfigEntityInterface extends EntityInterface {

+++ b/core/modules/entity/lib/Drupal/entity/ContentEntityInterface.phpundefined
@@ -0,0 +1,15 @@
+interface ContentEntityInterface extends EntityInterface {

+++ b/core/modules/entity/lib/Drupal/entity/Entity.phpundefined
@@ -17,6 +17,271 @@
-class Entity extends StorableBase implements EntityInterface {
+class Entity implements EntityInterface {

+++ b/core/modules/entity/lib/Drupal/entity/EntityInterface.phpundefined
@@ -10,6 +10,209 @@
-interface EntityInterface extends StorableInterface {
+interface EntityInterface {

+++ b/core/modules/node/lib/Drupal/node/Node.phpundefined
@@ -7,12 +7,13 @@
-class Node extends Entity {
+class Node extends Entity implements ContentEntityInterface {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Term.phpundefined
@@ -7,12 +7,13 @@
-class Term extends Entity {
+class Term extends Entity implements ContentEntityInterface {

So these are the API changes.

Hm, why don't File and User implement ContentEntityInterface?

Okay per #19 users aren't content, which makes sense. But IMO files are?

Patch.

Less broken patch.

I added a new change notification and also updated the table in the original entity change notification. The patch in #39 still needs review, although I did update File in both change notifications.

Yes, files exactly a kind of content

Makes sense to me.

Committed and pushed to 8.x.