Problem/Motivation

When we added the parent fields, we didn't expect that paragraphs are moved around and that that information could change between revisions.

But with new features like drag & drop and library, this can actually happen and then the old revisions would also have the new parent, which might break access and other things when looking at old revisions and other, worse problems.

Proposed resolution

Make the fields revisionable, write an update function that creates the revisionable storage and adds the value to the old revisions.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Berdir created an issue. See original summary.

johnchque’s picture

Assigned: Unassigned » johnchque

Working on this.

johnchque’s picture

Status: Active » Needs review
StatusFileSize
new4.51 KB

Seems to fall when having translation, will keep testing.

Status: Needs review » Needs work

The last submitted patch, 3: parent_fields_revisionable-2904231-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new4.62 KB
new1.08 KB

Tested again many times, it seems this is not the right way to follow. The revisions are all null and we just get one revision of each paragraph after running the updates.

Status: Needs review » Needs work

The last submitted patch, 5: parent_fields_revisionable-2904231-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new5.54 KB
new6.59 KB

OK, first working version. Haven't thrown any warning/error and the fields seem to be updated accordingly. :)

Status: Needs review » Needs work

The last submitted patch, 7: parent_fields_revisionable-2904231-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new6.51 KB
new772 bytes

Strange, one of the test that fails is passing locally. Testing again.

Status: Needs review » Needs work

The last submitted patch, 9: parent_fields_revisionable-2904231-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new7.82 KB
new2.04 KB

Pretty strange, tried with multiple versions of EB and they pass locally. Adding some debugs to see the page content and see what is happening.

Status: Needs review » Needs work

The last submitted patch, 11: parent_fields_revisionable-2904231-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new7.9 KB
new1.99 KB

Let's fix these tests :) :)

Status: Needs review » Needs work

The last submitted patch, 13: parent_fields_revisionable-2904231-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new9.63 KB
new2.51 KB

Trying once again.

johnchque’s picture

Sorry, the patch included fixes for tests that will be done in #3026890: Update to Entity Browser 8.x-2.x in tests. :)

Status: Needs review » Needs work

The last submitted patch, 16: parent_fields_revisionable-2904231-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
berdir’s picture

+++ b/paragraphs.install
@@ -255,3 +255,91 @@ function paragraphs_update_8015() {
+    // Update the field storage repository.
+    \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
+    $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+    $last_installed_schema = \Drupal::service('entity.last_installed_schema.repository');
+
+    $storage_definition = $definition_update_manager->getFieldStorageDefinition('parent_id', 'paragraph');
+    $storage_definition->setRevisionable(TRUE);
+    $last_installed_schema->setLastInstalledFieldStorageDefinition($storage_definition);
+    $storage_definition = $definition_update_manager->getFieldStorageDefinition('parent_type', 'paragraph');
+    $storage_definition->setRevisionable(TRUE);
+    $last_installed_schema->setLastInstalledFieldStorageDefinition($storage_definition);
+    $storage_definition = $definition_update_manager->getFieldStorageDefinition('parent_field_name', 'paragraph');
+    $storage_definition->setRevisionable(TRUE);
+    $last_installed_schema->setLastInstalledFieldStorageDefinition($storage_definition);
+
+    // Update the stored field schema.
+    // @todo: There has to be a better way to do this.
+    $key = 'paragraph.field_schema_data.parent_id';
+    $field_schema = \Drupal::keyValue('entity.storage_schema.sql')->get($key);
+    $field_schema['paragraphs_item_revision_field_data']['fields']['parent_id'] = $column_schema;
+    \Drupal::keyValue('entity.storage_schema.sql')->set($key, $field_schema);
+    $key = 'paragraph.field_schema_data.parent_type';
+    $field_schema = \Drupal::keyValue('entity.storage_schema.sql')->get($key);
+    $field_schema['paragraphs_item_revision_field_data']['fields']['parent_type'] = $column_schema;
+    \Drupal::keyValue('entity.storage_schema.sql')->set($key, $field_schema);
+    $key = 'paragraph.field_schema_data.parent_field_name';
+    $field_schema = \Drupal::keyValue('entity.storage_schema.sql')->get($key);
+    $field_schema['paragraphs_item_revision_field_data']['fields']['parent_field_name'] = $column_schema;
+    \Drupal::keyValue('entity.storage_schema.sql')->set($key, $field_schema);
+  }
+  else {
+    $sandbox['#finished'] = ($sandbox['current'] / $sandbox['total']);
+  }

I think we could put this stuff in a loop for the 3 fields.

We could check if https://www.drupal.org/project/drupal/issues/2984782 makes this easier, but then we have to add a dependency on 8.7 for this and wait until May or so.

berdir’s picture

Status: Needs review » Needs work

Tested the patch on a site with 800k paragraphs, took about 2min, that is better than I expected.

However, drush entup still reports that those fields need to be changed, which means that something has not been properly updated yet.

berdir’s picture

Priority: Normal » Critical
berdir’s picture

What's failing is this check "$storage_definition->isRevisionable() != $original->isRevisionable()" in "\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresFieldStorageSchemaChanges", $original still says revisionable FALSE.

The code that we have should fix that, but it looks like something doesn't work.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new5.44 KB
new4.78 KB

@Berdir was right! We added complexity by trying to do everything at once. This should work better. :)

johnchque’s picture

This should be included too.

johnchque’s picture

Refactoring the code thanks to @Berdir. This should work better. :)

johnchque’s picture

Hmmm, it seems they had different limits.

johnchque’s picture

While testing with many paragraphs I found out that this was missing.

berdir’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/paragraphs.install
@@ -346,8 +346,8 @@
-
-  drupal_set_message($sandbox['current'] . ' nodes processed.');
+  $sandbox['#finished'] = ($sandbox['current'] / $sandbox['total']);
+  return t('@count nodes processed.', ['@count' => $sandbox['current']]);
 }
 

We are processing paragraphs, not nodes ;)

Can be fixed on commit, this seems to be working well, also on very large installations.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Committed while fixing the string.

Status: Fixed » Closed (fixed)

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