HAL serialization for ERRItem is broken

+1 On this...

Have applied the patch, am looking into fixing this also.

Small patch, not sure if its the best approach and i still need to test more but seems to work after preliminary testing...

Patch off rc4

Let's see what testbot thinks. Patches please always against latest dev.

+++ b/entity_reference_revisions.info.yml
@@ -3,4 +3,7 @@ type: module
+dependencies:
+  - hal
+  - rest
+  - serialization

Instead of doing this, we should use a ServiceProvider that only adds the normalizer if these modules are enabled, e.g. see FileEntityServiceProvider in file_entity module

This will also fix support for paragraphs in Entity Pilot #2696447: Paragraph module integration and default_content module

Will try and push this along sometime this week

I've created a new patch adding a service provider but i still wasn't able to drop the hal/ serialization dependencies...
Am i missing something ?

The export is also broken but im going to take a look into that when i can

I did select, test against dev but its not worked again...

1. +++ b/entity_reference_revisions.info.yml
   @@ -3,4 +3,6 @@ type: module
   +dependencies:
   +  - hal
   +  - serialization
   
   +++ b/entity_reference_revisions.services.yml
   @@ -0,0 +1,7 @@
   +services:
   +  serializer.normalizer.entity_reference_revision_item.hal:
   +    class: Drupal\entity_reference_revisions\Normalizer\EntityReferenceRevisionItemNormalizer
   +    parent: serializer.normalizer.entity_reference_item.hal
   +    arguments: ['@rest.link_manager', '@serializer.entity_resolver']
   +    tags:
   

   So both of these can go

2. +++ b/src/EntityReferenceRevisionsServiceProvider.php
   @@ -0,0 +1,34 @@
   +    $modules = $container->getParameter('container.modules');
   

   We should check if hal exists in $modules, if so, then we alter the container, if not we don't

3. +++ b/src/EntityReferenceRevisionsServiceProvider.php
   @@ -0,0 +1,34 @@
   +      $service_definition->addTag('normalizer', array('priority' => 20));
   

   This should match what is in services.yml, so priority should be 11.

4. +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -0,0 +1,30 @@
   +	 protected $supportedInterfaceOrClass = 'Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem';
   +	 /**
   +    * Overrides \Drupal\hal\Normalizer\FieldItemNormalizer::constructValue().
   +    */
   +   protected function constructValue($data, $context) {
   +		 $value = parent::constructValue($data, $context);
   +     if ($value) {
   +			 $value['target_revision_id'] = $value['target_id'];
   +     }
   +		 return $value;
   

   there are some white space issues here

1. Care to explain why? your comment is anything but helpful. (If these dependencies aren't there it fails)
2. Without HAL the installed this fails anyway, will re instate the if.
3. It was set to 11 but i made it higher while we are still debugging

Sorry, I didn't mean to be rude, I'll update the patch.

Patch attached.

What I was trying to say in point 1 is that if you use a ServiceProvider, then you don't need a services.yml file.

And then you don't need the dependency - see the interdiff.

Working on fails

No problem, id been working on a patch and was about to submit it just as you commented! nice work! The dependencies are met if the module is installed on an active site but in my case this module is enabled by paragraphs which is inturn enabled by our custom default content module that is enabled when installing our profile... Even though our default content module relies on hal and serialization... Adding those back to the info file fixes the build... Any ideas ?

This should sort default_content, not sure about your profile - note that the order you define stuff in your .info.yml file is significant from memory

Fixes failing test and adds a test for the new functionality.

meh

This patch isnt going to be what you're looking for but ive made a couple of changes that work for me... Have uploaded and will use this in my make file till this is totally fixed.

@zach.bimson can You post the innerdiff? Or, what are your changes?

Here is @zach.bimson's interdiff

Here is the same patch as #25 which is the one to be reviewed

Just some nitpicks, looks good to me otherwise.

1. +++ b/src/EntityReferenceRevisionsServiceProvider.php
   @@ -0,0 +1,39 @@
   +/**
   + * @file
   + * Contains \Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider.
   + */
   +
   
   +++ b/src/Normalizer/EntityReferenceRevisionItemNormalizer.php
   @@ -0,0 +1,48 @@
   +/**
   + * @file
   + * Contains \Drupal\entity_reference_revisions\Normalizer\EntityReferenceRevisionItemNormalizer.
   + */
   

   @file isn't needed anymore.

2. +++ b/src/EntityReferenceRevisionsServiceProvider.php
   @@ -0,0 +1,39 @@
   +      // The priority must be higher than that of
   +      // serializer.normalizer.entity_reference_revisions.hal in
   +      // hal.services.yml.
   

   That reads like it was copied from file_entity.

   You probably want to refer to serializer.normalizer.entity_reference_item.hal instead?

3. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -319,13 +319,17 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
       */
      public function delete() {
        parent::delete();
   -    if ($this->entity && $this->entity->getEntityType()->get('entity_revision_parent_type_field') && $this->entity->getEntityType()->get('entity_revision_parent_id_field')) {
   +    if ($this->entity && $this->entity->getEntityType()
   +        ->get('entity_revision_parent_type_field') && $this->entity->getEntityType()
   +        ->get('entity_revision_parent_id_field')
   +    ) {
          $this->entity->delete();
        }
   -}
   - /**
   - * {@inheritdoc}
   - */
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   

   unrelated changes?

4. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -1,7 +1,7 @@
    /**
     * @file
   - * Contains \Drupal\entity_reference_revisions\EntityReferenceRevisionsCompositeTest.
   + * Contains \Drupal\entity_reference_revisions\Tests\EntityReferenceRevisionsCompositeTest.
   

   also an unrelated fix.

Seems like the test could be a kernel test and would be a lot faster then but would also be a lot of work to change that now.

Re-roll and fixes #31

Great thx, committed.

I think this doesn't work well with default_content. When the referenced content is imported it gets a new sequential revision_id, so target_revision_id in the referencing field is wrong.

I think there is a way to save an entity with a specific revision ID, migrate does that AFAIK too. Maybe we can change default_content to ensure that.