zfcampus / zf-hal

BSD 3-Clause "New" or "Revised" License
30 stars 49 forks source link

Different output from onRenderEntity and onRenderCollection when adding additional Entities in Events #53

Open Alanin opened 10 years ago

Alanin commented 10 years ago

As stated by Matus Zeman @matuszeman here https://groups.google.com/a/zend.com/forum/#!topic/apigility-users/zck43pReklw there seems to be a bug in the Hal helper when rendering single entities.

Quoted from the google group: "I believe there is a bug in Hal helper (https://github.com/zfcampus/zf-hal/blob/master/src/Plugin/Hal.php#L1140) where convertEntityToArray() method caches converted entities so when you set new entities or links into entity using 'renderEntity' event convertEntityToArray() returns original entity anyway ignoring everything - https://github.com/zfcampus/zf-hal/blob/master/src/Plugin/Hal.php#L495"

Code for single Entity (http://192.168.33.101/events/2182)

    public function onRenderEntity($e)
    {
        $halEntity = $e->getParam('entity');
        $version = $this->isEntityOfInterest($halEntity->entity, 'Events', 1);

        if (false !== $version) {
            if($halEntity->entity instanceof EventsEntity) {
                $repository = $this->sm->get('Umid\V1\Rest\Unternehmen\UnternehmenResource');
                $item = $repository->fetch($halEntity->entity['events_location_Id']);
                $halEntity->entity['unternehmen'] = $item;
            }
            $this->injectEventsEntityLinks($halEntity, $version);
            return;
        }
    }

Dump of $halEntity

object(ZF\Hal\Entity)[637]
  protected 'id' => string '2182' (length=4)
  protected 'links' => 
    object(ZF\Hal\Link\LinkCollection)[635]
      protected 'links' => 
        array (size=1)
          'self' => 
            object(ZF\Hal\Link\Link)[636]
              protected 'props' => 
                array (size=0)
                  empty
              protected 'relation' => string 'self' (length=4)
              protected 'route' => string 'umid.rest.events' (length=16)
              protected 'routeOptions' => 
                array (size=0)
                  empty
              protected 'routeParams' => 
                array (size=1)
                  'events_id' => string '2182' (length=4)
              protected 'url' => null
  protected 'entity' => 
    object(Umid\V1\Rest\Events\EventsEntity)[502]
      public 'Id' => string '2182' (length=4)
      ...
      public 'unternehmen' => 
        object(Umid\V1\Rest\Unternehmen\UnternehmenEntity)[696]
          public 'Id' => string '12608' (length=5)
          public 'land_Id' => string '1' (length=1)
          public 'bundesland_Id' => string '0' (length=1)
          ...

Output - embedded entity is missing

{
    Id: "2182"
    events_location_Id: "12608"
    location_Id: null
    unternehmen_Id: "12608"
    sehenswuerdigkeit_Id: "0"
    veranstalter_Id: "12608"
    ...
    -_links: {
        -self: {
            href: "http:\/\/192.168.33.101\/events\/2182"
        }
    }
}

Code for Collection (http://192.168.33.101/events)

    public function onRenderCollectionEntity($e)
    {
        $entity = $e->getParam('entity');
        $version = $this->isEntityOfInterest($entity, 'Events', 1);

        if (false !== $version) {
            $halEntity = new HalEntity($entity, $entity['Id']);
            if($halEntity instanceof EventsEntity) {
                $repository = $this->sm->get('Umid\V1\Rest\Unternehmen\UnternehmenResource');
                $item = $repository->fetch($halEntity['events_location_Id']);
                $halEntity['unternehmen'] = $item;
            }
            $this->injectEventsEntityLinks($halEntity, $version);
            $e->setParam('entity', $halEntity);
            return;
        }
    }

Dump of $halEntity

object(ZF\Hal\Entity)[685]
  protected 'id' => string '2182' (length=4)
  protected 'links' => 
    object(ZF\Hal\Link\LinkCollection)[687]
      protected 'links' => 
        array (size=1)
          'self' => 
            object(ZF\Hal\Link\Link)[690]
              protected 'props' => 
                array (size=0)
                  empty
              protected 'relation' => string 'self' (length=4)
              protected 'route' => string 'umid.rest.events' (length=16)
              protected 'routeOptions' => 
                array (size=0)
                  empty
              protected 'routeParams' => 
                array (size=1)
                  'events_id' => string '2182' (length=4)
              protected 'url' => null
  protected 'entity' => 
    object(Umid\V1\Rest\Events\EventsEntity)[667]
      public 'Id' => string '2182' (length=4)
      ...
      public 'unternehmen' => 
        object(Umid\V1\Rest\Unternehmen\UnternehmenEntity)[716]
          public 'Id' => string '12608' (length=5)
          public 'land_Id' => string '1' (length=1)
          public 'bundesland_Id' => string '0' (length=1)
          ...

Output - with embedded entity

{
    Id: "2182"
    events_location_Id: "12608"
    location_Id: null
    unternehmen_Id: "12608"
    sehenswuerdigkeit_Id: "0"
    veranstalter_Id: "12608"
    ...
    -_embedded: {
        -unternehmen: {
            Id: "12608"
            land_Id: "1"
            bundesland_Id: "0"
            landkreis_Id: "0"
            region_Id: "0"
            ...
            -_links: {
                -self: {
                    href: "http:\/\/192.168.33.101\/unternehmen\/12608"
                }
            }
        }
    }
    -_links: {
        -self: {
            href: "http:\/\/192.168.33.101\/events\/2182"
        }
    }
}
matuszeman commented 10 years ago

Desired behaviour from my point of view would be:

renderEntity and renderCollection.entity have same behaviour and event params

Currently

renderEntity event:

Set links/embedded entities are NOT rendered in HAL response due to described behaviour in description of this issue.

renderCollection.entity :

Set links/embedded entities are rendered in HAL response.

Desired

Make both events consistent in terms of params and behaviour. This would avoid confusion and remove unnecessary code e.g. https://gist.github.com/matuszeman/b797d9c70a4ce9e0974a#file-gistfile1-php-L27

renderEntity event:

Set links/embedded entities on 'entity' are rendered in HAL response.

renderCollection.entity :

Set links/embedded entities on 'entity' are rendered in HAL response.

Additional ideas

HalEntity holds extracted array of domain entity only and domain entity identifier e.g. class name or entity itself (not used for further modification of an result though). Add 'embedded' property into HalEntity as we have 'links' now. Method on HalEntity::getEmbedded() returns embedded collection which could be used to add new embedded entities.

renderEntity event:

renderCollection.entity :

weierophinney commented 9 years ago

@matuszeman Actually, your suggestion to unify the parameters for the events is not really possible. The reason renderCollection.entity does not have a ZF\Hal\Entity instance is because, at the time we trigger, we do not yet know if the value is such an entity, or if it is a value object/array composed by the collection. One reason for the event is actually to allow developers to tie into the logic and create ZF\Hal\Entity instances themselves. If you look at the lines following the triggering of the event, we create a ZF\Hal\Entity if the entity isn't one yet.

Regarding the original issue posted by @Alanin however: the problem is that we're keying the serializedEntities property on objects! This won't work unless we use SplObjectHash; I'm going to create a fix for that if I can figure out a test first...

weierophinney commented 9 years ago

Okay, interesting: we're using an SplObjectStorage instance for the serializedProperties, so that's not actually the issue.

weierophinney commented 9 years ago

@Alanin Can you please provide a unit test for this? Based on your writeup, I'm unable to build a failing unit test at this point.

weierophinney commented 4 years ago

This repository has been closed and moved to laminas-api-tools/api-tools-hal; a new issue has been opened at https://github.com/laminas-api-tools/api-tools-hal/issues/21.