How does Entity Reference Revisions relate to the Multiversion module?

Assuming they are not trying to solve similar issues, are they compatible with each other?

Comments

greggmarshall created an issue. See original summary.

miro_dietiker’s picture

Entity reference revisions is a different thing.
Its primary purpose is to model a composite relationship and whatever is needed with it (such as pointing to a specific target revision.)
We are only depending to core and making sure the composite relationship is properly covered in integrity with revision handling.
We are NOT changing how revision handling works nor extend core with any advanced generic revision handling workflow / UI.

Multiversion is about content staging requirements. Trash states, revision flags, extended revisioning UI.

IMHO, these two things should work together and i'm happy to hear back about if there are any conflicts.

miro_dietiker’s picture

From quick discussions, we are pretty sure that there are missing integration points to content staging, because export / import needs a serialiser that supports the composite relationship we are introducing through the revision UUID that is added through multiversion.

greggmarshall’s picture

Thanks, are the missing integration points something that needs to be done in Multiversion or Entity Reference Revisions?

miro_dietiker’s picture

I guess it would be entity reference revisions since it introduces a new concept that is not yet widely adopted.

greggmarshall’s picture

Great, not an immediate need, I just see it arising when/if we expand our scope to staging (or if Workbench Moderation uses it as mentioned in their D8 road map as an option)

DamienMcKenna’s picture

Title: Relationship to/Compatibility with Multiversion? » Add Multiversion compatibility to Entity Reference Revisions
Category: Support request » Feature request
Status: Active » Needs review
Related issues: +#2756417: Integration with the Paragraphs module
FileSize
3.89 KB

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

DamienMcKenna’s picture

Title: Add Multiversion compatibility to Entity Reference Revisions » Add Multiversion compatibility to Entity Reference Revisions (for Paragraphs, etc)

FYI it will also require #2780877: Add Multiversion support to Paragraphs if people want to use this for Paragraphs.

DamienMcKenna’s picture

Version: 8.x-1.0-rc4 » 8.x-1.0
DamienMcKenna’s picture

Version: 8.x-1.0 » 8.x-1.x-dev
miro_dietiker’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/entity_reference_revisions.module
    @@ -218,3 +218,14 @@ function entity_reference_revisions_form_field_ui_field_storage_add_form_alter(a
    +  if (\Drupal::moduleHandler()->moduleExists('multiversion')) {
    +    if (isset($info['entity_reference_revisions'])) {
    +      $info['entity_reference_revisions']['class'] = '\Drupal\entity_reference_revisions\EntityReferenceRevisionsItem';
    
    +++ b/src/EntityReferenceRevisionsItem.php
    @@ -0,0 +1,70 @@
    +class EntityReferenceRevisionsItem extends BaseEntityReferenceRevisionsItem {
    ...
    +        $uuid_index = \Drupal::service('multiversion.entity_index.factory')
    

    I needed quite some time to understand that this class is really Multiversion specific.
    Don't use general names for a specific thing. We have interfaces to check compability and don't use the name.
    This also makes the "as" rename in the use unneeded.

    A littlebit more intro description in code (such as why this class is needed at all) for the class would help.

  2. +++ b/src/EntityReferenceRevisionsItem.php
    @@ -0,0 +1,70 @@
    +        if ($uuid && $record = $uuid_index->get($uuid)) {
    ...
    +          if ($this->entity->_rev->is_stub && is_numeric($record['entity_id'])) {
    ...
    +              ->getStorage($entity_type_id)
    +              ->load($record['entity_id']);
    ...
    +          elseif (!$this->entity->_rev->is_stub) {
    

    Why it it OK to load this target only be entity_id? We otherwise always load by revision.

    And i do not fully understand the stub case. For a stub without a record, we end up not loading it, nor assigning it. I think we should check and fail for unexpected situaitons to be clear.

  3. +++ b/src/EntityReferenceRevisionsItem.php
    @@ -0,0 +1,70 @@
    +      $this->target_id = $this->entity->id();
    +      $this->target_revision_id = $this->entity->getRevisionId();
    ...
    +    parent::preSave();
    +    if ($this->entity) {
    +      $this->target_id = $this->entity->id();
    +      $this->target_revision_id = $this->entity->getRevisionId();
    

    OK, parent writes a new revision if it needs a save, but we don't need to reassign the target_id. Parent even writes target_revision_id. IMHO we can skip the update after preSave().

Finally this is a specific Multiversion feature. We need a first test dependency patch to make testbot happy.
And then we need a test that cares about these additional use cases.
It would have helped me already when reviewing this to understand what i'm really reviewing from the beginning... ;-)

Grimreaper’s picture

Hello,

Here is a patch that takes miro_dietiker review into account.

And there is a patch for the test dependency.

I want to help with this issue and the paragraph one #2780877: Add Multiversion support to Paragraphs but I never dug into paragraphs/entity_reference_revisions code and I am currently testing the deploy ecosystem so if someone can give some advises about the tests that needs to be done it would really help.

Thanks.