[PropertyInfo] Support singular adder and remover #18337
Conversation
|
AppVeyor errors not related. |
| return array($reflectionMethod, $prefix); | ||
| if (null !== $singulars && in_array($prefix, self::$arrayMutatorPrefixes)) { | ||
| $names = $singulars; | ||
| $names[] = $ucProperty; |
There was a problem hiding this comment.
I think the plural version should have the highest precedence.
There was a problem hiding this comment.
I prefer to keep it as is: if someone rely on the old behavior (say it has both methods, currently the singular is used), it should not change or it will be a BC break.
There was a problem hiding this comment.
I don't see the plural version of the adder/remover being checked at all in PropertyAccessor. Did I miss anything?
There was a problem hiding this comment.
It's not supported by the PropertyAccess component but it is supported by the PropertyInfo component. Changing this behavior would be a BC break.
There was a problem hiding this comment.
I'm not sure I get what you mean. The PropertyInfo component currently only supports the plural version. How would it be a BC break if we give the highest precedence to the plural version?
There was a problem hiding this comment.
As in:
$names = $singulars;
array_unshift($names, $ucProperty);or its equivalent.
|
@dunglas The Inflector component was merged. |
|
@dunglas please update your PR. |
|
Inflector component integrated and @teohhanhui comments fixed. Status: needs review |
| try { | ||
| $reflectionMethod = new \ReflectionMethod($class, $prefix.$ucProperty); | ||
| if (in_array($prefix, self::$arrayMutatorPrefixes)) { | ||
| array_unshift($names, $ucProperty); |
There was a problem hiding this comment.
This would keep on prepending...
There was a problem hiding this comment.
What about:
$ucProperty = ucfirst($property);
$ucSingulars = (array) Inflector::singularize($ucProperty);
foreach (self::$mutatorPrefixes as $prefix) {
$names = array($ucProperty);
if (in_array($prefix, self::$arrayMutatorPrefixes)) {
$names = array_merge($names, $ucSingulars);
}
...
}
|
Changes from @teohhanhui are merged. This is ready to be merged. |
|
|
||
| foreach ($reflectionProperties as $reflectionProperty) { | ||
| foreach ((array) Inflector::singularize($reflectionProperty->name) as $name) { | ||
| if ($name === lcfirst($matches[2])) { |
There was a problem hiding this comment.
Should we use strtolower() when comparing as method names are case-insensitive (like is the regular expression used at the beginning of this method)?
|
Travis errors not related. |
| return array($reflectionMethod, $prefix); | ||
| } | ||
| } catch (\ReflectionException $reflectionException) { | ||
| // Try the next prefix if the method doesn't exist |
There was a problem hiding this comment.
This is now not true anymore. We try the next name (or next prefix) instead.
There was a problem hiding this comment.
// Try the next one if method does not exist
|
👍 |
|
👍 |
|
@fabpot ok to merge this one for the rc1? |
|
@symfony/deciders, unfortunately this PR hasn't been merged in 3.1. |
|
@dunglas But the |
|
We can add |
Fix #18166 with only a soft dependency to the PropertyAccess component. If #18260 is accepted, this PR can be reworked to use this new component, in the meantime it fixes the problem with a soft dependency.
I don't now if it should be considered as a bug fix (and then merged in 2.8) or as a new feature. It's technically a bug fix but I'm not sure that introducing a new soft dependency is acceptable if older branches. What do you think @symfony/deciders?