Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As a split-off from #2756417: Integration with the Paragraphs module, add support for Multiversion to Paragraphs, ultimately so that paragraph entities can be used with the Deploy module suite.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff_18-21.txt | 964 bytes | vacho |
#21 | paragraphs-n2780877-21.patch | 8.24 KB | vacho |
#20 | interdiff_17-18.txt | 976 bytes | vacho |
#18 | paragraphs-n2780877-18.patch | 8.16 KB | vacho |
#17 | paragraphs-n2780877-17.patch | 7.01 KB | vacho |
Comments
Comment #2
DamienMcKennaThis is jeqq's patch from comment 24 in #2756417 separated out.
Comment #3
DamienMcKennaComment #4
DamienMcKennaSome small fixes to namespaces, etc.
Comment #5
miro_dietikerI didn't check multiversion in detail, but agree that we should greatly integrate with nice staging workflows.
To unblock this issue, please help work on test coverage.
It also helps me to review the situation as i can see the exact process we try to cover.
Comment #6
GrimreaperHello,
I have tested the patch with the entity reference revisions patch from #2674882-12: Add Multiversion compatibility to Entity Reference Revisions (for Paragraphs, etc) and it does not work.
I have a few questions on the patch:
I don't understand why the patch needs to affect all content entity types.
Should this check on paragraph entity type must be done before?
Comment #7
vflirt CreditAttribution: vflirt commentedI have made a minor change in the patch as it seems to be just copied over from the linked issue. The paragraph supports revision so the lines for the checks are removed.
Also the patched introduced a hidden dependency on entity_storage_migrate so I wrapped the storage_schema in a module exists check.
I have not touched any of the other code. I have tested and this seems to work, I got my node with the paragraphs transferred with deploy.
There is one other thing that needs work on: once the paragraphs got transferred then the workspace started showing conflict and when I try to view the conflict I got :
Error: Call to undefined method Drupal\paragraphs\Entity\Paragraph::getChangedTime() in Drupal\workspace\Controller\Component\ConflictListBuilder->buildRow() (line 158 of modules/contrib/workspace/src/Controller/Component/ConflictListBuilder.php).
Not sure if a 'changed' field should be added to paragraphs content type.
In order to make the deploy worked I had to update the "multiversion.setting" enabled_entity_types and add paragraph, then had to clear cache so multiversion could add the database fields and all to work out. I tried to use the multiversion drush command to enable paragraph but it failed with sql error that the needed database columns did not exist.
Comment #8
akoe CreditAttribution: akoe commentedTested the patch in combination with https://www.drupal.org/node/2756417 successfully.
Besides that i faced the same error
Error: Call to undefined method Drupal\paragraphs\Entity\Paragraph::getChangedTime()
in the conflicts list.Paragraphs intentionally removed its 'changed' field from Paragraph entities (https://www.drupal.org/node/2715855) and relies on the node change property. So im unsure what a solution would be:
* either adding an exception in the ConflictListBuilder for Paragraph entities, which wouldn't be in the line with the whole entity idea in my eyes or
* adding a method to Paragraph entity, which would be just a wrapper for the nodes 'changed' property.
Comment #9
miro_dietikerHm.. We should be clean here, it means somehow, someone is blindly relying on Paragraphs implementing EntityChangedInterface.
Plz add the method and check the stacktrace and figure out why this is called.
We might need to add that implementation to make us compatible with Multiversion. And we might want to implement this as a plain wrapper to the host changed method, because we introduce the host concept.
But the current caller should only try to call this method if the object implements EntityChangedInterface, thus check for it.
Comment #10
sinn CreditAttribution: sinn at Adyax commentedI've received error
Drupal\Core\Entity\EntityStorageException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'sgsh.paragraph_revision__field_sgsh_project_slider_items' doesn't exist: UPDATE {paragraph_revision__field_sgsh_project_slider_items}
due to system doesn't have "paragraph_revision__field_sgsh_project_slider_items" table. Proper table for my field revision table is "paragraph_r__b80457ef2b".
Part of patch #7
was updated using latest dev version.
Comment #11
sinn CreditAttribution: sinn at Adyax commentedFixed an use case described in https://www.drupal.org/node/2756417#comment-12201918 - not all entities have revision table and are supported by multiversion. Also added proper calling of Entity Field Manager.
Comment #12
afi13 CreditAttribution: afi13 commentedOK for me.
Comment #14
miro_dietikerNew functionality without tests can't be RTBC here. Didn't look further into the requirement / solution.
Comment #15
BerdirJust use ParagraphsStorage::class, then PHP takes care of the namespacing for you.
Comment #16
vacho CreditAttribution: vacho at Skilld commentedComment #17
vacho CreditAttribution: vacho at Skilld commentedPatch rerolled
Comment #18
vacho CreditAttribution: vacho at Skilld commentedSolving test problems
Comment #19
johnchque@vacho please upload interdiffs along with your patches.
Comment #20
vacho CreditAttribution: vacho at Skilld commentedInterdiff but strangely this not solve the problem at test.
Comment #21
vacho CreditAttribution: vacho at Skilld commentedTrying to solve test problems again.
Comment #22
BerdirI don't understand this, this test will never have multiversion enabled, as a test specifically controls which modules are enabled.
What we would need is a specific test that tests things with that other module.
you are removing the moderation handler code and that's not correct. I don't know if it's required for multiversion (doubt it?) but then it would need to go inside an else condition.
these classes should have specific names/namespaces to make clear that it is about the multiversion module.