Embed Paragraph Content in Normalized Parent Entities (REST)

Yeah that's cool.

Now we just need test coverage. :-)

Isn't this an ERR composite subject? So it should apply to all ERR fields that point to an entity that defined the Composite definition in annotation.

This would certainly simplify exporting default content a lot, interesting.

Can actually be part of the ERR normalizer based on the composite properties, so should be moved there.

Also, we should be sure that this is backwards compatible on existing exported data.

We have a similar problem with an icon field (image upload) on a paragraph type.
Would it be possible to serialize the icon into parent config?
#2830016: Add a thumbnail/icon field to Paragraphs type

Config deployment (export/import) has nothing to do with the serializer structure) so it's not really related.

There is a way for config storage to customize what is imported/exported, but I'm not convinced at all that it is a good idea to implement that. It is a generic problem, that others are working on solving, as commented in the other issue.

Changing to ERR.
Added a test for the paragraph normalizing.

Left to do:
Fix the normalize method for paragraph (it is failing atm)

How about a multilingual szenario?
With translations on paragraphs, it would be important to save the translations to the same paragraph entity and not create another paragraph entity.

But i guess the serializer is simply serializing a single (current) language and doesn't answer any of those multilingual questions, right?

hal+json supports translations in the same exported thing, so that should in theory just work. In practice, things are more complicated, especially when adidng or removing translations. See #2135829: [PP-1] EntityResource: translations support.

Separate issue IMHO if we want to ensure that works well, and likely postponed on that core issue.

Fixed normalizer.

Fixed test (not sure if new test needs to be created, because of dependency on paragraphs_demo).

1. Added kernel test for serialization (normalize and denormalize).
2. Unsetting the _embedded link to the paragraph in normalize(), since the paragraph is attached in the separate field and this was replacing it on denormalizing.

Added followup #2871688: Multilingual support for export/import on embedded paragraph

Fixing dependency for test

Adding separate patches (1 with dependency, 1 with test and normalizer).

Changed test to use composite entity test instead of paragraphs.

Ok, worked a bit on this.

* We moved the paragraph fields into _embedded where other reference fields are too. Otherwise we have a BC change for existing REST implementations that might expect the inner data to be there.
* Implemented denormalize() as well
* Improved and fixed tests.
* Added a non-composite reference with multiple values to make sure those things still work.

I think we will need to add some more assertions, also save those things and make sure that works expected. Also not yet sure about a reliable way to detect if we really have embedded data. for that, we likely want to add a test that creates a completely new node with a composite reference, out of a raw php array with minimal data.

Fixed tests.

1. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -2,6 +2,8 @@
   +use Drupal\Core\Entity\Entity;
   

   this doesn't seem used

2. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -21,7 +23,25 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
   +      if ($target_entity = $this->serializer->denormalize($data, $target_type->getClass(), 'hal_json')) {
   +        if ($target_entity instanceof EntityNeedsSaveInterface) {
   +          $target_entity->needsSave();
   +        }
   

   I'm not sure what we should do here in the else case. New entities should be saved, but updates might not work without that, but I could live with that.

3. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -33,11 +53,21 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
   +    $target_entity = $field_item->get('entity')->getValue();
   

   you can use $field_item->entity

4. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -0,0 +1,200 @@
   + * @requires module paragraphs
   

   nope, doesn't do that anymore.

5. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -0,0 +1,200 @@
   +class EntityReferenceRevisionsNormalizerTest extends EntityKernelTestBase {
   

   We have an existing UI test that tests the existing normalization logic (\Drupal\entity_reference_revisions\Tests\EntityReferenceRevisionsNormalizerTest).

   As a follow-up, I think we should merge that (basically, referencing a specific revision and ensure that works correctly) either directly into this method or a second method on this kernel test. Then we can save a slow web test. Can you create that?

6. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -0,0 +1,200 @@
   +    // Create the test composite entity.
   +    $composite = EntityTestCompositeRelationship::create([
   +        'name' => $composite_name = $this->randomMachineName(),
   +    ]);
   +    $composite->save();
   +
   +    $nested_composite = EntityTestCompositeRelationship::create([
   +      'name' => $nested_composite_name = $this->randomMachineName(),
   +      'composite_nested_reference' => $composite
   +    ]);
   +    $nested_composite->save();
   

   As discussed, I would switch the variables (nested), the inner composite is the nested one.

7. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -0,0 +1,200 @@
   +    // Check the denormalization of the embedded composite field.
   +    $nested_composite->delete();
   +    $node->delete();
   +    $node = $this->serializer->denormalize($normalized, Node::class, 'hal_json');
   

   we should delete all 3 nested composites and then save the node, to make sure that the nested ones are being saved as well.

   $node->save();

   And then reload the node to make sure that the references still work

1. Removed this and injection of entity manager and database
2. Left it like this
3. $field_item->entity returned Node and the test was failing after,
$field_item->get('entity') returned the ERR, so I left it with ->get('entity')
4. Removed the requires paragraphs
5. Created followup for the tests merge #2876568: Merge Normalizer tests (web and kernel).
6. Switched the variables
7. Deleted the composite reference and saved node, then deleted node and reloaded it.

https://danmessenger.wordpress.com/2014/07/15/haljson-embedded-inside-em... says that nested _embedded is valid.

1. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -22,7 +23,25 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
      protected function constructValue($data, $context) {
   

   I'm not going to lie: I have a very hard time understanding what's going on here.

   I know I had a hard time reviewing the quite closely related #2827164: Entity reference field normalization has target_type and target_uuid, but not used in denormalization too.

   I think it'd be good to get @damiankloip's and @tedbow's thoughts on this, they worked a lot on #2827164 and may be remembering more of it.

2. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -108,6 +109,24 @@ class EntityReferenceRevisionsNormalizerTest extends EntityKernelTestBase {
   +    $field = FieldConfig::create(array(
   

   Nit: s/array()/[]/

3. +++ b/tests/src/Kernel/EntityReferenceRevisionsNormalizerTest.php
   @@ -122,49 +141,60 @@ class EntityReferenceRevisionsNormalizerTest extends EntityKernelTestBase {
   -/*
   -    FAILING TEST
   

   FAILING TEST no longer failing :)

After discussion with @Ginovski, I am taking care of this one.
Rebasing, addressed #33 and other cleanups;

Correct interdiff.

Nice to see this enhancement coming along. Some comments on the patch.

1. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -21,6 +22,23 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
   +    $target_type = \Drupal::entityTypeManager()->getDefinition($target_type_id);
   

   The entity_type.manager service should be injected.

2. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -21,6 +22,23 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
   +    // If the target type has a revision parent id field and that is field is
   +    // part of data, this is a composite normalization, denormalize it.
   

   Typo, should read "and that field is".

   More broadly:

   I'm not going to lie: I have a very hard time understanding what's going on here.

   Agreed. I found this code comment cryptic and had to study the code to have any idea what it meant. Overall, these changes could use more code comment explanation.

3. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -21,6 +22,23 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
   +    if ($target_type->get('entity_revision_parent_id_field') &&
   

   Elsewhere there are efforts to remove reliance on local IDs in the normalizer: #2780395: ERR fields are not deployable, #2913713: Support normalization without local revision ID. Does this and the parallel usage of the parent ID field in ::normalize potentially exacerbate the problem?

Review:

+++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
@@ -21,6 +22,23 @@ class EntityReferenceRevisionItemNormalizer extends EntityReferenceItemNormalize
+          $target_entity->needsSave();

This function does not seem to do anything?

If you (mis)use the "field is translatable" setting on an entity reference revision field (this is separate from the transalatability of the target entity or its fields), this setting is lost during the normalisation process.

The following addition to the patch fixes this.

I strongly recommend you subclass \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase, so you get complete integration testing.

Thanks for the reviews.

#36
1. Injected the entity type manager and hardened the way that we define our service. #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration already almost broke this if I hadn't commented about that :)
2. Fixed typo and tried to add a bit more comments. Open for specific suggestions but not sure what else to document.
3. Yeah, I've seen those issues. The thing is that it is not relevant what the value of that field is. It will be updated automatically on save anyway if necessary, we just need anything reliable to make sure it is an embedded entity. That said, changed to use the parent_type field now, that probably makes more sense anyway. If we wouldn't need something to identify that we have an entity, we could even drop those parent fields and let them get re-set automatically.

#37
It doesn't do much on new entities and honestly, I'm not sure how this would work on PATCH requests exactly but it would ensure that an existing entity is saved.

#40

I'm not sure. EntityResourceTestBase tests a million things that *should* not be relevant for us. We're a serializer. It just doesn't scale if every single custom serializer needs to re-test the whole REST workflow. Also, it would not test much that would actually be relevant for us as we would need to test it e.g. on a node and re-create all those fields with all the referenced entities ourself, so more or less what we are doing here.

Some REST tests might make sense, specifically for PATCH support. But I think that doesn't really belong in here.

Also did a bit of cleanup by dropping empty href entries in _links when embedded entities have no canonical link template.

"It doesn't do much on new entities and honestly, I'm not sure how this would work on PATCH requests exactly but it would ensure that an existing entity is saved."

I think this line should be removed, as the function it calls only retrieves the needsSave parameter, but the function return is discarded in this case. So nothing happens.

See EntityNeedsSaveTrait.php:21

public function needsSave() {
    return $this->needsSave;
  }

So the whole construct

if ($target_entity instanceof EntityNeedsSaveInterface) {
  $target_entity->needsSave();
}

Can be removed as it only confuses people who try to understand what's happening (I'm one of them :-).
Or am I missing something?

Ha, no I get what you mean. This was supposed to call setNeedsSave() but that only exists on the trait, not the interface.

So agreed, lets remove that, we'll have to figure out what happens on update exactly and if and how this is needed.

@Berdir

Patch #43 worked for me when creating content. The Paragraph transferred from source to target site without issue. I ran into a problem when I tried to create another version of the published content. The target site's database complained that the Paragraph id and uuid were duplicate entries and it wouldn't let me pass revision_id. I got it to work by removing those pieces of data from the serialized content:

// serialize data in hal+json format
$serialized_content = \Drupal::service('serializer')->serialize($content, "hal_json");
// json decode serialized content so we can unset some data
$serialized_content = json_decode($serialized_content, true);  
// set the key for the Paragraph field we want to remove the data from
$key = "http://my-target-site.com/rest/relation/node/my_content_type/field_my_paragraph";
foreach($serialized_content['_embedded'][$key] as $index => $paragraph) {
	// Remove Paragraph fields casuing issues when updating on target site.
  	unset($serialized_content['_embedded'][$key][$index]['id']);       
  	unset($serialized_content['_embedded'][$key][$index]['uuid']);
  	unset($serialized_content['_embedded'][$key][$index]['revision_id']);
}
$serialized_content = json_encode($serialized_content);  
// post $serialized_content to target site

Now I'm running into an issue when I delete the Paragraph from the content on the source site. The Paragraph still shows on the target site even though it was deleted from the content and saved on the source site.

**EDIT
I figured out that removing id and uuid made it so that a new Paragraph is created on the target site every time the content is posted from source to target. The original is getting deleted, but the duplicates are not. The content on the target site points to the most recent duplicate.

I created a patch so that if a Paragraph exists in the target site, then it will update the existing Paragraph values to the posted Paragraph values. I know it's probably not the proper way to handle this, but it resolves the issue in my case/

diff --git a/modules/contrib/entity_reference_revisions/src/Normalizer/EntityReferenceRevisionItemNormalizer.php b/modules/contrib/entity_reference_revisions/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
index bf7ddd2..be1acb5 100644
--- a/modules/contrib/entity_reference_revisions/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
+++ b/modules/contrib/entity_reference_revisions/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
@@ -54,6 +54,19 @@ protected function constructValue($data, $context) {
       // Denormalize the embedded entity, if it implements the
       // EntityNeedsSaveInterface interface, ensure that it will be saved.
       if ($target_entity = $this->serializer->denormalize($data, $target_type->getClass(), 'hal_json')) {
+        // check to see if entity already exists.
+        // If it does, then update existing entity values.
+        $entities = \Drupal::entityTypeManager()->getStorage($target_entity->getEntityTypeId())->loadByProperties(['uuid' => $target_entity->uuid()]);
+        foreach($entities as $entity){          
+          foreach($target_entity->toArray()as $key => $val){
+            $ignore_keys = ["uuid", "id", "revision_id"];
+            if(!in_array($key, $ignore_keys)) {
+              $entity->set($key, $val);
+            }           
+          }
+          $entity->save();
+          return ['entity' => $entity];
+        } 
         return ['entity' => $target_entity];
       }
     }

I came to this issue via the (old?) issue about paragraphs not combining well with default content, but I've tried this patch in this context and it doesn't work out quite yet. When I use the dcer command (i.e. export with references) I still get a paragraph exported entity, which then wreaks havoc when trying to import the whole thing (you might say, don't use dcer, but I'm likely to need that for other references). Is this something that should be in scope for this issue? And if not, how to we handle default content exports breaking when this patch lands? Or - entirely possible - am I missing something entirely?

#43 ok

Versions
entity_reference_revisions : 1.8
drupal : 9.0.6

Note that default_content contains built-in support for paragraphs like this. I'm not sure if this is necessary anymore or should be done, putting it back to needs work for now.