Problem/Motivation

For revisionable entity types, core's SQL entity storage always creates dedicated data and revision tables for configurable and multi-valued base fields.

However, DefaultTableMapping::getTableNames() does not return the revision table name for multi-valued base fields of a revisionable entity type.

Proposed resolution

Fix \Drupal\Core\Field\BaseFieldDefinition::isRevisionable() to return TRUE when the base field has multiple values.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope, the storage correctly creates the table already.

CommentFileSizeAuthor
#6 interdiff-6.txt1.44 KBamateescu
#6 2947351-6.patch4.84 KBamateescu
#2 2947351.patch3.66 KBamateescu
#2 2947351-test-only.patch3.02 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2947351-test-only.patch3.02 KB
2947351.patch3.66 KB

This patch should fix it.

I looked into #2497737: Entity type revisionability is not taken into account when switching field revisionability first, but it's not the same problem.

The last submitted patch, 2: 2947351-test-only.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Makes sense, we currently always create revision tables for configurable fields as you said.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented 
+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -232,7 +232,9 @@ public function setTranslatable($translatable) {
+    // Multi-valued base fields are always considered revisionable, just like
+    // configurable fields.

What about adding this to the documentation of the methods \Drupal\Core\Field\FieldStorageDefinitionInterface::isRevisionable() and \Drupal\Core\Field\BaseFieldDefinition::setCardinality()?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2947351-6.patch4.84 KB
interdiff-6.txt1.44 KB

Sure, why not :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

Thank you!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

Committed 314ea3c and pushed to 8.6.x. Thanks!

Left open on 8.5.x for backport once 8.5.0 is out.

  • alexpott committed 314ea3c on 8.6.x 
    Issue #2947351 by amateescu, hchonov: DefaultTableMapping does not...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Patch (to be ported)

Setting the issue to patch to be ported for cherry-pick once 8.5.0 is out. let's not clog the rtbc queue and ptbp makes them easy to find once 8.5.0 is out.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Patch (to be ported) » Fixed

Committed 9a3a334 and pushed to 8.5.x. Thanks!

  • alexpott committed 606c617 on 8.5.x 
    Issue #2947351 by amateescu, hchonov: DefaultTableMapping does not...

Status: Fixed » Closed (fixed)

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