Move entity-type-specific schema information from the storage class to a schema handler

The patch looks ready to me if we want to keep the current approach. Reviews/feedback welcome.

A bit above my head at the moment, but requiring yet another extra class for each entity type is ideed not too great :-/

#2316171: [meta] Improve DX of entity defining (if you want a UI) already points that defining a new entity type is already quite mind bending in D8...

+  protected function schemaHandler() {
+    if (!isset($this->schemaHandler)) {
+      $this->schemaHandler = new FeedSchemaHandler($this->entityManager, $this->entityType, $this, $this->database);

+  protected function schemaHandler() {
+    if (!isset($this->schemaHandler)) {
+      $this->schemaHandler = new ItemSchemaHandler($this->entityManager, $this->entityType, $this, $this->database);

This looks a bit weird/inconsistent with the rest of the Entity system acrchitecture…

Why don't we declare/define/register the entity schema handler class like any other entity handler (formerly controller) in entity type plugin annotations?

Why don't we declare/define/register the entity schema handler class like any other entity handler (formerly controller) in entity type plugin annotations?

We discussed that approach in the initial phase of #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities. Basically the main concerns (that I didn't share :) were around polluting entity handler keys with something that is specific to our SQL storage implementation (somehow related to #2232465: Deprecate table names from entity definitions). Since handler keys are optional and can be extended by any module (Content Translation does it), I don't see any architectural problem with going the sun's way, which in fact is more consistent.

Is the schema really guaranteed to be SQL-specific? Are we sure that NoSQL implementations won't require it too; e.g., in order to introspect (arbitrary) actual data/columns against expected data/columns? Or do we assume that to be covered by (base) field definitions already?

Well, also an XML storage could exploit a schema handler or our config entity storage could use it to deal with config schema. Let's say that at least it shouldn't be a required part of the API. That was the main concern, I think.

Or do we assume that to be covered by (base) field definitions already?

This is not exactly an area where I am particularly knowledgeable, but I'd say that an entity object should be prevented from being populated with invalid values by typed-data checks / entity validation constraints. And yes, I'd expect a NoSQL implementation to rely on field definitions to decide what values should be actually stored.

Coming here from #2316171: [meta] Improve DX of entity defining (if you want a UI)... When I look at this code, it just makes me cringe.

So here's my perspective.

On the main Entity class, I define the base fields for the entity in baseFieldDefinitions(). It seems like I should be able to say "Oh, and this field is never NULL" there, and cut out these types of lines:

+      // Marking the respective fields as NOT NULL makes the indexes more
+      // performant.
+      $schema['taxonomy_term_field_data']['fields']['weight']['not null'] = TRUE;
+      $schema['taxonomy_term_field_data']['fields']['name']['not null'] = TRUE;

Would that be hard to add to the field definition, and thereby pick up in the storage class?

Providing a way to define what the indexes are on the base class would also seem to be desirable. Yes, it is SQL-specific, but if the method for doing that was optional (with an empty return on the base Content Entity class or something), someone who's building a custom entity for a site that is using some other entity storage could leave it out, and someone building a generic entity for a contrib module could include it, right? Forcing developers to implement an extra class, just so that they can have database storage that is efficiently indexed, is not great for DX.

And this move -- splitting up the custom storage classes into two classes -- seems like it makes the DX even worse for entity developers, although it may make the back-end guts of the entity system cleaner (if you could explain better this in the issue summary instead of referring to several other issues, that would be nice!).

Since this issue is part of a beta-blocking meta, we should stay strictly on topic (also my fault, sorry). The only argument of discussion here should be whether it's ok to move entity schema customization to a dedicated class. Everything else can be discussed in a separate non-critical issue.

About

And this move -- splitting up the custom storage classes into two classes -- seems like it makes the DX even worse for entity developers, although it may make the back-end guts of the entity system cleaner

Personally I feel the opposite: as @sun points out these classes are entity handlers, at least in theory. We have a consistent pattern of implementing generic behavior in a base class and extending it whenever needed with entity-type-specific code (and the more we can do in the base class the better, FWIW): if we go another way here, we are introducing an inconsistency that IMO makes DX worse. I really cannot see how an additional class is a DX burden: code has to be located somewhere and if my entity type needs some custom code, I see very little benefit in sacrificing consistency and cleanly separated code just to save one file. I thought we already made this decision when we decided to embrace PSR-0, after all.

FWIW (since I am the proponent of option 1) I find the DX implied by #12 worse precisely because it introduces a pattern we use nowhere else AFAIK. I'd rather go with a customizer method on the storage class as proposed by @eff in the parent issue (let's call it option 3).

Thanks! I did only a small addition.

I kind of like option 3...
I totally see the pro's, and the listed con's do not strike me as really problematic ?

Clutters SqlEntityStorageInterface with a public method that isn't required by the other options

But the code that would live in this method has to live somewhere, so other options add that same clutter in different places :-)
if the method makes sense as part of "being an SqlEntityStorage" (and "altering the schema for the entity type" makes sense to me), then adding it to the interface hardly qualifies as clutter ?

Adds an additional dependency of ContentEntitySchemaHandler on the storage object. This object dependency already exists, but if we're ever able to refactor the other reasons for that, then this new one will be a barrier to removing the dependency entirely

Not sure I get this one. Feels ok for an SQLStorage to depend on a SchemaHandler ?

Side note : is there an issue somewhere where we rename ContentEntityDatabaseStorage to ContentEntitySqlStorage ? Coz it is really about Sql (proof : it implements SqlEntityStorageInterface *and* it has a SchemaHandler), not about "other databases like Mongo"

Thanks for the very clear issue summary! Personally, given that explanation, I think we could go with any of the options.

effulgentsia asked me to read this and post my opinion. Please note that I'm not an Entity API expert.

Having read all of this, and all three patches, I'm missing a fourth option. This is essentially about "entity schema optimization/customization when using a SQL storage back-end". All three options propose to somehow separate the schema customization to prevent "clutter" when not using an SQL-based storage back-end. AFAICT all 3 options make the DX of creating an entity type worse.

Why wouldn't a storage back-end be able to consider these optimizations/customizations as hints? If an index or a NOT NULL is meaningless in a storage back-end, surely it can choose to ignore it?

So I agree with sun in #5 and #8:

This looks a bit weird/inconsistent with the rest of the Entity system acrchitecture…

Why don't we declare/define/register the entity schema handler class like any other entity handler (formerly controller) in entity type plugin annotations?

Is the schema really guaranteed to be SQL-specific? Are we sure that NoSQL implementations won't require it too; e.g., in order to introspect (arbitrary) actual data/columns against expected data/columns? Or do we assume that to be covered by (base) field definitions already?

Therefore, I think an option 4 could be:

1. don't add another class, add an optional method (perhaps via an optional interface, something like EntitySchemaHandlerOptimizationHintsInterface) to specify optimization hints
2. let the SQL storage back-end use them, let other storage back-ends decide for themselves
3. this does not cause worse DX

All three options propose to somehow separate the schema customization to prevent "clutter" when not using an SQL-based storage back-end

From what I understand, no, the Schemas we're dealing with here are SQL schemas, and are about SQL-based storages.

Therefore, I think an option 4 could be: don't add another class, add an optional method [to the SchemaHandler used by the entity type]

Well, that's option 1 ? With the DX downside that I need to add yet another cutsom class to code my CustomEntityType ?

Adding some links for convenience.

All three options propose to somehow separate the schema customization to prevent "clutter" when not using an SQL-based storage back-end.

Well, not exactly: the main goal is making the schema handler encapsulate entity schema handling. AAMOF non-SQL storages would not inherit from ContentEntityDatabaseStorage, thus they would not be affected by the decisions we are making here.

Option 4 is not that distant from Option 3, so my concerns remain the same :)

@yched:

Side note : is there an issue somewhere where we rename ContentEntityDatabaseStorage to ContentEntitySqlStorage ? Coz it is really about Sql (proof : it implements SqlEntityStorageInterface *and* it has a SchemaHandler), not about "other databases like Mongo"

We talked about that earlier but there was no issue, I created one: #2330091: Rename ContentEntityDatabaseStorage to SqlContentEntityStorage.

Thanks to Alex who walked me through this issue and the various choices. Of those, I prefer the patch at #18 (option 3). Rationale:

- It's the closest to what's already in HEAD, so not as invasive of a change.
- It's also an *improvement* over what's in HEAD. I would never have guessed that a function called "getSchema" let me customize schema. :) customizeSchema(), on the other hand, is extremely clear.
- I sympathize with the DX concerns yched raised about forcing entity authors to add Yet Another Freaking Class in option #1. And that's in the best-case. Worst-case, people forget that this is here and *don't* implement the various optimizations to make their entities more performant, which we don't want.
- So in that respect, I think the trade-off of more visibility is worth the extra public method that most callers won't need. Especially given that we've really not been meticulous about correct function visibility throughout the code base, instead defaulting to public everywhere "just in case." So this isn't really introducing a "new" problem. The existing function is public as well.
- Option #2 both doesn't resolve the confusing naming problem, and also requires 5 lines of boilerplate code around every schema handler, making it not my first choice either.

So given that, #3 is what I would go with, personally.

@webchick:

- It's the closest to what's already in HEAD, so not as invasive of a change.

What we have in HEAD is not the final solution, not even close, so the amount of changes needed to get there IMHO should not affect the decision, given we already have a working patch for Option 1.

- It's also an *improvement* over what's in HEAD. I would never have guessed that a function called "getSchema" let me customize schema. :) customizeSchema(), on the other hand, is extremely clear.

This argument I don't understand: by the same rationale we would need a customizeForm() method on our entity form classes, instead of just extending them to provide entity-type specific behaviors. Same reasoning with any other entity handler. Option 1 is just consistent with rest of the entity system. An entity type may need to extend the base schema handler for other reasons, and so it would have schema customization in a method of the storage class, where it would be completely out of scope as the storage responsibility is not dealing with the schema, and the rest of the customization code in the entity-type specific child class. The customizeSchema() method approach does not scale and would be confusing in such a scenario.

- I sympathize with the DX concerns yched raised about forcing entity authors to add Yet Another Freaking Class in option #1. And that's in the best-case. Worst-case, people forget that this is here and *don't* implement the various optimizations to make their entities more performant, which we don't want.

I think the first sentence is the real argument, the (valid) concern that people may not realize that they need to customize their schema can be applied exactly in the same way to Option 3.

- So in that respect, I think the trade-off of more visibility is worth the extra public method that most callers won't need.

I am not really disturbed by it being public per se. What concerns me is that it's another blocker towards eliminating a circular dependency between the storage class and the schema handler. Circular dependencies usually are indicators of bad architecture, and bad architecture will bite us sooner or later.

To sum up I think what people brought up here are mainly three different DX concerns:

A) The need to create a new class (file?).
B) The need to customize entity schema.
C) The inconsistency between the base schema handler instantiation, which is explicitly performed in the base storage, and the other entity handlers, which can just be retrieved from the entity manager.

Concern A and B are really not overlapping, as the need for B is already in HEAD and is not solved (nor is meant to be) by any option we are discussing here. Concern C is valid but is slightly off-topic here; however I am mentioning it as it's an example of architectural inconsistency that is causing DX troubles.

That said, the focus for this issue should be around Concern A, i. e. introducing a new class. I kindly ask everyone here to drop for a moment the concern about the perceived burden of creating a new class and look at the big picture: we have a nice and simple entity system architecture, that is allowing us to customize every single aspect of entity handling. And that granularity is important because it allows us to override these behaviors selectively: each handler has its own area of responsibility, which makes understanding its code, maintaining and customizing it easier (DX++). Moreover modules can provide replacements just for the handlers required to implement their business logic, without needing to touch the rest. An alternative approach could be having a single monolithic class, dealing with every aspect of entity handling: that would save a lot of files, but would it really be an improvement over the current system? I don't think so.

But here we want to go a different route because otherwise we need Yet Another Freaking Class. Well, personally I don't care at all about that: creating a new class requires 30 seconds to write the boilerplate code (5 if you use an IDE) and 1 minute to document it. The hard part is understanding how to properly implement methods to achieve one's goals, and that's Concern B. We have multiple concrete drawbacks in not going with Option 1 (inconsistency, tighter coupling, lack of "architectural scalability"), while the only one we have with going with Option 1 is "I don't feel like creating a new class". Good OOP is all about isolating well-focused code in single classes with clearly defined relationships, my feeling is that Concern A is only caused by the fact that in Drupal we are still getting used to it.

I agree with @plach. Option #1 feels like the correct one, that class is responsible for generating the schema, so alterations should be done there.

I do understand why people prefer option 3 when looking at the patch and this issue. One reason is that all those cases already have that method on the storage handler, so it seems easy to just rename it and slightly change how it works. However, having a custom storage handler will probably actually not be that common in contrib. I am working on a considerable amount of contrib projects with content entities, and so far, only few have the need for custom storage handlers (Part of that might be that they did not yet move custom queries to storage methods). For an entity type that wants indexes on it's fields* or other schema changes and does not yet have a custom storage, option #3 will not make much sense IMHO and #2 even less.

However, option #1 then actually has an even bigger flaw. And that is that you actually need *two* custom classes to define it, the storage which then returns a custom handler class. Which looks very convoluted right now with the lazy loading logic and the arguments we have to pass in.

I think we can find a solution to solve that, the easiest one going from the current code would be a property on the storage with the class name. One that I'm more and more considering is to just use the system we have for this: making it a handler. I was initially against that because the handler is more or less an implementation detail of the database/sql backend, but our entity handler system has two purposes. On one side, it is part of our public API (storage, view builder, forms) but the second part is being able to use it as a common API for entity type specific custom things. We've had the content translation handler as an example of that for a long time, and since recently, we have a second example and that's views data generation. And what's funny is that especially with #1740492: Implement a default entity views data handler, that will be *very very* similar to the schema handler. Both of them are using the primary definition (the fields and their properties) to generate a secondary definition (views data/schema).

So if we go with a handler named "sql_schema" or so, then it makes perfect sense to me for that to live next to a views_data handler. And others might follow, for example, we could still move the token definitions for entities to the same pattern.

We don't have to do this here already, but it might not be too hard to switch (we can keep getSchema() and route that through the entity manager just like current patches are doing) and would simplify the things an entity type has to do.

* It would be great to find a non-sql specific way of defining "I want to query this field (or this group of fields), so do whatever you have to do make this performant), then most current customizations would go away.

What @berdir said

Shortly summarized I agree with plach and berdir here.

So here the long variant:

A bit above my head at the moment, but requiring yet another extra class for each entity type is ideed not too great :-/

I know we have been working with the assumptions of "multiple classes = bad DX" for quite a while, but I've been realizing: it's wrong. Having multiple, clearly separated classes are perceived as *better* DX than a single class that mixes a lots of different concerns. Yes, I do think we think we have baken too much in single classes in the past sometimes (e.g. as xano once pointed out to me: baking field settings and default values form into field type classes :/). We - as Drupal community are not used to creating lots of classes - but developers in general are not scared of creating classes at all.

In that concrete case, it might look simpler when everything is a single class from a first glance. However, when developers are digging into it for whatever reason (it doesn't not work as expected?), the complexity and confusing code-flow of two classes playing ping-pong won't be perceived as simple or good DX. On the other side, a simple class inheriting the default and customizing a certain aspect of the system (schema), is rather simple and it's pretty clear how it works and plays together for every OO-aware dev (=every d8 dev).

- So in that respect, I think the trade-off of more visibility is worth the extra public method that most callers won't need.

As plach, I'd disagree that the overridden method is more visible than the overridden class.

- It's also an *improvement* over what's in HEAD. I would never have guessed that a function called "getSchema" let me customize schema. :) customizeSchema(), on the other hand, is extremely clear.

I'd disagree with that as well. While customizeSchema() is a good method name, in an OO world things are customized by extending a class and overriding a method. Imo, good code is embracing that instead of cluttering it with shiny names. With good code, I also mean simple and understandable code as there is no additional clutter for handling shiny names involved.

Why wouldn't a storage back-end be able to consider these optimizations/customizations as hints? If an index or a NOT NULL is meaningless in a storage back-end, surely it can choose to ignore it?

Problem is that the optimizations are specific to the schema produced by certain handler, thus might be invalid / not applicable in other scenarios.

* It would be great to find a non-sql specific way of defining "I want to query this field (or this group of fields), so do whatever you have to do make this performant), then most current customizations would go away.

Yeah, interesting idea. Sounds like an abstraction of indexes to the entity field level...

As plach pointed out already, option 3 mixes schema handler business into the storage class, making the whole separation quite pointless. What do we have it for then? We could achieve code-reuse and separation by a trait as well, while people could easily override things in their custom storage class. (Option 5, yeah)
- I doubt the tiny amount of memory saved when not loading that code *once* for all storage handlers makes a big difference. By using the trait we'd save per entity custom handler classes and instantiating handlers at all.
- Code can be re-used via the trait easily
- The trait is not as flexible/easily exchangeable as e.g. the handler class as proposed by berdir.
- The trait mixes schema-handler specific customizations into storage handlers, what makes switching to different schema handler approaches impossible given a pre-existing custom storage handler class (more on that below)

On one side, it is part of our public API (storage, view builder, forms) but the second part is being able to use it as a common API for entity type specific custom things. We've had the content translation handler as an example of that for a long time, and since recently, we have a second example and that's views data generation.

True, handlers are not per-se public API, but are a general way of doing per entity-type customizations of certain aspects. While it seemed unnecessary to expose storage internals as a separate handler initially, I must agree that doing it is quite a DX boost while I do not see any cons given it's clearly sql specific, thus +1 on that!
I was also thinking that the DX part that needs work here is *how* you specify the schema handler class, not having the class per-se. The schema handler as handler/controller (whaah) solves that just fine (and resolves the whaah :D).

Summing up, I think we should either do
- Option 1 and having it as separate handler/controller
- Option 5, the trait if we do not think we can separate things properly anyway

We've already started with the separation and it makes sense to me. Thinking a bit more about it though, it only makes sense when custom storage methods like NodeStorage::revisionIds() are written table mapping agnostic as well. This would allow you to keep the NodeStorage while switching out NodeSchemaHandler. Given people will/are experimenting with stuff like using doctrine orm, etc. with Drupal, this seems to be worthwhile - even if not done in core and left for entities in contrib to experiment with. Thus, the handler/controller approach with Option 1 seems to be the best way forward to me.

Discussed the name with @plach in IRC and came up with storage_schema too, we were both OK with that. Forgot to post an update about that.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -234,7 +232,8 @@ public function getSchema() {
     if (!isset($this->schemaHandler)) {
-      $this->schemaHandler = new ContentEntitySchemaHandler($this->entityManager, $this->entityType, $this);
+      $schema_handler_class = $this->entityType->getHandlerClass('storage_schema') ?: 'Drupal\Core\Entity\Schema\ContentEntitySchemaHandler';
+      $this->schemaHandler = new $schema_handler_class($this->entityManager, $this->entityType, $this);

What about putting the default in ContentEntityType, next to the storage default for content entities?

Also, the constructor/create() would be a bit more complex, but what about using $entityManager->getHander('storage_schema') and rely on EntityHandlerInterface to pass in the field definitions/storage?

Re few vs. many classes. I think it is not the number of classes/methods that you have to implement that is a problem, it's the discoverability of those classes and know what class(es) you have to implement to do something specific. And I think this is a good direction as it makes those classes visible in the annotation.

#40 makes total sense to me, thanks for the new patch :)

Just one doubt: since we are renaming child classes wouldn't it make sense to rename the base one to SqlContentEntityStorageSchema (for consistency with #2330091: Rename ContentEntityDatabaseStorage to SqlContentEntityStorage)?

Went ahead and performed the renaming. Feel free to revert if that's not ok.

Yes, works for me, we can revisit that later.

I think this is RTBC then.

Ok, sounds like we've come to consensus. The option that's here is better than the original option #1 in that the fact that this class exists is now documented in the annotation, so module developers will be less likely to accidentally skip it. It doesn't resolve either mine nor yched's concerns about Yet Another Freaking Class, but yched's not back for another couple of weeks and I don't really care about that enough to block this.

Committed and pushed to 8.x. Thanks!

Convinced by #37 & #40, all's good :-)

Just not fully sure about "a non-sql-specific notion of schema". I have no idea what that would be or what use you'd make of that. The current thing we have is very much an SQL schema used strictly for SQL things. I'm a bit wary of making it look like a generic concept when it's not.

Also, very interested by @Berdir's suggestion of astract indexes in #37.

Thanks!

Retroactively tagging as part of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

There's follow-up to fix term storage #2919332: Fix $reset parameter inside TermStorageSchema::getEntitySchema() parent call

+++ b/core/modules/taxonomy/src/TermStorageSchema.php
@@ -0,0 +1,119 @@
+  protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $reset = FALSE) {
+    $schema = parent::getEntitySchema($entity_type, $reset = FALSE);

For some reason term has typo for reset - so it always by-pass cache