Aggregate hydrator #4448

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
5 participants
@Ocramius
Member

Ocramius commented May 8, 2013

As of zendframework/zf2#3829, this PR tries to implement a fairly flexible aggregate hydrator

  • Initial implementation
  • Collect feedback
  • Finalize implementation
  • Add soft-dependency to zendframework/zend-eventmanager
  • Test coverage
  • Documentation: see zendframework/zf2-documentation#862
@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Note: please ignore CS! I'm not handling CS until it is clear what path has to be taken.

Member

Ocramius commented May 8, 2013

Note: please ignore CS! I'm not handling CS until it is clear what path has to be taken.

+
+ public function add(HydratorInterface $hydrator, $priority = self::DEFAULT_PRIORITY)
+ {
+ $this->getEventManager()->attachAggregate(new HydratorListener($hydrator), $priority);

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Feedback needed: this attaches both to the extract and the hydrate event with the same priority. Usually, extraction is symmetrical to hydration, should I therefore attach one with $priority and another with - $priority?

@Ocramius

Ocramius May 8, 2013

Member

Feedback needed: this attaches both to the extract and the hydrate event with the same priority. Usually, extraction is symmetrical to hydration, should I therefore attach one with $priority and another with - $priority?

This comment has been minimized.

Show comment Hide comment
@prolic

prolic May 8, 2013

Contributor

yes!

@prolic

prolic May 8, 2013

Contributor

yes!

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 9, 2013

Member

This remains a tricky one - @weierophinney any idea on how to make this flexible enough to avoid regretting any wrong choice now? :)

@Ocramius

Ocramius May 9, 2013

Member

This remains a tricky one - @weierophinney any idea on how to make this flexible enough to avoid regretting any wrong choice now? :)

+ {
+ $event = new ExtractEvent($this, $object);
+
+ $this->eventManager->trigger($event);

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Not sure if the result should be used here

@Ocramius

Ocramius May 8, 2013

Member

Not sure if the result should be used here

This comment has been minimized.

Show comment Hide comment
@prolic

prolic May 8, 2013

Contributor

yes, return results, if any.
this allows also caching hydrator results based on events.

@prolic

prolic May 8, 2013

Contributor

yes, return results, if any.
this allows also caching hydrator results based on events.

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Very good point!

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 9 May 2013 01:26, prolic notifications@github.com wrote:

In library/Zend/Stdlib/Hydrator/Aggregate/AggregateHydrator.php:

  • */
    
  • protected $eventManager;
  • public function add(HydratorInterface $hydrator, $priority = self::DEFAULT_PRIORITY)
  • {
  •    $this->getEventManager()->
    
    attachAggregate(new HydratorListener($hydrator), $priority);
  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function extract($object)
  • {
  •    $event = new ExtractEvent($this, $object);
    
  •    $this->eventManager->trigger($event);
    

yes, return results, if any.
this allows also caching hydrator results based on events.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4145989
.

@Ocramius

Ocramius May 8, 2013

Member

Very good point!

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 9 May 2013 01:26, prolic notifications@github.com wrote:

In library/Zend/Stdlib/Hydrator/Aggregate/AggregateHydrator.php:

  • */
    
  • protected $eventManager;
  • public function add(HydratorInterface $hydrator, $priority = self::DEFAULT_PRIORITY)
  • {
  •    $this->getEventManager()->
    
    attachAggregate(new HydratorListener($hydrator), $priority);
  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function extract($object)
  • {
  •    $event = new ExtractEvent($this, $object);
    
  •    $this->eventManager->trigger($event);
    

yes, return results, if any.
this allows also caching hydrator results based on events.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4145989
.

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 9, 2013

Member

Thought about it: short-circuiting and using event return values will currently not be supported.

You can achieve that manually:

$cachingListener = function (ExtractEvent $event) use ($someCache) {
    if ($cache->knows($event->getExtractionObject()) {
        $event->setExtractedData($cache->get($event->getExtractionObject()));

        return false; // or explicitly stop propagation
    }
}

$aggregateHydrator->getEventManager()->attach($cachingListener, ExtractEvent::EVENT_EXTRACT, 100);

So I won't change the implementation for now. Short-circuiting may be supported later on

@Ocramius

Ocramius May 9, 2013

Member

Thought about it: short-circuiting and using event return values will currently not be supported.

You can achieve that manually:

$cachingListener = function (ExtractEvent $event) use ($someCache) {
    if ($cache->knows($event->getExtractionObject()) {
        $event->setExtractedData($cache->get($event->getExtractionObject()));

        return false; // or explicitly stop propagation
    }
}

$aggregateHydrator->getEventManager()->attach($cachingListener, ExtractEvent::EVENT_EXTRACT, 100);

So I won't change the implementation for now. Short-circuiting may be supported later on

This comment has been minimized.

Show comment Hide comment
@prolic

prolic May 9, 2013

Contributor

While I see you point, I am -1 for consistency here.

@prolic

prolic May 9, 2013

Contributor

While I see you point, I am -1 for consistency here.

+ {
+ $event = new HydrateEvent($this, $object, $data);
+
+ $this->eventManager->trigger($event);

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Not sure if the result should be used here

@Ocramius

Ocramius May 8, 2013

Member

Not sure if the result should be used here

This comment has been minimized.

Show comment Hide comment
@prolic

prolic May 8, 2013

Contributor

same again

@prolic

prolic May 8, 2013

Contributor

same again

+ */
+ public function getEventManager()
+ {
+ if (null === $this->eventManager) {

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Do the event identifiers get set here by the event manager initializer? Doesn't look like that - tips needed

@Ocramius

Ocramius May 8, 2013

Member

Do the event identifiers get set here by the event manager initializer? Doesn't look like that - tips needed

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 8, 2013

Member

Usually we set event identifiers in setEventManager(), which ensures that
they happen on injection. That also simplifies the getEventManager()
implementation, as it only needs to call setEventManager(new EventManager()).

On Wed, May 8, 2013 at 2:13 PM, Marco Pivetta notifications@github.comwrote:

In library/Zend/Stdlib/Hydrator/Aggregate/AggregateHydrator.php:

  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function setEventManager(EventManagerInterface $eventManager)
  • {
  •    $this->eventManager = $eventManager;
    
  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function getEventManager()
  • {
  •    if (null === $this->eventManager) {
    

Do the event identifiers get set here by the event manager initializer?
Doesn't look like that - tips needed


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4141310
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@weierophinney

weierophinney May 8, 2013

Member

Usually we set event identifiers in setEventManager(), which ensures that
they happen on injection. That also simplifies the getEventManager()
implementation, as it only needs to call setEventManager(new EventManager()).

On Wed, May 8, 2013 at 2:13 PM, Marco Pivetta notifications@github.comwrote:

In library/Zend/Stdlib/Hydrator/Aggregate/AggregateHydrator.php:

  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function setEventManager(EventManagerInterface $eventManager)
  • {
  •    $this->eventManager = $eventManager;
    
  • }
  • /**
  • \* {@inheritDoc}
    
  • */
    
  • public function getEventManager()
  • {
  •    if (null === $this->eventManager) {
    

Do the event identifiers get set here by the event manager initializer?
Doesn't look like that - tips needed


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4141310
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

This comment has been minimized.

Show comment Hide comment
@bakura10

bakura10 May 8, 2013

Contributor

That's what I thought too... Please add get_class($this), as well as 'Zend\Stdlib\Hydrator\HydratorInterface' as identifiers.

@bakura10

bakura10 May 8, 2013

Contributor

That's what I thought too... Please add get_class($this), as well as 'Zend\Stdlib\Hydrator\HydratorInterface' as identifiers.

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 9, 2013

Member
+
+ public function mergeExtractedData(array $additionalData)
+ {
+ $this->extractedData = ArrayUtils::merge($this->extractedData, $additionalData);

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

@bakura10 suggests array_merge here. @DASPRiD ping?

@Ocramius

Ocramius May 8, 2013

Member

@bakura10 suggests array_merge here. @DASPRiD ping?

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 8, 2013

Member

If it's a flat data structure, array_merge is fine; if it's hierarchical
at all, you should likely use ArrayUtils::merge.

On Wed, May 8, 2013 at 2:14 PM, Marco Pivetta notifications@github.comwrote:

In library/Zend/Stdlib/Hydrator/Aggregate/ExtractEvent.php:

  •    $this->extractionObject = $extractionObject;
    
  • }
  • public function getExtractedData()
  • {
  •    return $this->extractedData;
    
  • }
  • public function setExtractedData(array $extractedData)
  • {
  •    $this->extractedData = $extractedData;
    
  • }
  • public function mergeExtractedData(array $additionalData)
  • {
  •    $this->extractedData = ArrayUtils::merge($this->extractedData, $additionalData);
    

@bakura10 https://github.com/bakura10 suggests array_merge here.
@DASPRiD https://github.com/DASPRiD ping?


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4141326
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@weierophinney

weierophinney May 8, 2013

Member

If it's a flat data structure, array_merge is fine; if it's hierarchical
at all, you should likely use ArrayUtils::merge.

On Wed, May 8, 2013 at 2:14 PM, Marco Pivetta notifications@github.comwrote:

In library/Zend/Stdlib/Hydrator/Aggregate/ExtractEvent.php:

  •    $this->extractionObject = $extractionObject;
    
  • }
  • public function getExtractedData()
  • {
  •    return $this->extractedData;
    
  • }
  • public function setExtractedData(array $extractedData)
  • {
  •    $this->extractedData = $extractedData;
    
  • }
  • public function mergeExtractedData(array $additionalData)
  • {
  •    $this->extractedData = ArrayUtils::merge($this->extractedData, $additionalData);
    

@bakura10 https://github.com/bakura10 suggests array_merge here.
@DASPRiD https://github.com/DASPRiD ping?


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4448/files#r4141326
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

This comment has been minimized.

Show comment Hide comment
@bakura10

bakura10 May 8, 2013

Contributor

That is to say ?

$var = array('a' => array('b' => 'c'));
$vaa = array('a' => array('b' => 'd'));

var_dump(array_merge($var, $vaa));

It outputs array(1) {
'a' =>
array(1) {
'b' =>
string(1) "d"
}
}

which is what I expect from it. I don't see why we would need ArrayUtils::merge here.

@bakura10

bakura10 May 8, 2013

Contributor

That is to say ?

$var = array('a' => array('b' => 'c'));
$vaa = array('a' => array('b' => 'd'));

var_dump(array_merge($var, $vaa));

It outputs array(1) {
'a' =>
array(1) {
'b' =>
string(1) "d"
}
}

which is what I expect from it. I don't see why we would need ArrayUtils::merge here.

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

Fine by me: will use array_merge :)

@Ocramius

Ocramius May 8, 2013

Member

Fine by me: will use array_merge :)

@juriansluiman

This comment has been minimized.

Show comment Hide comment
@juriansluiman

juriansluiman May 8, 2013

Contributor

What are the benefits here of the EM? If you only need priorities, no short circuiting and reusing the same priority, I think you can use the Zend\Stdlib\PriorityQueue too? They are in the same component, so that saves another dependency.

Contributor

juriansluiman commented May 8, 2013

What are the benefits here of the EM? If you only need priorities, no short circuiting and reusing the same priority, I think you can use the Zend\Stdlib\PriorityQueue too? They are in the same component, so that saves another dependency.

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 8, 2013

Member

@juriansluiman didn't think about the dependency: good point!

The basic use case for the EventManager here is applying decoration work on hydrated/extracted data.

A simple use case would be serialization of an object an adding HAL data to it later on.

Another use case may be removal of fields that are not allowed for displaying in frontend

Member

Ocramius commented May 8, 2013

@juriansluiman didn't think about the dependency: good point!

The basic use case for the EventManager here is applying decoration work on hydrated/extracted data.

A simple use case would be serialization of an object an adding HAL data to it later on.

Another use case may be removal of fields that are not allowed for displaying in frontend

+ * Attaches the provided hydrator to the list of hydrators to be used while hydrating/extracting data
+ *
+ * @param \Zend\Stdlib\Hydrator\HydratorInterface $hydrator
+ * @param int $priority

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius May 9, 2013

Member

Will not make the HydratorListener symmetrical for now. I think that's a fairly advanced use case that should be handled by the user in very particular cases /cc @prolic

@Ocramius

Ocramius May 9, 2013

Member

Will not make the HydratorListener symmetrical for now. I think that's a fairly advanced use case that should be handled by the user in very particular cases /cc @prolic

@ghost ghost assigned weierophinney May 9, 2013

weierophinney added a commit that referenced this pull request May 9, 2013

weierophinney added a commit that referenced this pull request May 9, 2013

@Ocramius Ocramius referenced this pull request in zendframework/zf2-documentation May 9, 2013

Closed

Adding aggregate hydrator documentation #862

weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#4448 from Ocramius/fea…
…ture/aggregate-hydrator

[WIP] Aggregate hydrator

weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment