Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
- In #2326949: Move entity-type-specific schema information from the storage class to a schema handler, we introduced 'storage_schema' as another entity handler that can be specified in the entity type's annotation.
- However, unlike our other handlers so far, this one is currently dependent on the 'storage' handler, in that SqlContentEntityStorageSchema has constructor arguments that EntityManager doesn't know about, and EntityManager also doesn't know about what the default class should be if it's not specified in the annotation, since that default would depend on the storage handler.
- As a result, neither EntityManager itself, nor any other code external to the storage handler can interact with the storage schema handler directly: it can only interact with the storage handler, and it's up to the storage handler to delegate what it wants to the storage schema handler. This could be seen as either good or bad.
- It also means that calling
$entity_manager->getHandler('storage_schema')will break, and that's not documented anywhere. That also prevents EntityManager from being able to loop over all of its handlers reliably.
Proposed resolution
?
Remaining tasks
Decided what we want to do.
Comments
Comment #1
fagoThe constructor argumetn shouldn't be problematic when implement EntityHandlerInterface::createInstance()?
Choosing the default seems to be more problematic though. We could just hard-code the default for sql-based storage in the EM, or we allow the storage to specify it + have a dumb EntityHandler implementation action as default-factory only? (it figures out the class from the storage an instantiates that).
Maybe, this could be solved properly by an event system which allows handlers to alter the entity definition to add in defaults?
Anway, for now, I think hard-coding the sql-based default in the EM is fine.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedA constructor argument being added in #2330121: Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema is the $database connection. Presumably, the storage handler and the storage schema handler should act on the same database connection. An assumption we've made in the past with factory methods passed $container is that using those factory methods is optional, and that code should work properly if a consumer instantiates an object via its constructor, passing it arguments that don't need to match what's in the container.
So, suppose someone instantiates the storage handler with some other $database argument. How should ContentEntityDatabaseStorage::schemaHandler() be implemented? If it delegates to $entity_manager->getHandler('schema_storage') and that calls $schema_storage_class::createInstance($container), then that handler will get instantiated with a different $database than the storage handler is using.
Comment #3
fagoI see, but making the storage_schema handler a separate handler makes it part of the public API. Given that I think it's ok to say: when you switch the database for the storage you should do so for the schema as well.
Comment #4
plachThis looks major-ish to me...
Comment #5
plachMore or less...
Comment #9
Mile23There are a bunch of @todos for this issue. I'm encountering it from trying to inject a keyvalue service as in #2729597: [meta] Replace \Drupal with injected services where appropriate in core, via
SqlContentEntityStorageSchema::installedStorageSchema().Comment #14
DamienMcKennaFYI it seems this has lead to problems with EntityQueue, which resulted in a bug in distros that use it, e.g. #3126343: Scheduler needs to maintain its base fields properly.
Comment #16
joseph.olstadya , still flustered by this one.
we cannot upgrade to entityqueue 1.0 and I've followed the various threads to get here.
the combination of lightning_workflow with entityqueue beta5 installation seems to expose a problem and I haven't found a contrib way to solve it yet.
Comment #17
joseph.olstad