Allow the table mapping to be initialized from outside the storage

I think this makes a lot of sense. We had discussed where to put the initialization code when originally introducing the table mapping and at the time the table mapping was thought to be more of a value object without any logic, which is why we put it in the storage handler. It has since shifted its purpose a bit and the code is now a bit alien to the storage handler. Also, any way to remove complexity from the storage handler - even if it is simply moved verbatim to a different class - is appreciated as the storage handler is quite a monster.

All this is regardless of #2274017: Make SqlContentEntityStorage table mapping agnostic . I just wanted to provide some reasons why I think this makes sense as a separate issue regardless of the progress we make there.

Not RTBC-ing because, as this is both a very complex and a highly critical part of core, I want to scrutinize the moved code and its test coverage a bit more before RTBCing. Didn't see anything noteworthy from a cursory look, though.

If we are moving towards deprecating the ability to specify shared table names in the entity type definition, which is a Good Thing™, and we are also decoupling the instantiation of the table mapping from the storage handler, we should retain the ability for individual entity types to determine the shared table names.

I'm wondering whether it would make sense to use a factory, as we do for entity queries, so that an entity type can provide an alternative table mapping with its own shared table names.

Additionally, a factory may help us in keeping the instantiation logic out of the table mapping itself, which would keep it as a value object (more or less). I remember @yched suggesting that maybe we would need a separate table mapping class for each layout we support (base, translatable, revisionable, both), but maybe encapsulating the instantiation logic in a factory would be enough for now, and could still allow us to implement that later without BC issues.

Thoughts?

I'm curious where exactly would developers be able to specify the shared table names with your proposal

Well, by being able to specify a table mapping for each entity type we would be able to individually override table names, either by assigning a different value on construction or by doing that directly in the factory method, I didn't think about the details honestly.

[...] the actual goal of these issues should be to lower the complexity of the storage and storage_schema classes, and allow us to fix all those outstanding @todos about "do this in a better way when we're able to instantiate a table mapping from outside the storage".

Agreed, so coming up with a proper way to instantiate the table mapping seems definitely in scope to me :)

Ok then, wanna go ahead and try to work on a PoC patch?

Nope :)

I don't think I have time to work on that, right now. Feel free to move on with the current approach, if you don't have time or will to take #6 into account, but please make sure trying it later doesn't imply a BC break, by using @internal or whatever is needed ;)

Sorry, I misunderstood your earlier comments :)

I wasn't aware about the Entity Query factory deprecation. Maybe it could still be acceptable if we delegated the table mapping factory instantiation to the storage handler? This would only pass the entity type id and the entity type manager to the factory, while the entity type and field storage definitions would still be parameters passed to the instantiation method, e.g.

/** @var TableMappingFactoryInterface $factory */
$factory = $storage->getTableMappingFactory();
$table_mapping = $factory->getTableMapping($entity_type, $storage_definitions);

Edit: ::getTableMapping() would only check that the entity type id matches the one the factory was initialized with.

Various comments below, also on parts of the code that is just moved here. I'm not suggesting to change any of that, just trying to find my way back into the topic by going through this a bit and connecting some pieces and thinking out loud. Feel free to ignore anything that is not strictly relevant in your opinion and/or get back to it later in other issues.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -87,6 +115,120 @@ class DefaultTableMapping implements TableMappingInterface {
   +    // @todo Remove table names from the entity type definition in
   +    //   https://www.drupal.org/node/2232465.
   +    $this->baseTable = $this->entityType->getBaseTable() ?: $this->entityType->id();
   +    if ($revisionable) {
   

   Commented over there, this is just moved but I honestly think it's more than unlikely that we will be able to do this any time soon. Best we can hope for for now would be to offer defaults that can easily be overridden?

   And if we'd really get there at some point then I guess they would still remain as a read-only API on this object, so this is yet another step where this is going into the right direction.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -87,6 +115,120 @@ class DefaultTableMapping implements TableMappingInterface {
   +    $key_fields = array_values(array_filter([$id_key, $revision_key, $bundle_key, $uuid_key, $langcode_key]));
   

   Interesting that we here treat only very specific keys as actual keys (whitelist) while \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema() uses a blacklist approach to make all keys except a few not null. I guess things there will change with/after #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field?

   Also wondering if there is a specific reason that the bundle is in this list/the base table, I don't think there is a good reason for that?

3. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -87,6 +115,120 @@ class DefaultTableMapping implements TableMappingInterface {
   +      // performant queries. This means that only the UUID is not stored on
   +      // the data table.
   

   I've always wondered if there is a reason for that, avoiding duplication of the fairly long field?

   This is one of the reasons why I got somewhat stuck on #2875033: Optimize joins and table selection in SQL entity query implementation as it resulted in a regression for a pretty common entity query.

4. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -227,6 +365,12 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
   +   *   before Drupal 9.0.0. There will be no replacement for it because the
   +   *   default table mapping is now able to be initialized on its own.
   

   If we would move this to a factory then I guess this would no longer be true but I guess we'd instead inject it in the constructor to keep it immutable so just wrong on the details.

5. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php
   @@ -41,20 +41,20 @@ public function testGetTableNames() {
        // The storage definitions are only used in getColumnNames() so we do not
        // need to provide any here.
        $table_mapping = new DefaultTableMapping($this->entityType, []);
   

   It is apparently still not actually required to pass field definitions but at least the comment is no longer correct like that?

   And as you said, this is not really testing that much

6. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php
   @@ -362,47 +362,37 @@ public function testGetFieldTableName($table_names, $expected) {
   -    \Drupal::setContainer($container);
   +    $this->entityType
   +      ->expects($this->any())
   +      ->method('getRevisionMetadataKeys')
   +      ->willReturn([]);
    
   

   Nice cleanup of the mocking required here.

   I guess the EntityType classes are not quite plain value objects enough to just use them directly of mocking it and changing that would also result in large changes to the whole class.

   Part of the mocking seems a bit backwards to where we want to go in regards to #2232465: Deprecate table names from entity definitions, e.g. defining isTranslatable/isRevisionble based on whether we have the respective tables but we can change that later.

7. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -389,13 +389,22 @@ public function testGetSchemaBase() {
   +    $this->entityType->expects($this->any())
   +      ->method('getRevisionMetadataKeys')
   +      ->will($this->returnValue([]));
   

   Yep, exactly the BC layer in there is I guess one of if not the main problem to using them as non-mocked value objects in these tests. Too bad :)

   (Although I'm wondering if we could instead provide explicit values to skip the BC layer)

Thanks, I think this makes sense then as a first step, we have many other/related issues to make things nicer.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -25,6 +25,34 @@ class DefaultTableMapping implements TableMappingInterface {
   +  /**
   +   * The table that stores revision field data if the entity supports revisions.
   +   *
   +   * @var string
   +   */
   +  protected $revisionDataTable;
   

   This is missing a bit: "[...] and has multilingual support."

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -87,6 +115,120 @@ class DefaultTableMapping implements TableMappingInterface {
   +    $this->initializeMapping();
   

   This looks potentially BC-breaking to me: what if a DefaultTableMapping class is instantiated and expected to be empty/clean?

   Very unlikely scenario, I guess, but what if we added an @internal static ::create() method encapsulating the logic of ::initializeMapping() instead?

   DefaultTableMapping::create($entity_type, $storage_definitions);
   

   This would allow to keep the constructor "clean", and would be closer to an actual factory method.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -227,6 +365,12 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
   +   * @internal
   
   @@ -254,6 +398,12 @@ public function getExtraColumns($table_name) {
   +   * @internal
   

   Isn't @deprecated enough here? Are we adding those in case we need to un-deprecate these methods, if we end up introducing a factory? In that case I'd rely on constructor parameters/initial values and keep those methods as @deprecated.

Adding a static create() method means that we also need to add getters for base/data/revision/revision data tables

Why is that so? A static method can access protected and private members of a class instance. You just need to instantiate a $table_mapping in the ::create() method and replace all occurrences of $this with $table_mapping:

  public static function create(ContentEntityTypeInterface $entity_type, array $storage_definitions) {
    $table_mapping = new static($entity_type, $storage_definitions);
    $revisionable = $table_mapping->entityType->isRevisionable();
    $translatable = $table_mapping->entityType->isTranslatable();

    // Rest of the code from ::initializeMapping()...

    return $table_mapping;
  }   

That's right. And, same as above, we can figure out what needs to happen with these two methods in the instantiation issue. For the moment, I just took the safest approach.

I was asking because both @xjm and I found weird that we are adding both the @deprecated and the @internal tags: @deprecated should be enough to tell people they are not supposed to use those methods anymore.

Interesting indeed! I had a closer look and I discovered one thing that I didn't realize earlier: in HEAD the table mapping retrieves the base table names internally in ::getFieldTableName(), which makes sense, since it should ultimately be responsible to provide this knowledge to the outside world :)

Hence I think we should always initialize the table names upon construction, instead of doing that in the create method. This should be more compatible with the HEAD behavior and in line with the plan of deprecating the table name entity type keys.

In fact, if we do so, we should be able to revert all changes in DefaultTableMappingTest, except for the mocking on the entity type keys.

I think the review has been addressed and this is RTBC again.

Looks great, thanks, I'm planning to commit this tomorrow!

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php
@@ -362,38 +362,35 @@ public function testGetFieldTableName($table_names, $expected) {
+      ->willReturn(isset($table_names['data']) ? TRUE : FALSE);
...
+      ->willReturn(isset($table_names['revision']) ? TRUE : FALSE);

If you really wish to listen to my OCD, we could simplify these to just ->willReturn(isset($table_names['revision'])); ;)

Saving credit

Committed 295e297 and pushed to 8.6.x. Thanks!

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -227,6 +378,10 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
+   * @deprecated in Drupal 8.6.0 and will be changed to a protected method
...
   public function setFieldNames($table_name, array $field_names) {

@@ -254,6 +409,10 @@ public function getExtraColumns($table_name) {
+   * @deprecated in Drupal 8.6.0 and will be changed to a protected method
...
   public function setExtraColumns($table_name, array $column_names) {

needs follow-up and CR

Right, sorry.

I forgot about those because this is fairly internal code and I don't expect it to be heavily used directly.

On a second thought I'm not sure what to recommend in the CR: the new ::create() method is @internal and should not be used directly as well. The only legitimate way to instantiate a table mapping is still through the storage at the moment.

I mean that @deprecated needs link to CR https://www.drupal.org/core/deprecation#how-method

Yes, but I'm not sure what an hypothetical CR would day: those setters were kind of internal before...

Adding the deprecated tag to make sure we don't lose track of this issue and reference it in the CR, thanks for the reminder @andypost!