DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage

That sounds indeed like a bug which should be solved.

This blocks #2477899: Multiple valued Base fields won't work in Views and at least two contrib modules have to work around the problem (Dynamic Entity Reference and Entityqueue), so I'm bumping to major.

Here's the relevant portion of the patch from #2477899: Multiple valued Base fields won't work in Views and a unit test for it.

Looking into the failures for now.

Here is a fix for that.

Regarding the actual patch, this looked perfect for me. First I thought you renamed variables for the sake of it, but this was actually required as $definitions is reused later again.

This looks like a great solution. I remember from working on #2477899: Multiple valued Base fields won't work in Views that the $definitions rename was needed.

I think @plach_ should eyeball this other then that +1 for RTBC.

I think this looks good too. I've pinged @plach about this issue in IRC.

As far as I can see the patch looks good - it comes with a test that fails if the fix is not applied and the changes make sense. Also I think this is an obvious bug fix that belongs in 8.0.x.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -360,8 +360,8 @@ public function getTableMapping(array $storage_definitions = NULL) {
+        return $table_mapping->requiresDedicatedTableStorage($definition) && $definition->isBaseField();

Sorry, I don't understand this change: why are we excluding configurable fields?

Changing status given #11

@plach, I'm not sure either, it came from #2477899-11: Multiple valued Base fields won't work in Views - see the last paragraph of that comment.

If we remove that change from the patch and nothing breaks.. we're missing some more test coverage?

If we remove that change from the patch and nothing breaks.. we're missing some more test coverage?

I think we do: the previous patch was green and IIRC configurable fields are supposed to be returned.

IIRC configurable fields are supposed to be returned

Well.. in HEAD they are not :) If you try this snippet:

\Drupal::entityManager()->getStorage('node')->getTableMapping()->getFieldTableName('body');

you'll get a nice exception:

Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Table information not available for the 'body' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldTableName() (line 180 of core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

And with the patch, the correct table name is returned (i.e. node__body). Let's see if this small changes fixes all the failures from #14, and I'll work on adding an integration test for the default table mapping because phpunit tests can only take us so far...

After lots of digging around I finally have a firm answer for #12/#17: SqlContentEntityStorage::getTableMapping() is *not* supposed to return configurable fields, because of this line inside that method:

$definitions = $storage_definitions ?: $this->entityManager->getFieldStorageDefinitions($this->entityTypeId);

and \Drupal\Core\Entity\EntityFieldManager::getFieldStorageDefinitions() only returns base field storage definitions: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Entit...

So I went back to the patch from #7 and added a small integration test class for DefaultTableMapping. Interdiff is to #7.

+++ b/core/modules/system/src/Tests/Entity/DefaultTableMappingIntegrationTest.php
@@ -0,0 +1,75 @@
+class DefaultTableMappingIntegrationTest extends EntityUnitTestBase {

Please use the new base class: \Drupal\KernelTests\Core\Entity\EntityKernelTestBase

Please use the new base class: \Drupal\KernelTests\Core\Entity\EntityKernelTestBase

We need #2683391: Backport EntityKernelTestBase to 8.0.x if we want to do that.

Well, do we really need this patch for 8.0.x still?

Why not, 8.1.0 is still a month away :)

Well, you could use one base class for the tests for one version and the old one for 8.0.x

Yes, but it would be nicer to have a single patch for all three branches. Otherwise I wouldn't have bothered with #2683391: Backport EntityKernelTestBase to 8.0.x :)

Updated the patch to use EntityKernelTestBase.

Thank you @amateescu!

Thank you @amateescu for working on this patch. Assigning it to @plach to review it one final time.

Unrelated random test fail see #2609874: Boolean field "On label" and "Off label" are not translatable

Just a note I'm leaving this RTBC at the moment in case plach is able to re-review before commit.

Looks great and blocks entityqueue usage

Marking for @plach's subsystem maintainer review but leaving at RTBC as @catch said.

Discussed with @Cottser, @effulgentsia, @alexpott and we agreed to look at this as an RC target given its impact and the fact that it is risky for a patch release.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -360,8 +360,8 @@ public function getTableMapping(array $storage_definitions = NULL) {
   +        return $table_mapping->requiresDedicatedTableStorage($definition) && $definition->isBaseField();
   

   After lots of digging around I finally have a firm answer for #12/#17: SqlContentEntityStorage::getTableMapping() is *not* supposed to return configurable fields, because of this line inside that method:

   I'm sorry, but I'm not convinced by this argument: SqlContentEntityStorage::getTableMapping() *is* supposed to return configurable fields, in fact EntityFieldManager::getFieldStorageDefinitions() returns the body storage definition of the related configurable field. Besides of this, if we agree that user__roles should be part of the table mapping (and we do :), I don't see a reason on earth why node__body shouldn't be.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1587,7 +1591,12 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +    // The storage definition might be different stored to the one in the DB, so
   

   The first sentence sounds a bit weird, but maybe it's just me.

@plach, you're right, I only looked at the code of EntityFieldManager::getFieldStorageDefinitions() and it seemed to return only base fields, but after some manual testing I can confirm that it does return configurable fields as well.

So I added back the interdiffs from #14 and #19 to the latest patch and opened #2705205: Improve test coverage around updating table mapping after a new field storage definition is added for the problem discovered in #20.

Thanks! And here's a test-only patch

Patch looks good to me. I agree it shouldn't land in a patch release.

We also had one in #22 :P

The patch looks great to me now, and tests fail/succeed as expected.

:)

Pushed to 8.2.x and 8.1.x. I'm very happy that we waited for #40.1 to be addressed before committing this.

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -147,15 +147,16 @@ public function getFieldTableName($field_name) {
       $table_names = array(
         $storage->getDataTable(),
         $storage->getBaseTable(),
         $storage->getRevisionTable(),
+        $this->getDedicatedDataTableName($storage_definition),
       );

This patch doesn't add getDedicatedRevisionTableName(), which I think is okay, because the function returns the first matching table, and I don't think you can be in a situation where you wouldn't match the data table and match the revision table only. That said, is that logic also true for getRevisionTable(), in which case, should we remove that too? Or else, add a comment explaining why we have one but not the other? Either way, non-release-blocking follow-up material.

That said, is that logic also true for getRevisionTable(), in which case, should we remove that too? Or else, add a comment explaining why we have one but not the other? Either way, non-release-blocking follow-up material.

The current logic should be fine because we can have fields living only in the revision table, e.g. revision_log, while, as you were pointing out, we can't have a field living in a dedicated revision table while not living in the dedicated data table. A comment explaining this would definitely be helpful :)