Diff on entities

Before making a nice diff feature.

Indicating on the pull form if the content is a new content or will be updated.

Back to minor has a basic diff has been implemented.

Using the diff module https://www.drupal.org/project/diff. Maybe it will be possible to use:

diff/src/Controller/NodeRevisionController.php, compareNodeRevisions:

$build = $this->compareEntityRevisions($route_match, $left_revision, $right_revision, $filter);

But the problem is the route_match parameter that we may don't want.

Discussed with @friera, @richardrodriguez21, and others at Werfen.

Crediting people at Werfen to not forget.

Here is a POC I made.

It requires to have patch on diff module: #3088227-2: Use diff in another context

And a discussion with diff's maintainers to have something maintainable.

I am removing the options to only get the split_fields layout.

So a lot less modifications in the diff module will be required.

After simplifying the diff, I had been able to obtain a default implementation that requires only the following patch in diff module:

#3088274-2: Prevent fatal error if the revision has no author

jsonapiHelper and stateInformation services are more complex, and will be required to be rethought as part of #3060694: Rework service and tests.

I am waiting for feedbacks before merging.

1. +++ b/modules/entity_share_client/src/Controller/DiffController.php
   @@ -0,0 +1,170 @@
   +    $channels_infos = $this->remoteManager->getChannelsInfos($remote);
   

   I found that when a config entity is created with parameter upcasting, the overrides are not applied. I found some issues in search_api about that but not core (I admit that I searched only two minutes).

   I will upload a quick fix.

2. +++ b/modules/entity_share_client/src/Service/JsonapiHelper.php
   @@ -587,18 +602,40 @@ class JsonapiHelper implements JsonapiHelperInterface {
   +        StateInformationInterface::INFO_ID_SYNCHRONIZED,
   

   I have allowed to have a diff link even when the entity is marked has synchronized, because currently only the changed timestamp is used to check if there is a diff and also in DiffLayoutInterface, there is only one method "build".

   So to avoid relying only on the changed timestamp, the diff should be calculated on each row and then parse the renderable array to see if there is a diff. That adds too many calculation in my opinion, and so in case, the change timestamp is the same but there is a change it allows to see if there is a diff.

   If after a lot of usage, this case can be removed easily I think.

Also I will apply your suggestion on removing diff column.

Patch incoming.

I forgot: Thanks @quicksketch for the tests and feedbacks!

Here is the patch.

Also I am not able to reproduce the screenshot from comment 20. I attached a screenshot to show my results.

I use the default diff settings, maybe you have some different config?

If you can give me more steps to reproduce the behavior.

Thanks @Quicksketch for testing.

Great to find out it was mismatching entity ids!

Here is a patch that will resolve this case too.

Also for the diff appearing on "synchronized" entities it was still there so normal that you still saw the link. It is removed now.

Is the last patch ok for you?

Thanks!

This is merged now.

Here are the results of my investigations.

I reproduced the problem.

There is also the problem in the case of taxonomy term for example.

Seeing the options on admin/config/content/diff/fields and on https://git.drupalcode.org/project/diff/blob/8.x-1.x/src/Plugin/diff/Fie..., for entity reference field, there is only the option of a diff on the entity id or on the label.

For entity reference revisions, there is a dedicated plugin https://git.drupalcode.org/project/entity_reference_revisions/blob/8.x-1... which compare the entities:

$parsed_text = $this->entityParser->parseEntity($field_item->entity);

The problem is that the workaround of injecting the entity ID:

 // Force the remote entity id to be the same as the local entity otherwise
      // the diff is not helpful.
      $entity_data['attributes'][$id_public_name] = $left_revision->id();
      $right_revision = $this->jsonapiHelper->extractEntity($entity_data);

can't be done for referenced entities because the Diff module is thought to be used on one website and so in diff/src/Controller/NodeRevisionController.php:

  public function compareNodeRevisions(NodeInterface $node, $left_revision, $right_revision, $filter) {
    $storage = $this->entityTypeManager()->getStorage('node');
    $route_match = \Drupal::routeMatch();
    $left_revision = $storage->loadRevision($left_revision);
    $right_revision = $storage->loadRevision($right_revision);
    $build = $this->compareEntityRevisions($route_match, $left_revision, $right_revision, $filter);
    return $build;
  }

$right_revision has values loaded and for the entity reference fields there are values, they are populated. So when loading a referenced it is ok, the entity exists on the website.

But in the Entity share implementation, entity_share/modules/entity_share_client/src/Controller/DiffController.php, when extracting JSON data to create $right_revision, the entity has the fields, but there is no value for the entity referenced fields.

So I don't know how to go further with Diff. And I am thinking that it will require to have a custom diff implementation in Entity share.

Maybe getting JSON:API and/or Diff maintainers will be helpful.

Thanks @quickstketch for the feedback.

Yes that could be interesting, but that will require Diff maintainers help.

And there is still the second problem.

When doing diff, the way it has been intended, on one website, you have access to all the entities.

When extracting an entity in Entity Share, the entity reference fields are empty. I will take a look into the tests of JSON:API if there is a case of inclusion allowing to get those entities when extracting.

I have searched in JSON:API tests and asked on Slack if it is possible to extract an entity with entity reference values set.

In JSON:API tests, core/modules/jsonapi/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php, there are only tests for normalization involving the inclusion feature https://www.drupal.org/docs/8/modules/jsonapi/includes.

I also put a comment on #3088227-4: Use diff in another context.

Before forgetting.

Maybe a light custom solution using https://www.previousnext.com.au/blog/using-drupal-diff-component-custom-... can be tested.

Hello,

@harpreet16: first, thanks a lot for the article https://www.srijan.net/blog/entity-share-a-cost-effective-solution-to-ma....

Second, thanks for your dedication on this issue. The custom solution of comment 34 is only a proposition in case fixing "right revision entity" would be impossible. So yes, if it will be possible to fix "right revision entity", let's go for it for sure.

I think we can stay in this issue, as it has been reopened on comment 28.

I have started writing a piece of code for the same but need to switch to some other work on priority.

: no problem, I understand completly being staffed on something else. No pressure.

Hello @harpreet16,

Hope you're doing well. First of all sorry for taking so much time in getting some sort of solution for the issue we are facing here.

Thanks! Thanks a lot for your patch. And no problem for the delay. I know it is hard to get time to contribute.

Thanks for the detailed explainations, I will give a look to the patch.

Here is my review.

1. +++ b/modules/entity_share_client/src/Controller/DiffController.php
   @@ -134,68 +164,133 @@ class DiffController extends PluginRevisionController {
   +                if (sizeof($existing_entities) != 0) {
   +                  foreach ($existing_entities as $value_id) {
   +                    $key_id = $value_id->id();
   +                    $field_data = [$main_property => $key_id];
   +                    if (isset($field_value['meta'])) {
   +                      $field_data += $field_value['meta'];
   +                    }
   +                  }
   +                }
   +                else {
   +                  $field_data = [$main_property => $value['links']['related']['href']];
   +                  if (isset($field_value['meta'])) {
   +                    $field_data += $field_value['meta'];
   +                  }
   

   Why don't you request the related entity to be able to provide a complete diff?

   Infinite loop? Or maybe too many requests?

2. +++ b/modules/entity_share_client/src/Controller/DiffController.php
   @@ -134,68 +164,133 @@ class DiffController extends PluginRevisionController {
   +            ['data' => t('Removal'), 'colspan' => '2'],
   +            ['data' => t('Additions'), 'colspan' => '2'],
   

   I would have named the headers as before, to be clear of what represents the remote entity and what represents the local entity.

3. +++ b/modules/entity_share_client/src/DiffManager.php
   @@ -0,0 +1,96 @@
   +    'Plugin/diff',
   

   Plugin/EntityShareClient/diff to be sure to not interfere with the diff module.

4. +++ b/modules/entity_share_client/src/DiffManagerBase.php
   @@ -0,0 +1,71 @@
   +abstract class DiffManagerBase extends PluginBase implements DiffGeneratorInterface, ContainerFactoryPluginInterface {
   

   I would have named it DiffParserBase.

5. +++ b/modules/entity_share_client/src/Entity/Remote.php
   @@ -97,4 +97,14 @@ class Remote extends ConfigEntityBase implements RemoteInterface {
   +  public function getUrl() {
   

   Same as #3139150-3: Get the remote Entity ID, I think you don't need this method.

Beyond the review, great work, but...

1. The way it is done, maybe it can now be a dedicated sub-module or a module.
2. If you can, please make a patch for the 8.x-3.x branch. As I doubt I will merge any new features on the 8.x-2.x
3. Yes, the UI is less convenient than with the Diff module.
4. main point: The main problem of referenced entities is still there. Maybe @quicksketch who reopened the issue comment 28 can give feedback on your work too.

1. Diff won't work in our case as it has no means to load entities(referenced) from a remote site.

I think we can, by making a GET on $value['links']['related']['href'], and then JsonapiHelper::extractEntity, we can have an entity object that can be injected in the entity we wan't to see the diff.

This would be the approach I would try.

And/Or, changing diff on referenced entities into a link to a new custom controller page, where the diff of this new entity will be done, etc.

Is it ok for you?

Regards,

Hello,

@ivan.vujovic, Thanks a lot for the patch!

Here is a summary of our discussion.

I will test with recursive paragraphs to see the real effect of the patch. And I will test with the approach I mentioned in my comment 43:

I think we can, by making a GET on $value['links']['related']['href'], and then JsonapiHelper::extractEntity, we can have an entity object that can be injected in the entity we wan't to see the diff.

And post the screenshots of the results.

Because the suggestion I made on comment 37 which led to using core diff system was only in a first time for experiment.

And even if this not much code, if we can avoid reinventing what the Diff module does that would be nice.

So I am testing what I have in mind and then, we will be able to compare and take a decision.

For Dynamic Entity Reference fields, it is "normal" that sharing is not working, there is a dedicated issue #3056102: Support dynamic entity reference fields.

Cheers :)

I have tested the custom diff with subparagraphs and it works. Nice @Ivan.vujovic!

The screenshots are from the Diff module on a revision comparaison on the same site, native Diff usage, and it also works recursively with paragraphs.

I have tested the approach I wanted to test on comment 37, and now I know why it won't work. When unserializing the referenced content, and injecting it into the $right_revision, the problem is that it does not work. the values in the entity reference fields are target_id which changes between the local and remote entities. So impossible to use Diff module properly on Entity Share when dealing with Entity Reference fields.

Except if someone else has an idea on that?

I agree now that the approach with custom diff plugin is the only one remaining.

In addition (may be redundant) to what @ivan.vujovic wrote in comment 47, this is what I think needs to be done:

• Decide if this should be a sub-module, entity_share_diff, or part of entity_share_client
• Uniformize with the other plugin system in terms of class names and position in codebase, dedicated folder, etc.
• Refactor some parts to avoid duplicated code (will point it out in review)
• Keep the table header like it is currently, so it clearer what is local and what is remote
• Improve the diff table to look the more similar as possible as what the Diff module provides because otherwise it is hard to distinguish field names of field values and referenced entities
• Similar to /admin/config/content/diff/fields, if possible have a way to choose what field we want to diff or not so it will avoid to have hardcoded list of ignored field, etc. This may justify having a sub-module.

Any opinion on that?

1. +++ b/modules/entity_share_client/src/Service/EntityParser.php
   @@ -0,0 +1,360 @@
   +    $resource_type = $this->resourceTypeRepository->get(
   +      $entity->getEntityTypeId(),
   +      $entity->bundle()
   +    );
   +    return $resource_type;
   

   I wonder if this can be refactored, or it is worth refactoring.

2. +++ b/modules/entity_share_client/src/Service/EntityParser.php
   @@ -0,0 +1,360 @@
   +  public function referencedTypesHandlable(array $entity_type_ids) {
   +    if (empty($entity_type_ids)) {
   +      return FALSE;
   +    }
   +    // All referenced entity types must be supported.
   +    foreach ($entity_type_ids as $entity_type_id) {
   +      if ($entity_type_id == 'user') {
   +        return FALSE;
   +      }
   +      if ($this->entityDefinitions[$entity_type_id]->getGroup() == 'configuration') {
   +        return FALSE;
   +      }
   +    }
   +    return TRUE;
   +  }
   

   Need to refactor with modules/entity_share_client/src/Plugin/EntityShareClient/Processor/EntityReference.php::isUserOrConfigEntity()

Hello,

@ivan.vujovic, really good work! Thanks a lot for your patch!

Before going further, I would like to get some feedbacks from @harpreet16 and @quicksketch.

So do not take my review points into account please. Especially the coding standards/unused variables, I can handle it when merging if no other patch has to be provided.

1. +++ b/modules/entity_share_client/src/Form/PullForm.php
   @@ -660,7 +660,7 @@ class PullForm extends FormBase {
   +    if ($this->moduleHandler->moduleExists('entity_share_diff')) {
   

   Now that we depend of a sub-module of our own and not a contrib module, would it be possible to do like in entity_share_async, with a hook_form_FORM_ID_alter?

   If yes, we can remove the moduleHandler service because it is no more used.

   (Also, maybe we will have to think on an approach to not have entity_share_async removing the diff link)

2. +++ b/modules/entity_share_client/src/Service/EntityReferenceHelper.php
   @@ -0,0 +1,106 @@
   +class EntityReferenceHelper {
   

   I really like this idea!

   Can we introduce an interface for that service please?

3. +++ b/modules/entity_share_client/src/Service/FormHelper.php
   @@ -192,32 +192,34 @@ class FormHelper implements FormHelperInterface {
   +    if ($this->moduleHandler->moduleExists('entity_share_diff')) {
   

   Same as for the PullForm, if possible, move that in hook_form_FORM_ID_alter into entity_share_diff.

4. +++ b/modules/entity_share_diff/entity_share_diff.services.yml
   @@ -0,0 +1,20 @@
   +    arguments: ['@config.factory']
   

   If you want to override the diff.formatter service. You can avoid to have ro redeclare the arguments (and so if diff.formatter will change it will be automatically taken into account) by adding "parent: diff.formatter".

   See modules/entity_share_client/tests/modules/entity_share_client_remote_manager_test/entity_share_client_remote_manager_test.services.yml

5. +++ b/modules/entity_share_diff/src/Annotation/DiffGenerator.php
   @@ -0,0 +1,42 @@
   +
   

   Missing "declare(strict_types = 1);". I will do a check when commiting.

6. +++ b/modules/entity_share_diff/src/Annotation/DiffGenerator.php
   @@ -0,0 +1,42 @@
   + * Note that the "@ Annotation" line below is required and should be the last
   + * line in the docblock. It's used for discovery of Annotation definitions.
   

   I think this line can be removed.

7. +++ b/modules/entity_share_diff/src/Controller/DiffController.php
   @@ -0,0 +1,181 @@
   +    $build = [];
   

   Unused variable.

8. +++ b/modules/entity_share_diff/src/Controller/DiffController.php
   @@ -0,0 +1,181 @@
   +    $build = $this->diffGenerator($left_yaml, $right_yaml, $header);
   +    return $build;
   

   I think you can just do "return $this->..."

9. +++ b/modules/entity_share_diff/src/Controller/DiffController.php
   @@ -0,0 +1,181 @@
   +    $left_changed = '';
   

   Unused variable.

10. +++ b/modules/entity_share_diff/src/Controller/DiffController.php
    @@ -0,0 +1,181 @@
    +   * Helper.
    

    PHPDoc to complete.

11. +++ b/modules/entity_share_diff/src/Diff/DiffFormatter.php
    @@ -0,0 +1,67 @@
    +    if (empty($this->htmlOutput)) {
    

    Is "if (!$this->htmlOutput) {" ok? Because htmlOutput is a boolean

12. +++ b/modules/entity_share_diff/src/Plugin/DiffGenerator/EntityReferenceFieldDiffParser.php
    @@ -0,0 +1,87 @@
    +      $data = $entities_json['data'] ?? [];
    

    Unused variable.

13. +++ b/modules/entity_share_diff/src/Plugin/DiffGenerator/FileFieldDiffParser.php
    @@ -0,0 +1,100 @@
    +            $label = $label;
    

    ?

14. +++ b/modules/entity_share_diff/src/Plugin/DiffGenerator/FileFieldDiffParser.php
    @@ -0,0 +1,100 @@
    +      $data = $entities_json['data'] ?? [];
    

    Unused variable.

    And some small code duplication.

15. +++ b/modules/entity_share_diff/src/Service/EntityParser.php
    @@ -0,0 +1,370 @@
    +    $embeddable_types = [
    +      'paragraph',
    +      'media',
    +    ];
    

    Can we put this list into a constant in the class please? so if someone wants to override it he/she, only have to override the service and replace the constant.

    This would be a compromise between having a config form and the availability to override it.

    Or this can stay as it is and people overriding the service only have to override this method. As for the getFieldsIrrelevantForDiff it is impossible to put the list into a constant.

    Forget my comment.

16. +++ b/modules/entity_share_diff/src/Service/EntityParser.php
    @@ -0,0 +1,370 @@
    +    $field_names = [];
    

    Unused variable.

What I didn't do:

Add a Back-office page with ability to configure plugins (set if a field should be skipped, show as embedded, shown as UUID...). I didn't want to do this now, mostly because it would take too much time.

Absolutely no problem with that. This is already a very big patch and feature. If people want to push further they will have to open a new issue. No need to over-engineer the stuff.

About the dependence on the diff configuration:

• I tried to change it with the provided Drush command, I didn't see any change, also after clearing the cache. I also change the other setting "lines_leading", also no effect
• I have attached a screenshot with a paragraph. I wonder if this is because the diff component is seeing the timestamp field as change that the 'Text plain' field from the paragraph is not indented.
• Also about the diff config, if it has an effect, to avoid impacting other parts of the website, we can provide a small config form and use it to override diff settings injected in the service used to override core service.

As of the introduction of a new sub-module for something inside the "main" module. I wonder if we should add a hook_update to enable it.

PS: ok about Readme.

Thanks again @ivan.vujovic! I think we are really near the end of this issue.

Regards,

Aout the review points from comment 52:
1: ok, postponed
2: ok
3: ok, postponed
4: ok
5: ok
6: ok
7: ok
8: ok
9: ok
10: ok
11: ok
12: ok
13: ko
14: ok
15: ok
16: ok

I will fix 13

Will do a new code review and manual testing.

I have created a PR for easier code review.

Thanks a lot @ivan.vujovic!!!

I will fix the small adjustments then manual testing.

Manual test ok. I manually re-do the commits for proper credit attribution.

Thanks everyone!