Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PropertyInfo][ReflectionExtractor] Check the array mutator prefixes last when the property is singular #36305

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Apr 1, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #36079
License MIT
Doc PR -

Check the related tickets that have a very descriptive example.

If the property is singular, we should prioritize non array mutator prefixes and do the opposite for plural property. It relies on some guessing but it actually fixes real world scenarios.

@@ -61,6 +61,9 @@ class ReflectionExtractor implements PropertyListExtractorInterface, PropertyTyp
*/
private $arrayMutatorPrefixes;

private $arrayMutatorPrefixesFirst;
Copy link
Contributor Author

@fancyweb fancyweb Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these new class properties if we do some refacto on the existing $mutatorPrefixes and $arrayMutatorPrefixes ones. Basically, currently, $mutatorPrefixes includes the values of $arrayMutatorPrefixes. We could separate the values and join them when we need them.

Copy link
Member

@dunglas dunglas Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As these properties are marked @internal, doing the refactoring is ok. I'm in favor for the refactor if it simplifies the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the refacto should target master

@@ -330,7 +336,9 @@ private function getMutatorMethod($class, $property)
$ucProperty = ucfirst($property);
$ucSingulars = (array) Inflector::singularize($ucProperty);

foreach ($this->mutatorPrefixes as $prefix) {
$mutatorPrefixes = \in_array($ucProperty, $ucSingulars, true) ? $this->arrayMutatorPrefixesLast : $this->arrayMutatorPrefixesFirst;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the the property singulars contains the property itself, we can assume the property is singular isn'it?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 3, 2020
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit 547c99e into symfony:3.4 Apr 6, 2020
@fancyweb fancyweb deleted the property-info-reflection-extractor-prioritize-mutator-method branch April 6, 2020 10:12
@nicolas-grekas
Copy link
Member

@fancyweb now merged up to master, refacto unlocked :)

This was referenced Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants