Hydrator refactoring #4752

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

mouhamed commented Jun 29, 2013

No description provided.

Coverage Status

Coverage remained the same when pulling 0493616 on mouhamed:hydrator-refactoring into 22554c4 on zendframework:develop.

Coverage Status

Coverage remained the same when pulling c874d77 on mouhamed:hydrator-refactoring into 22554c4 on zendframework:develop.

Owner

weierophinney commented Jul 1, 2013

@Ocramius , @bakura10 , @cgmartin -- thoughts on this?

@mouhamed Can you elaborate a bit on the reasoning behind this addition, so that others coming to this PR later know what was added and why?

@@ -195,4 +162,34 @@ public function removeFilter($name)
{
return $this->filterComposite->removeFilter($name);
}
+
+ /**
@Ocramius

Ocramius Jul 1, 2013

Member

Don't change order of methods for no reason: it just scrambles the diffs

Member

Ocramius commented Jul 1, 2013

@weierophinney I see no reason in having the default strategy used like that - it's just useless overhead in this case (and quite a lot).

For the interface, I think the PR is still WIP and is missing the real reason to exist :)

Contributor

bakura10 commented Jul 1, 2013

Same conclusion, I tried to understand the reason behind this but I cannot see it. The strategy is not pulled from service locator so there isn't even the advantage of being able to override a default strategy globally. The overhead is not worthwhile imho.

I'll wait for @mouhamed to explain it :).

Contributor

mouhamed commented Jul 1, 2013

Hi all, thx u for your reviews, there is two changes :
1 - Add FilterEnabledInterface like StrategyEnabledInterface to ensure that filtering is enabled for a custom hydrator that does not inherit from AbstractHydrator (also in the same logic than StrategyEnabledInterface) > for composition purpose for instance
2 - The logic behinds the second changes (use of the default strategy) is that this rewrite is more readable and more consistent with the rest of the framework and with the strategy pattern. I think that this should go further with a static method setDefaultStrategy or even to permit that the strategy is pulled from service locator.

@Ocramius There is no reason for the order change, it result perhaps from a luckless copy/paste ;)

Member

Ocramius commented Jul 1, 2013

@mouhamed can you please separate the two changesets then, so that each of your requested changes gets its own review?

Contributor

bakura10 commented Jul 1, 2013

Add FilterEnabledInterface like StrategyEnabledInterface to ensure that filtering is enabled for a custom hydrator that does not inherit from AbstractHydrator (also in the same logic than StrategyEnabledInterface) > for composition purpose for instance

Isn't the FilterProviderInterface (https://github.com/zendframework/zf2/blob/master/library/Zend/Stdlib/Hydrator/Filter/FilterProviderInterface.php) enough

2 - The logic behinds the second changes (use of the default strategy) is that this rewrite is more readable and more consistent with the rest of the framework and with the strategy pattern. I think that this should go further with a static method setDefaultStrategy or even to permit that the strategy is pulled from service locator.

Well, the main problem is that it adds a lot of overhead for nothing. The hydrator has already grow to the point that it is quite slow, adding this kind of thing does not bring enough interest to justify the loss in performance. I think it's best, if you need a custom strategy for all fields by default, you can use the wildcard strategy.

For ZF3 I'd be in favor to keep a very lightweight hydrator as abstract, and find another mechanism to extend it. The filter thing is already a performance hunger, and I think I never used it once.

Contributor

mouhamed commented Jul 1, 2013

As @Ocramius suggests, I've separated the two changes into two PR
See #4764 and #4765

@mouhamed mouhamed closed this Jul 1, 2013

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