Make SqlContentEntityStorage table mapping agnostic

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.

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.

Is this now a duplicate of #2326719: Move pseudo-private table mapping functions from ContentEntityDatabaseStorage to public API of DefaultTableMapping ?

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.

#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities landed long ago.

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

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.

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.

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.

I've uploaded a patch to do that to #2565307: Document SqlContentEntityStorage::setEntityType as @internal instead of @deprecated

@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.

Let's give this a try.

I'm not sure about who should be responsible for generating the default table names and the authority for retrieving this information, I assume TableMappingInterface? If so, we also need to add getBaseTable() and friends to that interface.

Fixing most of the failing tests means that we have to decide who's the primary source of information about the entity tables, either the storage handler or default table mapping class, so I'm going to hold on with any updates until we decide one way or the other.

Posted this on IRC after a cursory look at the patch, didn't actually do a line-by-line review:

I like that issue a lot. We should try to summon plach for that one, though. I originally wanted to put the initTableMapping() logic in TableMapping, as well, but plach wanted to have it more as a value object. (I didn't think of even putting baseTable(), etc. in there as well at the time, but I like that!!)

Reroll

Next try

And an other try

Fixed some tests and some code cleanups.

Some code changes and fixed the test class Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.

Somehow some code changes where reverted.

Lets see what the testbot thinks.

Lets see what the testbot thinks.

Thanks @daffie for getting this patch to a state where it actually passes all the tests!

Sadly, I was too ambitious in my initial patch from #18 and tried to fix too many things at once.

If we want to finally put this issue to rest I think we need to work in smaller steps, so I opened a child issue with the first step: #2916018: Allow the table mapping to be initialized from outside the storage

That first step from #39 turned into a huge plan (#2960147: Finalize the entity storage) with the ultimate goal of being able to fix this issue.

NW for an updated issue summary.

And reroll for 10.x

Re-roll patch against 10.x

Re-roll patch against 10.x