I do not know whether this is out of scope but did anyone ever consider integrating with the paragraphs module?
At the moment I am building a replication workflow with nodes having paragraphs attached to them. For this cause I had to create a paragraphs storage class using ContentEntityStorageTrait and implementing a custom database update method to keep its parent's target_revision_id in sync with the paragraph's revision_id (else any change to the parent node entity through Entity API would trigger ContentEntityStorageTrait's save logic and create a new revision each time) which is naturally prone to error.
Also I had to replace the EntityReferenceRevisions field type class (which the paragraphs module uses) with my own implementation which has a similar functionality to multiversion's EntityReferenceItem class. I think creating a trait and using it in the EntityReferenceRevisions and EntityReferenceItem classes would be a great follow-up issue.

Files: 
CommentFileSizeAuthor
#78 multiversion-paragraphs-2756417-78.patch1.92 KBjeqq
#64 interdiff-63-64.txt484 bytesjeqq
#64 multiversion-paragraphs-2756417-64.patch21.42 KBjeqq
#63 interdiff-58-63.txt2.34 KBjeqq
#63 multiversion-paragraphs-2756417-63.patch21.3 KBjeqq
#58 interdiff.txt3.54 KBjeqq
#58 multiversion-paragraphs-2756417-58.patch22.06 KBjeqq
#57 source data.png63.47 KBPapaGrande
#57 target data.png50.75 KBPapaGrande
#54 paragraphs-field-values-on-target-site.png10.81 KBjeqq
#54 paragraph-field-source-site.png14.75 KBjeqq
#49 integration_with-2756417-49.patch21.32 KBundertext
#46 integration_with-2756417-40.patch19.87 KBundertext
#44 integration_with-2756417-39.patch19.67 KBundertext
#42 integration_with-2756417-38.patch19.4 KBundertext
#39 integration_with-2756417-37.patch19.4 KBundertext
#37 integration_with-2756417-36.patch19.37 KBundertext
#35 integration_with-2756417-35.patch19.38 KBundertext
#9 multiversion-n2756417-9.interdiff.txt4.84 KBDamienMcKenna

Comments

alexej_d created an issue. See original summary.

DamienMcKenna’s picture

Might you be able to share some of your code so we could review it? Thanks.

alexej_d’s picture

Here is a link to my branch on Github. The code is a bit messy because I was short on time. But it seems so be working now. You also need to patch replication modules ContentEntityNormalizer for it to set the target_revision_id for entity_reference_revisions fields on denormalization.

DamienMcKenna’s picture

Title: Integration with paragraphs module » Integration with Paragraphs module

The fork isn't adding the appropriate record to the entity_reference_revisions field, is that what you're talking about? It is finally copying the Paragraph's field data though, but without the entity_reference_revisions record it's like a detached limb.

DamienMcKenna’s picture

I made a PR of your branch's changes: https://github.com/dickolsson/drupal-multiversion/pull/89

DamienMcKenna’s picture

The branch *does* handle revisions properly within the paragraph entities, which is good..

alexej_d’s picture

That's what I meant. You need to patch the ContentEntityNormalizer in the Replication module. Add this after this line here and it should work properly:

if($type === 'entity_reference_revisions') {
    $translation[$field_name][$delta]['target_revision_id'] = $target_entity->revision_id->value;
}
DamienMcKenna’s picture

DamienMcKenna’s picture

Here are @alexej_d's changes in patch format with minor code formatting changes.

Status: Needs review » Needs work

The last submitted patch, 9: multiversion-n2756417-9.patch, failed testing.

The last submitted patch, 9: multiversion-n2756417-9.patch, failed testing.

The last submitted patch, 9: multiversion-n2756417-9.patch, failed testing.

DamienMcKenna’s picture

To the maintainers - what should we do about tests? I'd love to get some added, but I'm not sure where they should go given the scenario involves about six modules (full Deploy suite plus Paragraphs and Entity Reference Revisions).

DamienMcKenna’s picture

Every single test passes locally:

$ simpletest8 --verbose multiversion

Drupal test run
---------------

Tests to be run:
  - Drupal\multiversion\Tests\CommentStatisticsTest
  - Drupal\multiversion\Tests\ConflictTrackerTest
  - Drupal\multiversion\Tests\EntityQueryTest
  - Drupal\multiversion\Tests\EntityStorageTest
  - Drupal\multiversion\Tests\MenuLinkTest
  - Drupal\multiversion\Tests\MigrationTest
  - Drupal\multiversion\Tests\MultiversionManagerTest
  - Drupal\multiversion\Tests\RevisionFieldTest
  - Drupal\multiversion\Tests\RevisionTreeIndexTest
  - Drupal\multiversion\Tests\SequenceIndexTest
  - Drupal\multiversion\Tests\UninstallTest
  - Drupal\multiversion\Tests\UuidIndexHooksTest
  - Drupal\multiversion\Tests\UuidIndexTest
  - Drupal\multiversion\Tests\Views\DeletedTest
  - Drupal\multiversion\Tests\Views\WorkspaceTest
  - Drupal\multiversion\Tests\WorkspaceTest
  - Drupal\relaxed\Tests\StubTest
  - Drupal\Tests\multiversion\Kernel\MultiversionIndexFactoryTest
  - Drupal\Tests\multiversion\Unit\DefaultWorkspaceNegotiatorTest
  - Drupal\Tests\multiversion\Unit\WorkspaceManagerTest

Test run started:
  Tuesday, August 2, 2016 - 21:55

Test summary
------------

Drupal\multiversion\Tests\CommentStatisticsTest               43 passes                           10 messages
Drupal\multiversion\Tests\ConflictTrackerTest                 17 passes                            2 messages
Drupal\multiversion\Tests\EntityQueryTest                     75 passes                            2 messages
Drupal\multiversion\Tests\EntityStorageTest                  351 passes                            2 messages
Drupal\multiversion\Tests\MenuLinkTest                        13 passes                            2 messages
Drupal\multiversion\Tests\MigrationTest                       57 passes                                      
Drupal\multiversion\Tests\MultiversionManagerTest             35 passes                                      
Drupal\multiversion\Tests\RevisionFieldTest                  162 passes                            4 messages
Drupal\multiversion\Tests\RevisionTreeIndexTest               42 passes                            4 messages
Drupal\multiversion\Tests\SequenceIndexTest                   22 passes                            2 messages
Drupal\multiversion\Tests\UninstallTest                       15 passes                            2 messages
Drupal\multiversion\Tests\UuidIndexHooksTest                  13 passes                            2 messages
Drupal\multiversion\Tests\UuidIndexTest                       15 passes                            2 messages
Drupal\multiversion\Tests\Views\DeletedTest                   30 passes                            6 messages
Drupal\multiversion\Tests\Views\WorkspaceTest                 23 passes                            4 messages
Drupal\multiversion\Tests\WorkspaceTest                       83 passes                           16 messages
Drupal\relaxed\Tests\StubTest                                  5 passes                                      
Drupal\Tests\multiversion\Kernel\MultiversionIndexFactoryTes   1 passes                                      
Drupal\Tests\multiversion\Unit\DefaultWorkspaceNegotiatorTes   2 passes                                      
Drupal\Tests\multiversion\Unit\WorkspaceManagerTest            6 passes                                      

Test run duration: 12 min 55 sec
DamienMcKenna’s picture

Lets see if adding a test dependency for entity_reference_revisions will fix the problem.

Status: Needs review » Needs work

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

jeqq’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

The last submitted patch, 15: multiversion-n2756417-15.patch, failed testing.

jeqq’s picture

@DamienMcKenna, @alexej_d Thank you for working on this!

There should be a check if the field exists, otherwise it always will require the Entity Reference Revisions module in EntityReferenceRevisionsItem.

jeqq’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Thanks for working out what the problem was with the tests. One minor thing - I don't think there's anything wrong with adding entity_reference_revisions as a test dependency, it'll make it easier to add tests for the integration.

DamienMcKenna’s picture

IMHO it might be worth adding test_dependencies lines for each of the supported modules - entity_reference_revisions, uuid, media, etc. That way when someone writes tests for them you won't run into chicken-and-egg problems because of the new test_dependencies. (see https://www.mediacurrent.com/blog/how-fix-testbot-bug-when-adding-new-tests for more details)

dixon_’s picture

In general I would like contrib modules to integrate with Multiversion within their own modules (not the other way around as with this patch). This makes more sense as Multiversion will get into core in the comings months and cease to exists. See #2721129: Workflow Initiative.

So it's better that the integrations live where they can be fully maintained moving forward.

However, Paragraph module have a lot of usage and I could be ok with committing this patch if someone goes ahead and writes decent test coverage for this patch :)

dixon_’s picture

Issue tags: +Needs tests
DamienMcKenna’s picture

Title: Integration with Paragraphs module » Integration with the Multiversion module
DamienMcKenna’s picture

Title: Integration with the Multiversion module » Integration with the Paragraphs module
Grimreaper’s picture

Status: Needs review » Needs work

Hello,

I have tested patches from the issues #2780877-4: Add Multiversion support to Paragraphs and #2674882-7: Add Multiversion compatibility to Entity Reference Revisions (for Paragraphs, etc). It does not work.

I have tested on Drupal 8.1.10 and with the dev version of paragraphs and the deploy suite.

Also it seems that the patch introduce a dependency on the entity_storage_migrate module, no?

friera’s picture

The patch doesn't work for me.

undertext’s picture

Here is the patch for paragraphs integration, new one, with another approach.

Small description what have been done:

The «Entity reference revisions» field is like «Entity reference» field with extra handling.
This extra handling is related to fact that "entity reference revisions» fields assume that referenced entity can store information about its parent. So Paragraphs entity has columns «parent_id» and «parent type» in the database.
And the logic of the postSave() method in EntityReferenceRevisionsItem.php is pretty simple - update the entity with parent information without creating new revision after the parent entity has been saved . The more clear explanation:
1) We create new node with new paragraph field
2) When paragraph field is saving it creates paragraph entity without parent information (because we do not know the parent node id on this step)
3) Node is saved, know we know the parent id.
4) Paragraph entity is updated in postSave with parent node information.

The current multiversion storage forces new revision on every save() call. But we do not need this on postSave.
So the method has been added to ContentEnityStorageInterface and ContentEntityStorageTrait - saveWithoutForcingNewRevision().

Also the default EntityReferenceRevisionsItem class was replaced with multiversion-supported version.
The changes comparing to default EntityReferenceRevisionsItem class are next:
save() call was replaced by saveWithoutForcingNewRevision() call.
Added code for supporting stubs and handling references to already created nodes.

undertext’s picture

Status: Needs work » Needs review
undertext’s picture

Change namespace of test to right one.

Status: Needs review » Needs work

The last submitted patch, 37: integration_with-2756417-36.patch, failed testing.

undertext’s picture

Add @requires annotation for test bot.

undertext’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: integration_with-2756417-37.patch, failed testing.

undertext’s picture

Status: Needs work » Needs review
FileSize
19.4 KB

Try one more time.

Status: Needs review » Needs work

The last submitted patch, 42: integration_with-2756417-38.patch, failed testing.

undertext’s picture

Status: Needs work » Needs review
FileSize
19.67 KB

Ok, changing patch according to this one
https://www.drupal.org/node/2692407#comment-11833737

Status: Needs review » Needs work

The last submitted patch, 44: integration_with-2756417-39.patch, failed testing.

undertext’s picture

Assigned: Unassigned » undertext
Status: Needs work » Needs review
FileSize
19.87 KB

Adding missing drupal modules dependencies to composer.json.

akoe’s picture

Tested successfully in combination with https://www.drupal.org/node/2780877

PapaGrande’s picture

The patch isn't working for me. In the source node, the paragraph field revisions are getting double-saved in the revision tables. It looks fine in the UI. However, when I deploy the node to a different server, the data comes over and the node displays correctly on the first replication, but subsequent updates in the paragraph fields are not shown in the UI.

+++ b/src/EntityReferenceRevisionsItem.php
@@ -0,0 +1,120 @@
+        $storage->saveWithoutForcingNewRevision($entity);

The call to saveWithoutForcingNewRevision() seems to be the second save of the field data. Disabling it prevents the second save on the source node, but it doesn't fix replication to the destination node.

I'm also getting null values for the parent fields in the paragraphs_item_field_data table.

undertext’s picture

@PapaGrande Do you try this patch in combination with https://www.drupal.org/node/2780877 ?
If yes - try to test the replication only with this patch applied.

The problem with double-saved revision should be fixed in updated patch.

jeqq’s picture

Status: Needs review » Needs work
jeqq’s picture

Status: Needs work » Needs review
jeqq’s picture

I've tested with the patch from #46 (just this patch), on replication, paragraphs are not replicated correctly, there are missing fields on target site.

Testing now #49.

jeqq’s picture

@undertext Could you please upload the interdiff file for next patches, to be easier to review them?

Thank you for your work on this!

jeqq’s picture

Tested replication with the patch from #49.

I have a Paragraphs type with 2 fields: text and number (both can have multiple values), number field has 2 default values: 777 and 333. For default content type - Article, added a new field, the paragraphs field created before.

Created a new article and added multiple values for paragraphs field. Field values on source site:

source

Field values on target site (after replication):

target

As you see in the last image, on the target site I have values just for the Number field (these are the default field values).

I assume this is normalization/denormalization problem. Probably fields values are sent to target site but they are not denormalized correctly.

alexej_d’s picture

@jeqq check out comment #7

jeqq’s picture

@alexej_d We already have that code in denormalizer, here.

PapaGrande’s picture

FileSize
50.75 KB
63.47 KB

@undertext I tried #49 and the field data is still doubled with and without the patch from https://www.drupal.org/node/2780877#comment-11985327.

I'm using this query to debug:

SELECT 
  node__field_tile_list.entity_id AS nid, 
  node__field_tile_list.field_tile_list_target_id, 
  node__field_tile_list.field_tile_list_target_revision_id, 
  paragraph_revision__field_description.entity_id AS para_id, paragraphs_item_field_data.id, 
  paragraph_revision__field_description.revision_id AS field_description_revision_id, 
  paragraph_revision__field_description.entity_id AS field_description_id, 
  paragraph_revision__field_description.field_description_value,
  paragraphs_item_field_data.parent_id,
  paragraphs_item_field_data.parent_type,
  paragraphs_item_field_data.parent_field_name
FROM paragraph_revision__field_description
JOIN paragraphs_item_field_data ON paragraph_revision__field_description.entity_id = paragraphs_item_field_data.id
JOIN node__field_tile_list ON node__field_tile_list.field_tile_list_target_id = paragraphs_item_field_data.id
WHERE node__field_tile_list.entity_id = 993;

(The node paragraph field is 'field_tile_list' and an example paragraph field is 'field_description'.)

Notice in the "target data.png" screen shot how the target data has the initial value "test 1.0" saved again so it's a later revision than what should show, "test 1.1".

Also, note that there are more revisions on the source ("source data.png") than the target.

I do have both workbench_moderation and content_translation enabled so they may be contributing to the problem.

Finally, can I request that we prefix the patch file names with "multiversion", per https://www.drupal.org/patch/submit#patch_naming?

jeqq’s picture

Added some assertions to check if the parent's default revision is correct when saving it multiple times. The test should fail now (the problem is in Drupal\multiversion\EntityReferenceRevisionsItem). Fixed some typos and codding standards.

Status: Needs review » Needs work

The last submitted patch, 58: multiversion-paragraphs-2756417-58.patch, failed testing.

PapaGrande’s picture

Thanks, @jeqq for the quick turnaround, but unfortunately #58 didn't fix the double-saving in my setup. What info can I provide to help debug?

Edit: I just re-read the comment in #58 a bit more slowly this time, and realized that it doesn't fix the problem, but makes it easier to debug with a test. Thanks for that.

+++ b/src/EntityReferenceRevisionsItem.php
@@ -0,0 +1,120 @@
+    $this->entityReferencePresave();

Small nit: this should be entityReferencePreSave to match the alias.

jeqq’s picture

Status: Needs work » Needs review

@PapaGrande I'm working on a fix for this.

jeqq’s picture

Status: Needs review » Needs work

Accidentally changed the status. Changing it back to Needs work.

jeqq’s picture

This fixes the duplicate save. Also removed the tests I've added in #58, those assertion are failing because of other bug I've found in Multiversion. That bug affects this issue (it should fix the problem with the default revision), so marking this as postponed until I'll commit the fix for it.

Edit: The paragraph is still saved twice but just when saved for the first time.

jeqq’s picture

Delete paragraph when the parent entity is deleted.

This works if used with https://github.com/dickolsson/drupal-multiversion/pull/100 (this is the fix I wrote about in #63).

jeqq’s picture

Status: Postponed » Needs review

  • jeqq committed 41295b2 on 8.x-1.x authored by undertext
    Issue #2756417 by undertext, jeqq, DamienMcKenna, PapaGrande:...

The last submitted patch, 63: multiversion-paragraphs-2756417-63.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 64: multiversion-paragraphs-2756417-64.patch, failed testing.

PapaGrande’s picture

@jeqq Thanks for patch #64 and the one from Github, but I'm still getting double saves of paragraph fields.

dixon_’s picture

@PapaGrande Can you provide more details around what Paragraph types and fields you are experiencing problems with? The more details the better :)

Edit: It helps to read the issue history :) I see you provided lots of great details already. Thanks!

jeqq’s picture

@PapaGrande when it gets double saved? When I've tested it, the paragraph was double saved just on first node save, next saves where ok - one revision per save.

PapaGrande’s picture

@jeqq I see double saves of revision values in the paragraph fields every time the parent node is saved, both on the source when I save the node and on the target every time the node is replicated.

timmillwood’s picture

Status: Needs work » Fixed

see #66

PapaGrande’s picture

Status: Fixed » Needs work

With respect, this isn't fixed for me. See #72 and my other comments.

jeqq’s picture

I'm investigating #72. This is not fixed yet.

jeqq’s picture

@PapaGrande Please pull recent changes from Replication module and test again. I think that will fix the problem with duplicates.

jeqq’s picture

I can reproduce duplicate save now just on replication with the external replicator (when Relaxed module is enabled), with internal replicator works fine. When I replicate now with the external replicator, on target workspace, for each field from paragraph I have 2 new revisions: first revision is the new revision that came from source and the second revision is the current revision that exists on target (it's saved as default). Investigating this.

jeqq’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

  • jeqq committed 711cc6e on 8.x-1.x
    Issue #2756417 by jeqq, PapaGrande: Integration with the Paragraphs...

  • jeqq committed deedbdd on 8.x-1.x
    Issue #2756417 by jeqq: Fix typo.
    
PapaGrande’s picture

@jeqq, thanks for the hard work. I'm still testing various use case scenarios--still having some issues with workbench_moderation statuses and translated nodes not replicating consistently--but the paragraph entities and fields so far are successfully replicating inserts and updates between the source and a remote target server.