Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Hydrator refactoring #4752

Closed
wants to merge 3 commits into from

5 participants

@mouhamed

No description provided.

@coveralls

Coverage Status

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

@coveralls

Coverage Status

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

@weierophinney

@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?

@Ocramius Ocramius commented on the diff
library/Zend/Stdlib/Hydrator/AbstractHydrator.php
@@ -195,4 +162,34 @@ public function removeFilter($name)
{
return $this->filterComposite->removeFilter($name);
}
+
+ /**
@Ocramius Collaborator
Ocramius added a note

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

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

@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 :)

@bakura10

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 :).

@mouhamed

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 ;)

@Ocramius
Collaborator

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

@bakura10

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.

@mouhamed

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

@mouhamed mouhamed closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
67 library/Zend/Stdlib/Hydrator/AbstractHydrator.php
@@ -14,8 +14,9 @@
use Zend\Stdlib\Hydrator\Filter\FilterComposite;
use Zend\Stdlib\Hydrator\StrategyEnabledInterface;
use Zend\Stdlib\Hydrator\Strategy\StrategyInterface;
+use Zend\Stdlib\Hydrator\Strategy\DefaultStrategy;
-abstract class AbstractHydrator implements HydratorInterface, StrategyEnabledInterface
+abstract class AbstractHydrator implements HydratorInterface, StrategyEnabledInterface, FilterEnabledInterface
{
/**
* The list with strategies that this hydrator has.
@@ -100,40 +101,6 @@ public function removeStrategy($name)
}
/**
- * Converts a value for extraction. If no strategy exists the plain value is returned.
- *
- * @param string $name The name of the strategy to use.
- * @param mixed $value The value that should be converted.
- * @param array $object The object is optionally provided as context.
- * @return mixed
- */
- public function extractValue($name, $value, $object = null)
- {
- if ($this->hasStrategy($name)) {
- $strategy = $this->getStrategy($name);
- $value = $strategy->extract($value, $object);
- }
- return $value;
- }
-
- /**
- * Converts a value for hydration. If no strategy exists the plain value is returned.
- *
- * @param string $name The name of the strategy to use.
- * @param mixed $value The value that should be converted.
- * @param array $data The whole data is optionally provided as context.
- * @return mixed
- */
- public function hydrateValue($name, $value, $data = null)
- {
- if ($this->hasStrategy($name)) {
- $strategy = $this->getStrategy($name);
- $value = $strategy->hydrate($value, $data);
- }
- return $value;
- }
-
- /**
* Get the filter instance
*
* @return Filter\FilterComposite
@@ -195,4 +162,34 @@ public function removeFilter($name)
{
return $this->filterComposite->removeFilter($name);
}
+
+ /**
@Ocramius Collaborator
Ocramius added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * Converts a value for extraction. If no strategy exists the plain value is returned.
+ *
+ * @param string $name The name of the strategy to use.
+ * @param mixed $value The value that should be converted.
+ * @param array $object The object is optionally provided as context.
+ * @return mixed
+ */
+ public function extractValue($name, $value, $object = null)
+ {
+ $strategy = $this->hasStrategy($name) ? $this->getStrategy($name) : new DefaultStrategy();
+
+ return $strategy->extract($value, $object);
+ }
+
+ /**
+ * Converts a value for hydration. If no strategy exists the plain value is returned.
+ *
+ * @param string $name The name of the strategy to use.
+ * @param mixed $value The value that should be converted.
+ * @param array $data The whole data is optionally provided as context.
+ * @return mixed
+ */
+ public function hydrateValue($name, $value, $data = null)
+ {
+ $strategy = $this->hasStrategy($name) ? $this->getStrategy($name) : new DefaultStrategy();
+
+ return $strategy->hydrate($value, $data);
+ }
}
View
67 library/Zend/Stdlib/Hydrator/FilterEnabledInterface.php
@@ -0,0 +1,67 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Stdlib\Hydrator;
+
+use Zend\Stdlib\Hydrator\Filter\FilterInterface;
+use Zend\Stdlib\Hydrator\Filter\FilterComposite;
+
+interface FilterEnabledInterface
+{
+ /**
+ * Add a new filter to take care of what needs to be hydrated.
+ * To exclude e.g. the method getServiceLocator:
+ *
+ * <code>
+ * $composite->addFilter("servicelocator",
+ * function($property) {
+ * list($class, $method) = explode('::', $property);
+ * if ($method === 'getServiceLocator') {
+ * return false;
+ * }
+ * return true;
+ * }, FilterComposite::CONDITION_AND
+ * );
+ * </code>
+ *
+ * @param string $name Index in the composite
+ * @param callable|FilterInterface $filter
+ * @param int $condition
+ * @return FilterComposite
+ */
+ public function addFilter($name, $filter, $condition = FilterComposite::CONDITION_OR);
+
+ /**
+ * Get the filter instance
+ *
+ * @return FilterComposite
+ */
+ public function getFilter();
+
+ /**
+ * Check whether a specific filter exists at key $name or not
+ *
+ * @param string $name Index in the composite
+ * @return bool
+ */
+ public function hasFilter($name);
+
+ /**
+ * Remove a filter from the composition.
+ * To not extract "has" methods, you simply need to unregister it
+ *
+ * <code>
+ * $filterComposite->removeFilter('has');
+ * </code>
+ *
+ * @param $name
+ * @return FilterComposite
+ */
+ public function removeFilter($name);
+}
Something went wrong with that request. Please try again.