[PropertyInfo] Made ReflectionExtractor's prefix lists instance variables #22696

neemzy · 2017-05-12T11:58:31Z

Q	A
Branch?	`3.4`
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

This PR makes ReflectionExtractor's mutator/accessor prefixes instance variables in order to be able to override them to change its behavior.

Gregoire-M · 2017-05-12T12:02:26Z

👍

neemzy · 2017-05-12T12:17:24Z

I don't think the failing test on Travis is related to this changeset in any way.

nicolas-grekas · 2017-05-15T07:31:23Z

I don't think it's a good idea to allow this kind of global configuration. All instances will be impacted by this overriding.
Instead, what about turning these into regular properties?
This should be done on master as a new feature btw.

neemzy · 2017-05-15T08:03:33Z

Fair enough! I'll be working on this.

neemzy · 2017-05-15T10:52:15Z

@nicolas-grekas Does this look good to you? I left the static properties untouched (apart from their name) because they are referenced in PhpDocExtractor.

nicolas-grekas

ping @dunglas for "domain" review

nicolas-grekas · 2017-05-15T13:38:43Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -137,7 +137,7 @@ public function getTypes($class, $property, array $context = array())
            return;
        }

-        if (!in_array($prefix, ReflectionExtractor::$arrayMutatorPrefixes)) {
+        if (!in_array($prefix, ReflectionExtractor::$defaultArrayMutatorPrefixes)) {


I guess this also should be turned into $this->arrayMutatorPrefixes

No can do, this is the other class, cf. my last reply

Edit: or do you mean these should be instance properties in PhpDocExtractor as well?

I mean your edit yes: if ReflectionExtractor needs this, why shouldn't PhpDocExtractor also?
But note that I'd suggest to wait for some feedback from @dunglas before spending more time on code here, just to not waste yours :)

Duly noted. Do you think default values for these instance properties should come from ReflectionExtractor::$default*Prefix?

nicolas-grekas · 2017-05-15T13:39:14Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -217,7 +217,10 @@ private function getDocBlockFromProperty($class, $property)
     */
    private function getDocBlockFromMethod($class, $ucFirstProperty, $type)
    {
-        $prefixes = $type === self::ACCESSOR ? ReflectionExtractor::$accessorPrefixes : ReflectionExtractor::$mutatorPrefixes;
+        $prefixes = $type === self::ACCESSOR


using $this also here
please revert the code style change

dunglas · 2017-05-15T14:03:38Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

+     *
+     * @return self
+     */
+    public function setMutatorPrefixes(array $mutatorPrefixes)


Why not passing those values in the constructor to make this class immutable?

neemzy · 2017-05-16T12:26:52Z

@nicolas-grekas @dunglas Is that better?

nicolas-grekas

a few test cases and might be good :)

nicolas-grekas · 2017-05-16T14:00:43Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

    {
        $this->docBlockFactory = $docBlockFactory ?: DocBlockFactory::createInstance();
        $this->contextFactory = new ContextFactory();
        $this->phpDocTypeHelper = new PhpDocTypeHelper();
+        $this->mutatorPrefixes = $mutatorPrefixes ?: ReflectionExtractor::$defaultMutatorPrefixes;


I think we should allow empty arrays, which means we should compare with null here

nicolas-grekas · 2017-05-16T14:01:06Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

+     * @param string[] $accessorPrefixes
+     * @param string[] $arrayMutatorPrefixes
+     */
+    public function __construct(array $mutatorPrefixes = array(), array $accessorPrefixes = array(), array $arrayMutatorPrefixes = array())


same: null as default, then compare with null below

nicolas-grekas · 2017-05-16T14:01:20Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

+    /**
+     * @return string[]
+     */
+    public function getMutatorPrefixes()


not sure we need any accessor, do we?

No, actually we don't

neemzy · 2017-05-16T14:39:14Z

@nicolas-grekas There you go ;)

nicolas-grekas · 2017-05-16T14:40:15Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

    {
        $this->docBlockFactory = $docBlockFactory ?: DocBlockFactory::createInstance();
        $this->contextFactory = new ContextFactory();
        $this->phpDocTypeHelper = new PhpDocTypeHelper();
+        $this->mutatorPrefixes = $mutatorPrefixes !== null ? $mutatorPrefixes : ReflectionExtractor::$defaultMutatorPrefixes;


yoda everywhere: null !== ...

nicolas-grekas · 2017-05-16T14:41:11Z

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php


-        if (preg_match('/^('.$pattern.')(.+)$/i', $methodName, $matches)) {
-            if (!in_array($matches[1], self::$arrayMutatorPrefixes)) {
+        if (!empty($pattern) && preg_match('/^('.$pattern.')(.+)$/i', $methodName, $matches)) {


neemzy · 2017-05-16T15:11:54Z

Once again, the CI results are beyond me.

neemzy · 2017-05-22T07:58:07Z

@nicolas-grekas @dunglas Just fixed conflicts that occurred since the last merges on master. May I request a moment of your time to review this changeset for what I reckon will be the last time? :)

nicolas-grekas · 2017-05-22T08:29:58Z

this should be rebased on 3.4 in fact (I already changed the base branch)

neemzy · 2017-05-22T09:20:50Z

@nicolas-grekas Alright, done!

xabbuh · 2017-05-27T07:24:48Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

+
+    /**
+     * @param DocBlockFactoryInterface $docBlockFactory
+     * @param string[]                 $mutatorPrefixes


string[]|null in fact

xabbuh · 2017-05-27T07:30:52Z

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

@@ -53,11 +53,35 @@ class PhpDocExtractor implements PropertyDescriptionExtractorInterface, Property
     */
    private $phpDocTypeHelper;

-    public function __construct(DocBlockFactoryInterface $docBlockFactory = null)
+    /**
+     * @var string[]


docblocks are actually not needed when private properties are initialised in the constructor

They were present for existing properties, so I added them for consistency's sake.

xabbuh · 2017-05-27T07:31:15Z

PhpDocExtractor tests should probably be updated too.

…bles This allows for easier, instance-specific overriding of said values.

neemzy · 2017-05-29T08:19:40Z

@xabbuh Thanks for the feedback, I took it into account.
@nicolas-grekas @dunglas Anything left to do in your opinion?

nicolas-grekas

👍

neemzy · 2017-05-31T07:30:19Z

@dunglas Is that OK for you? :)

fabpot · 2017-06-07T12:14:59Z

Thank you @neemzy.

… instance variables (neemzy) This PR was merged into the 3.4 branch. Discussion ---------- [PropertyInfo] Made ReflectionExtractor's prefix lists instance variables | Q | A | ------------- | --- | Branch? | `3.4` | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT This PR makes `ReflectionExtractor`'s mutator/accessor prefixes instance variables in order to be able to override them to change its behavior. Commits ------- 58e733b [PropertyInfo] Made ReflectionExtractor's prefix lists instance variables

carsonbot added Status: Needs Review PropertyInfo Bug labels May 12, 2017

nicolas-grekas added this to the 2.8 milestone May 15, 2017

neemzy changed the base branch from 2.8 to master May 15, 2017 10:43

neemzy changed the title ~~[PropertyInfo] Fixed static property references in ReflectionExtractor~~ [PropertyInfo] Made ReflectionExtractor's prefix lists instance variables May 15, 2017

nicolas-grekas reviewed May 15, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.4, 2.8 May 15, 2017

dunglas requested changes May 15, 2017

View reviewed changes

nicolas-grekas reviewed May 16, 2017

View reviewed changes

nicolas-grekas changed the base branch from master to 3.4 May 22, 2017 08:29

xabbuh reviewed May 27, 2017

View reviewed changes

[PropertyInfo] Made ReflectionExtractor's prefix lists instance varia…

58e733b

…bles This allows for easier, instance-specific overriding of said values.

nicolas-grekas approved these changes May 29, 2017

View reviewed changes

xabbuh approved these changes May 29, 2017

View reviewed changes

dunglas approved these changes Jun 7, 2017

View reviewed changes

fabpot merged commit 58e733b into symfony:3.4 Jun 7, 2017

neemzy deleted the allow-reflection-extractor-extension branch June 9, 2017 08:51

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Made ReflectionExtractor's prefix lists instance variables #22696

[PropertyInfo] Made ReflectionExtractor's prefix lists instance variables #22696

neemzy commented May 12, 2017 •

edited

Loading

Gregoire-M commented May 12, 2017

neemzy commented May 12, 2017

nicolas-grekas commented May 15, 2017

neemzy commented May 15, 2017

neemzy commented May 15, 2017

nicolas-grekas left a comment

nicolas-grekas May 15, 2017

neemzy May 15, 2017 •

edited

Loading

nicolas-grekas May 15, 2017 •

edited

Loading

neemzy May 15, 2017

nicolas-grekas May 15, 2017

dunglas May 15, 2017

neemzy commented May 16, 2017

nicolas-grekas left a comment

nicolas-grekas May 16, 2017

neemzy May 16, 2017

nicolas-grekas May 16, 2017

nicolas-grekas May 16, 2017

neemzy May 16, 2017

neemzy commented May 16, 2017

nicolas-grekas May 16, 2017

nicolas-grekas May 16, 2017

neemzy commented May 16, 2017 •

edited

Loading

neemzy commented May 22, 2017

nicolas-grekas commented May 22, 2017

neemzy commented May 22, 2017

xabbuh May 27, 2017

xabbuh May 27, 2017

neemzy May 29, 2017

xabbuh commented May 27, 2017

neemzy commented May 29, 2017

nicolas-grekas left a comment

neemzy commented May 31, 2017

fabpot commented Jun 7, 2017

[PropertyInfo] Made ReflectionExtractor's prefix lists instance variables #22696

[PropertyInfo] Made ReflectionExtractor's prefix lists instance variables #22696

Conversation

neemzy commented May 12, 2017 • edited Loading

Gregoire-M commented May 12, 2017

neemzy commented May 12, 2017

nicolas-grekas commented May 15, 2017

neemzy commented May 15, 2017

neemzy commented May 15, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neemzy May 15, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neemzy commented May 16, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neemzy commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neemzy commented May 16, 2017 • edited Loading

neemzy commented May 22, 2017

nicolas-grekas commented May 22, 2017

neemzy commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented May 27, 2017

neemzy commented May 29, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

neemzy commented May 31, 2017

fabpot commented Jun 7, 2017

neemzy commented May 12, 2017 •

edited

Loading

neemzy May 15, 2017 •

edited

Loading

nicolas-grekas May 15, 2017 •

edited

Loading

neemzy commented May 16, 2017 •

edited

Loading