Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

This is jeqq's patch from comment 24 in #2756417 separated out.

DamienMcKenna’s picture

Title: Add Multiversion support » Add Multiversion support to Paragraphs
DamienMcKenna’s picture

miro_dietiker’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I 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.

Grimreaper’s picture

Hello,

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:

  1. +++ b/paragraphs.module
    @@ -210,3 +210,43 @@ function template_preprocess_paragraph(&$variables) {
    +        // Make all content entity types revisionable.
    

    I don't understand why the patch needs to affect all content entity types.

  2. +++ b/paragraphs.module
    @@ -210,3 +210,43 @@ function template_preprocess_paragraph(&$variables) {
    +        switch ($entity_type->id()) {
    +          case 'paragraph':
    

    Should this check on paragraph entity type must be done before?

vflirt’s picture

I 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.

akoe’s picture

Tested 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.

miro_dietiker’s picture

Hm.. 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.

sinn’s picture

Status: Needs work » Needs review
FileSize
4.71 KB

I'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

+      // Address normal and revision tables.
+      foreach (['', '_revision'] as $table) {
+        $query = $db
+          ->update($parent_entity_type . $table . '__' . $parent_entity_field_name)

was updated using latest dev version.

sinn’s picture

Fixed 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.

afi13’s picture

Status: Needs review » Reviewed & tested by the community

OK for me.

The last submitted patch, 7: paragraphs-n2780877-7.patch, failed testing. View results

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

New functionality without tests can't be RTBC here. Didn't look further into the requirement / solution.

Berdir’s picture

+++ b/paragraphs.module
@@ -293,3 +293,25 @@ function paragraphs_preprocess_field_multiple_value_form(&$variables) {
+        $namespace = 'Drupal\paragraphs\Entity\Storage\Sql';
+        $entity_type->setHandlerClass('storage', "$namespace\\ParagraphsStorage");
+        if (\Drupal::moduleHandler()->moduleExists('entity_storage_migrate') ) {

Just use ParagraphsStorage::class, then PHP takes care of the namespacing for you.

vacho’s picture

Issue tags: +Needs reroll
vacho’s picture

Patch rerolled

vacho’s picture

Solving test problems

johnchque’s picture

@vacho please upload interdiffs along with your patches.

vacho’s picture

FileSize
976 bytes

Interdiff but strangely this not solve the problem at test.

vacho’s picture

Trying to solve test problems again.

Berdir’s picture

  1. +++ b/modules/paragraphs_library/tests/src/FunctionalJavascript/ParagraphsContentModerationTest.php
    @@ -408,7 +408,12 @@ class ParagraphsContentModerationTest extends JavascriptTestBase {
         $this->drupalGet('admin/config/workflow/workflows/manage/' . $this->workflow->id());
    -    $assert_session->pageTextNotContains('Paragraph types');
    +    if (\Drupal::moduleHandler()->moduleExists('multiversion')) {
    +      $assert_session->pageTextContains('Paragraph types');
    +    }
    +    else {
    +      $assert_session->pageTextNotContains('Paragraph types');
    +    }
         $assert_session->pageTextContains('Content types');
    

    I 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.

  2. +++ b/paragraphs.module
    @@ -514,11 +514,24 @@ function paragraphs_migration_plugins_alter(array &$migrations) {
     function paragraphs_entity_type_alter(array &$entity_types) {
    -  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
    -  // Remove the handler class for moderation as it is managed by the host.
    -  $entity_types['paragraph']->setHandlerClass('moderation', '');
    +  if (\Drupal::moduleHandler()->moduleExists('multiversion') ) {
    +    /** @var \Drupal\multiversion\MultiversionManagerInterface $manager */
    +    $manager = \Drupal::service('multiversion.manager');
    +
    

    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.

  3. +++ b/src/Entity/Storage/Sql/ParagraphsStorage.php
    @@ -0,0 +1,127 @@
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    +
    +class ParagraphsStorage extends SqlContentEntityStorage implements ContentEntityStorageInterface {
    +
    

    these classes should have specific names/namespaces to make clear that it is about the multiversion module.