Release

[bakura10]

Fixed @Ocramius lol

[Ocramius]

Wondering if we can use constructor for strategies...

[bakura10]

Same thing, we need setters. Please don't remove all this just for the sake of it. It's useful and I absolutely don't want to set all the strategies, filters... through constructor.

[Ocramius]

You don't really need them...

[bakura10]

I don't get you. I REALLY need them. I use them in 100% of my hydrators.

[devosc]

It might be worth thinking about a HydratorConfig object, the interface of these config objects can indicate how the intended Hydrator is to be used or what it will be doing. And at the same time would remove any need for get/set methods (but they can be left there for BC). This way the constructor can simply set the configuration object or the parts it wants (e.g. $strategies).

In the constructor I'd rather see $this->strategies = $strategies than a for loop.

Ideally (imo), all the strategy related methods, should go into a Strategy Config object, and the hydrator could provide a convenience method so that they can be configured via $hydrator->strategies()->addStrategy() etc.

The HydratorConfigInterface might look like

interface ConfigInterface
{
  public function filters();
  public function strategies();
}

I'm currently not seeing much point to the Extract and Hydrate Contexts, why not just pass the object itself?

The namingStrategy bugs me a little, but I don't know what to suggest atm.

I also wondered if the cache could also be a config?

[bakura10]

I'll think about it (although it complicates configuring those objects by a very big factor which I'm not sure I like it).

Regarding the context objects:

• as of today context is equal to object for extraction phase but is equal to array in hydration phase. This is confusing when using it because of the generic $context name. While in some other zf2 component where context can really be anything, in hydrator the context is either an object or an array. So those objects just make it clear.
• in zf2 as I said during hydration the context is equal to the array data. But in lot of use cases I actually not only needed the array data BUT also the object itself that is being hydrated. Of course you could set an indexed array as context but this is even more confusing. Once again the contexts objects make it clear and allow to encapsulate all the context.

Envoyé de mon iPhone

Le 18 mai 2014 à 00:12, devosc notifications@github.com a écrit :

In src/Hydrator/AbstractHydrator.php:

• \* Constructor
  

• */
  

• public function __construct()
• {

•    $this->compositeFilter = new CompositeFilter();
  

•    $this->namingStrategy  = new UnderscoreNamingStrategy();
  

• }

• /**

• \* Set a new strategy for a given property
  

• *
  

• \* @param  string            $name
  

• \* @param  StrategyInterface $strategy
  

• \* @return void
  

• */
  

• public function setStrategy($name, StrategyInterface $strategy)
  It might be worth thinking about a HydratorConfig object, the interface of these config objects can indicate how the intended Hydrator is to be used or what it will be doing. And at the same time would remove any need for get/set methods (but they can be left there for BC). This way the constructor can simply set the configuration object or the parts it wants (e.g. $strategies).

In the constructor I'd rather see $this->strategies = $strategies than a for loop.

Ideally (imo), all the strategy related methods, should go into a Strategy Config object, and the hydrator could provide a convenience method so that they can be configured via $hydrator->strategies()->addStrategy() etc.

The HydratorConfigInterface might look like

interface ConfigInterface
{
public function filters();
public function strategies();
}
I'm currently not seeing much point to the Extract and Hydrate Contexts, why not just pass the object itself?

The namingStrategy bugs me a little, but I don't know what to suggest atm.

I also wondered if the cache could also be a config?

—
Reply to this email directly or view it on GitHub.

[devosc]

I would also think about adding an accept method to the Hydrator class, this could be used instead of accessing the compositeFilter in the hydrator's extract method, if ($this->accept($property, $context)) which makes the hydrator method easier to mock out? Also similar suggestion for $this->namingStrategy->getNameForExtraction($property, $context), would be $this->nameForExtraction($property, $context). I would consider having traits for those two interfaces and use them in the hydrator class to provide the two suggested methods which means testing the Hydrator does not include tests related to the other two components - since those traits are for those components and would have their own tests.

[Ocramius]

Can you return early twice to avoid using multiple else blocks?

[Ocramius]

@bakura10 inlining hasStrategy would be acceptable only if the implementation wouldn't allow child classes implementing their own hasStrategy

[bakura10]

@Ocramius , can't we index the hydrationMethodsCache by $objectClass like in extract?

[bakura10]

If we make the whole hydrators final, it kinda makes some filters like "ExcludeMethods" a bit useless. :/

@weierophinney, I'd love to have your opinion on that. I used to use a lot of hydrators that extended from ClassMethods, and add them filters to not output some given fields:

class MyHydrator extends ClassMethods
{
    public function __construct()
    {
        parent::__construct();
        $this->compositeFilter->add('myFilter', new ExcludeMethodsFilter());
    }
}

It prevents us to do some heavy caching for performance (because people may add some specific filters that are dependent on the context, so it makes every cache strategy fails).

@Ocramius point is to make all ZF2 built-in hydrators final, which allow to make us some assumptions about how they work, hence allowing very aggressive performance optimization (which is, imho very important for hydrators, this is very sensitive). He suggested to instead favour composition, assuming that because things like removing specific fields is very business specific, to make hydrators that compose another one:

class MyHydrator implements HydratorInterface
{
    protected $hydrator;

    public function __construct()
    {
        $this->hydrator = new ClassMethods();
       // You can add strategies here
    }

    public function extract($object)
    {
        $values = $this->hydrator->extract($object);

        // Strip unwanted values here
    }
}

I used to be not really for that at first but.. actually it can makes sense. With traits, people can actually recreate very powerful hydrators by extending AbstractHydrator or using just the traits they need, but by restricting the built-in hydrators, we can have the best performance for typical use cases.

I think that's really something we should carefully think about.

[bakura10]

Hi @devosc ,

I already segreted the NamingStrategy and Strategies into traits, so it should be a lot easier now. Regarding the AbstractHydrator, I think it's pretty useful. It is very lightweight, it just composes the two traits, actually.

Regarding separating the Hydrate and Extract: this is already the case as you have ExtractInterface and HydrateInterface. Maybe we should allow the plugin manager those interfaces instead of the HydratorInterface though.

[devosc]

@bakura10, what I'm thinking about is new Hydrator(Extract $extractor, Hydrate $hydrator). Ultimately I think that would be the better approach. It may seem "too much", but then again once you start getting into tests, it should pay off, Also remember that these hydrators will most likely be shared so their instantiation is only once and a factory class can be used so that not everything it forced into the constructor(s).

For example that 'ClassMethodsHydratorTest::testExtraction()' will need more work if you want to get more specific and test the cached methods?

[bakura10]

That would be indeed nice to have but to me this is a complexity that is not necessarily. To be honest, hydrators are nearly always used in par. I've never encounter a use case myself where I would only want to use extract without hydrate and vice-versa.

[weierophinney]

To be honest, hydrators are nearly always used in par. I've never encounter a use case myself where I would only want to use extract without hydrate and vice-versa.

I was going to argue that in Apigility, we only worry about extraction, but that's not true; users have to first populate the objects that we extract, which means that the both sides of the equation are present in the same request. So... agreed!

[bakura10]

Exactly. Same in ZfrRest, GET only use extract.. but POST also uses hydrate ;). So really I'm not sure about the benefit of added complexity/setup of completely segregate Hydrate and Extract

[devosc]

@weierophinney, they may occur in the same request, but do they occur in the same method, i,e is a single method calling both extract and hydrate or are they separated - maybe even (listeners).

I'm NOT completely against whats happening here, because it works. But when I read the code to try and follow what is happening, it would be so much easier if there were separated, because then the Hydrator class doesn't really care much about how the Extract and Hydrate is actually performed.

For example if you apply the new trait methods to the extract and hydrate functions, and then have another trait for the filterChain, then the Hydrator's extract and hydrator methods, would not know exactly how it is accomplished, it just knows that those interface methods will accomplish it.

What I was thinking about (very late), was it should even be possible to have a Hydrator class that does not not know anything about its underlying extraction/hydration behaviours, it just invokes their interfaces.

The only bit that doesn't sit well with me is the AbstractHydrator class, with no abstract methods defined, and hidden dependencies in the constructor - the latter is moreso because I think we should be exploring how DI will best work here.

[bakura10]

I actually think the opposite, it would be harder to follow. Actually, the fact is that you always have a mapping between extract and hydrate. For instance, strategy are often opposite transform for hydrate/extract. If you have two objects, this means you have much more settings to do as you need to add the same strategies to both objects.

[devosc]

I think you have the same, its just done in a factory class (or elsewhere), and becomes a little more cleaner. My reasoning for this argument is the 'filters' used in the extract - so it is not clear cut, looking at the constructor might suggest that both hydrate and extract needs filters. Whereas looking at the constructor and just seeing an extract object and a hydrate object is clear with no further thought needed.

Anyhow, its just my impression.

Something else that might be worth mentioning is maybe adding an __invoke method to the hydrator class, which will proxy to the 'hydrate' method. I ended up doing something similar with the route classes which allows it be used as a listener also.

[devosc]

If I have a plugin hydration manager, and want to hyrdate an object from an unknown source, how would this be implemented, e.g $manager->hydrate('My\Member\Class', $json) ?

[devosc]

A call to the SM's get method followed by a call to a Hydrator Manager, is DI.

$member = $sm->get('My\Member\Class', [constructor, arguments, here]);
$member = $hm->hydrate($member, $options);

The best way is to just make this an event via a Hydration manager that will create a HydrationEvent and retrieve the class from the SM. The "name" of the HydrationEvent is a string name that can be the name of the hydrator, a class name, or is just an identifiable name.

$hm->hydrate('My\Member\Class', $options);

And the HM then, if needed, creates the $source object and then triggers the HydrationEvent event

function hydrate($source, $options)
{    
  $name  = is_string($source)  ? $source : get_class($source);
  $source = is_string($source) ? $this->get($source) : $source;

  return $this->trigger(['HydrationEvent', $name, $source], $options);
}

The HydrationEvent is the HydrationContext.

Because handling it as an event allows a series of decoupled listeners to perform a dedicated task, I would suggest moving the NamingStrategies, and Strategies out of the ClassMethods Hydrator and creating Hydrators for each that will be called prior to the ClassMethods hydrator. This provides SRP and makes it even easier to reuse and to configure.

The Hydrator class would then have an invoke method so that it can be used as an event listener which would pass the source and the options to the hydrate method.

class ClassMethods
{
  public function hydrate($object, $data)
  {
     foreach($data as $method => $arg) {
       $object->$method($arg);
     }

     return $object
  }

  public function __invoke(HydrationEventInterface $event, $options)
  {
    return $this->hydrate($event->source(), $options)
  }
}

So for a straight forward ClassMethods hydration it would be

$hm->hydrate(['ClassMethods', $source], $options);

The NamingStrategyHydrator, StrategyHydrator, and then the ClassMethodsHydrator are called in priority order.

[bakura10]

Can I merge @Ocramius ?