Follow-up to #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities:


Right, now both the SqlContentEntityStorage and the schema handler have to work together based on a known table mapping, what makes the line between both object blurry.

Proposed resolution

- Make the SqlContentEntityStorage table mapping agnostic and get the table mapping from the schema handler
- As suggested by yched in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities we could create multiple table mapping classes for the different table mapping cases also.

Remaining tasks

- Agree on the exact way forward and do it.

User interface changes


API changes

None, only some base classes change a little


fago’s picture

Status:Active» Postponed
yched’s picture

Basically, what differentiates an SQL storage strategy from another is entirely contained in the mapping layout (per field tables, per bundle tables...)
So having switchable mapping classes would allow alternate SQL storage strategies in contrib, while using the same EntityDatabaseStorageController.

yched’s picture

Related : crossposting from #2079019-10: Make Views use SqlEntityStorageInterface

I'd think TableMappingInterface are the objects Views should be taking its informations from ? In other words, IMO TableMappingInterface should be "the official business object to turn to when you need info about the SQL layout of an entity typed stored in an SqlEntityStorageInterface".

That probably requires working a bit on the API and DX of TableMappingInterface.
That might also mean moving the logic in _fieldTableName(), _fieldSqlSchema() & friends out of ContentEntityDatabaseStorage and into a base class for TableMappingInterface.

#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities got in (yay!) and added TableMappingInterface. IMO this isssue here is where we refine what we want to do exactly with that concept, and adjust its APIs accordingly.

fago’s picture

agreed. I think I this should be postponed on #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions now. It technically isn't, but that issue is going to the touch the same area quite a bit.

yched’s picture

plach’s picture

Nope, the other issue does not touch the storage class at all. The goal here is implementing the approach you suggested of moving the table mapping initialization logic that currently lives in CEDB::getTableMapping() to the table mapping instantiation itself (or lazy initialize it, details tbd). This also aims to make CEDB table layout agnostic and make it rely only on the provided table mapping.

plach’s picture

plach’s picture

Issue tags:+entity storage
andypost’s picture

Mile23’s picture

setEntityType() is deprecated for removal before 8.0.0, and references this issue.

ianthomas_uk’s picture

At this point is doesn't look like this issue is going to make it into the RC, therefore we need to decide what to do with the deprecation. Should we postpone it until 9.x, or can we mark the function as @internal? In core, there are calls to it in Core/Entity/Sql and the views module (edit: same name, different function), but no where else.

setEntityType was introduced in #2337927: SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields.

Edit: A couple of quotes from that issue:

"Adding deprecated functions to get the beta in kind of sucks." -- alexpott

"I agree that's not nice ... Since that method is not part of any public API, although being public, I think the problems implied by going this way are more theoretical than practical." -- plach

plach pointed out that while the method is public, it is not part of the interface.

plach’s picture

I'd go with @internal: this is an area that will need a lot of clean-up during the D8 lifecycle and at the same time I think most people won't need to fiddle directly with.

effulgentsia’s picture

I don't know which parts make sense to mark @internal, but +1 to someone figuring that out and opening a new issue for that.

ianthomas_uk’s picture

ianthomas_uk’s picture

@effulgentsia: My aim at the moment is to close #2205673: [META] Remove all @deprecated functions marked "remove before 8.0" before RC1 and this is a blocker for that. If there's more functions you or someone else wants to add so you maintain the flexibility to resolve this issue then please do. I don't see any big problems with marking too many methods @internal - after RC1 it's much easier to remove an @internal tag than to add one.