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

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2947351-test-only.patch3.02 KB
new 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
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 Burgas
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 commented
StatusFileSize
new 2947351-6.patch4.84 KB
new interdiff-6.txt1.44 KB

Sure, why not :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

Thank you!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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 🇪🇺🌍
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 🇪🇺🌍
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.