[PropertyAccess] consistency in createPropertyAccessor #19040
Conversation
|
Travis failure unrelated to these changes. |
| * @return PropertyAccessor The new property accessor | ||
| */ | ||
| public static function createPropertyAccessor($throwExceptionOnInvalidIndex = false, $magicCall = false) | ||
| public static function createPropertyAccessor($magicCall = false, $throwExceptionOnInvalidIndex = false, CacheItemPoolInterface $cacheItemPool = null) |
There was a problem hiding this comment.
those properties should not exist as well because that is where the builder should be used. otherwise this static method is just a wrapper around the constructor of PropertyAccessor.
There was a problem hiding this comment.
The order of $magicCall and $throwExceptionOnInvalidIndex shouldn't be inverted (it's a BC break).
There was a problem hiding this comment.
otherwise this static method is just a wrapper around the constructor of PropertyAccessor.
It was already before, just more restrictive due to the missing arguments.
Also I understand your point of view about the usefulness of this, but most of time there is no need of the builder itself because these arguments are defined once only when getting the PropertyAccessor instance. IMO the builder should be used to deal with differently configured property accessor, enabling features via the builder methods rather than re-pass all the PropertyAccessor constructor args each time you want to change one of its properties..
Plus I don't see the value of creating a PropertyAccessorBuilder instance + adding an additional method call (getPropertyAccessor()) that does exactly the same logic that new PropertyAccessor(...) does.
There was a problem hiding this comment.
So is this change a BC break? (inversed args)
There was a problem hiding this comment.
@chalasr : No it isn't, as you introduced them in https://github.com/symfony/symfony/pull/18977/files#diff-223e0a8a8125ad444ff1668096eeb7deR28, which targets the same branch in development.
I guess @dunglas was simply not aware of that, which mislead him.
chalasr
force-pushed
the
property_access_factory
branch
from
02905a3 to
1e5eea1
Compare
|
Closing as #18977 has been reverted. |
7 participants
This adds the missing
$cacheItemPoolargument toPropertyAccess::createPropertyAccessor()next to #18977 .Also, rather than using the PropertyAccessorBuilder to get a PropertyAccessor instance and make one method call per argument (feature enabling), it directly calls
new PropertyAccessorwith all the method arguments passed.About @Tobion comment in 18977 Please have a look to the doc PR, it now presents all features enabling from
createPropertyAccessorarguments and documents thePropertyAccessorBuilderas the right way to get multiple property accessors independently configured.