Problem/Motivation

Since #2916018: Allow the table mapping to be initialized from outside the storage, the table mapping is responsible for generating the default table names for a content entity type, but SqlContentEntityStorage was not updated to get the table names from there.

Proposed resolution

Make SqlContentEntityStorage get its table names from the table mapping.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#15 interdiff-15.txt1.46 KBamateescu
#15 2981220-15.patch11.21 KBamateescu
#12 interdiff-12.txt1.5 KBamateescu
#12 2981220-12.patch11.2 KBamateescu
#9 interdiff-9.txt3.26 KBamateescu
#9 2981220-9.patch9.99 KBamateescu
#9 2981220-9-test-only.patch2.71 KBamateescu
#6 interdiff-6.txt1.11 KBamateescu
#6 2981220-6.patch7.61 KBamateescu
#4 interdiff-4.txt4.19 KBamateescu
#4 2981220-4.patch7.59 KBamateescu
#2 2981220.patch3.41 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2981220.patch3.41 KB

This should do it.

Status: Needs review » Needs work

The last submitted patch, 2: 2981220.patch, failed testing. View results

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2981220-4.patch7.59 KB
new interdiff-4.txt4.19 KB

This should fix the tests.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Category: Bug report » Task
Status: Needs review » Needs work

Nothing is broken here. It is just incomplete. This seems like a task not a bug.

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -245,6 +245,54 @@ public static function create(ContentEntityTypeInterface $entity_type, array $st
+   * @return string
...
+   * @return string
...
+   * @return string

These can be null as well.

amateescu’s picture

amateescu commented
Category: Task » Bug report
Status: Needs work » Needs review
StatusFileSize
new 2981220-6.patch7.61 KB
new interdiff-6.txt1.11 KB

As mentioned in the IS, the table mapping is now responsible for generating the table names from a set of entity type and field storage definitions, but when you set the table mapping in the storage through \Drupal\Core\Entity\Sql\SqlContentEntityStorage::setTableMapping(), the internal table name variables of the storage are not updated. That is a bug.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Looks good to me. Seems like we could write a test for #6, no? Otherwise seems RTBC to me.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

That is a bug.

So can we have a test only patch?

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -247,7 +247,7 @@ public static function create(ContentEntityTypeInterface $entity_type, array $st
   /**
    * Gets the base table name.
    *
-   * @return string
+   * @return string|null
    *   The table name.

I don't think base table can be null for SQLStorage.

amateescu’s picture

amateescu commented
StatusFileSize
new 2981220-9-test-only.patch2.71 KB
new 2981220-9.patch9.99 KB
new interdiff-9.txt3.26 KB

Sure.

The last submitted patch, 9: 2981220-9-test-only.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Thanks, looks great now.

amateescu’s picture

amateescu commented
StatusFileSize
new 2981220-12.patch11.2 KB
new interdiff-12.txt1.5 KB

I found a few more places that are getting the base table name from the entity type definition instead of the table mapping.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Nice! #12 will fix some fatals when not specifying the tables in the annotation, which is nice. Not sure if all of them, but that's for #2232465: Deprecate table names from entity definitions to find out. RTBC++

And thanks for your great effort, in particular the idea to split this off in the first place. 👍🏿

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Looks good!

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -184,22 +184,20 @@ protected function initTableLayout() {
+    $this->baseTable = $this->getTableMapping()->getBaseTable();
...
+      $this->revisionTable = $this->getTableMapping()->getRevisionTable();
...
+      $this->dataTable = $this->getTableMapping()->getDataTable();
...
+      $this->revisionDataTable = $this->getTableMapping()->getRevisionDataTable();

    Can we retrieve the table mapping just once? :) 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
@@ -40,11 +40,6 @@ class EntityTypedDataDefinitionTest extends KernelTestBase {
-    NodeType::create([

@@ -91,6 +86,11 @@ public function testFields() {
+    NodeType::create([

    Why is this needed?

amateescu’s picture

amateescu commented
StatusFileSize
new 2981220-15.patch11.21 KB
new interdiff-15.txt1.46 KB

Re #14:

1. Sure, done :)
2. Because that node type is only needed in EntityTypedDataDefinitionTest::testEntities() and creating it in the setUp() method breaks EntityTypedDataDefinitionTest::testEntityDefinitionIsInternal() somehow. See the first fail from #2.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Still looks good to me, thanks. 👍🏿

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Saving credit.

  • plach committed cf458f2 on 8.6.x 
    Issue #2981220 by amateescu, jibran, tstoeckler: SqlContentEntityStorage...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Reviewed & tested by the community » Fixed

Committed cf458f2 and pushed to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.