Problem/Motivation

Currently entity type revisionability is not taken into account when determining whether marking fields as revisionable should trigger db updates. In fact, if the entity is not revisionable switching field revisionability shouldn't affect the final schema, at least for the default SQL storage.

Additionally, a field should be considered revisionable if and only if both the entity type and the storage definition are marked as revisionable, which is not the case currently.

Proposed resolution

  • Mark sure updates are not triggered when switching field revisionability for non-revisionable entity types.
  • Make sure both entity and field revisionability are taken into account when in code dealing with revisionable data.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Review it

User interface changes

None

API changes

Switching field revisionability no longer triggers entity definition updates when the entity type is not revisionable.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because updates are not triggered correctly when switching field revisionability.
Issue priority Major because this affects all core non-revisionable entity types.
Unfrozen changes Unfrozen because it is a bug fix.
Disruption Dedicated field revision table will no longer be created for non revisionable bundle fields attached to revisionable entity types (definitely an edge case).
CommentFileSizeAuthor
#11 entity-revisionable_fields-2497737-11.patch28.56 KBplach
#9 entity-revisionable_fields-2497737-9.patch28.5 KBplach
#9 entity-revisionable_fields-2497737-9.interdiff.txt2.83 KBplach
#4 entity-revisionable_fields-2497737-4.patch15.02 KBplach
#4 entity-revisionable_fields-2497737-4.interdiff.txt811 bytesplach
#4 entity-revisionable_fields-2497737-4-test.patch8.33 KBplach
#1 entity-revisionable_fields-2497737-1.patch14.23 KBplach
#1 entity-revisionable_fields-2497737-1-test.patch7.54 KBplach

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +D8 upgrade path
StatusFileSize
new entity-revisionable_fields-2497737-1-test.patch7.54 KB
new entity-revisionable_fields-2497737-1.patch14.23 KB

This provides only the bug fix. Core field definitions revisionability is not switched yet.

The last submitted patch, 1: entity-revisionable_fields-2497737-1-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: entity-revisionable_fields-2497737-1.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Title: Make sure all potentially revisionable field definitions are marked as such » All potentially revisionable field definitions should be marked as such
Status: Needs work » Needs review
StatusFileSize
new entity-revisionable_fields-2497737-4-test.patch8.33 KB
new entity-revisionable_fields-2497737-4.interdiff.txt811 bytes
new entity-revisionable_fields-2497737-4.patch15.02 KB

Fixed test failure

The last submitted patch, 4: entity-revisionable_fields-2497737-4-test.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Title: All potentially revisionable field definitions should be marked as such » Entity type revisionability is not taken into account when switching field revisionability
Assigned: plach » Unassigned
Issue summary: View changes

I decided to rescope this issue as marking field definitions as revisionable does not make sense unless definitions for revision metadata are added, in fact without them an entity type cannot be marked as revisionable.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue summary: View changes

Updated IS, reviews welcome.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
@@ -604,4 +602,141 @@ public function testEntityTypeSchemaUpdateAndRevisionableBaseFieldCreateWithoutD
+    $definition->setRevisionable(TRUE);
+    $this->assertFalse($this->entityDefinitionUpdateManager->needsUpdates(), 'Switching base field revisionability on a non-revisionable entity type does not trigger updates.');

Its interesting that its possible to have revisionable fields on non revisionable entity types and the other way round.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1331,6 +1331,7 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
+      $revisionable = $this->entityType->isRevisionable() && $storage_definition->isRevisionable();

Given that this appears multiple times in the patch, I'm curious whether it would be worth to encapsulate this logic somewhere?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new entity-revisionable_fields-2497737-9.interdiff.txt2.83 KB
new entity-revisionable_fields-2497737-9.patch28.5 KB

Its interesting that its possible to have revisionable fields on non revisionable entity types and the other way round.

Well, revisionable for field storage definitions means "potentially revisionable", that is the whole point of this issue. Unless the entity type is revisionable, the revisionable property can be ignored, but once an entity type is made revisionable, having already decided which fields makes sense to revision allows to proceed smoothly.

Viceversa, if an entity type is revisionable we may still want to have non-revisionable fields, like the node type or UUID.

Status: Needs review » Needs work

The last submitted patch, 9: entity-revisionable_fields-2497737-9.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new entity-revisionable_fields-2497737-11.patch28.56 KB

Rerolled, this probably needs an upgrade path now.

Status: Needs review » Needs work

The last submitted patch, 11: entity-revisionable_fields-2497737-11.patch, failed testing.

The last submitted patch, 11: entity-revisionable_fields-2497737-11.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Priority: Major » Normal
Related issues: +#2705389: Selected content entities should extend EditorialContentEntityBase or RevisionableContentEntityBase, +#2721313: Upgrade path between revisionable / non-revisionable entities

This was discussed a few days ago with @alexpott, @catch, @cilefen, @effulgentsia, @xjm and the entity and field system maintainers while triaging major issues. We agreed that this does not meet the criteria for a major bug, since there is a workaround for it: making the entity type revisionable while making field definitions revisionable. This is precisely what the Workflow initiative is aiming to do for core (see #2705389: Selected content entities should extend EditorialContentEntityBase or RevisionableContentEntityBase and #2721313: Upgrade path between revisionable / non-revisionable entities).

We agreed it would still be good to fix this as proposed.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue tags: -D8 upgrade path +Upgrade path

Updating tag.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.