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] Made ReflectionExtractor's prefix lists instance variables #22696

Merged
merged 1 commit into from Jun 7, 2017
Merged

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

merged 1 commit into from Jun 7, 2017

Conversation

neemzy
Copy link
Contributor

@neemzy neemzy commented May 12, 2017

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
Copy link

👍

@neemzy
Copy link
Contributor Author

neemzy commented May 12, 2017

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

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone May 15, 2017
@nicolas-grekas
Copy link
Member

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
Copy link
Contributor Author

neemzy commented May 15, 2017

Fair enough! I'll be working on this.

@neemzy neemzy changed the base branch from 2.8 to master May 15, 2017 10:43
@neemzy
Copy link
Contributor Author

neemzy commented May 15, 2017

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

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

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

ping @dunglas for "domain" review

@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@neemzy neemzy May 15, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

using $this also here
please revert the code style change

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 2.8 May 15, 2017
*
* @return self
*/
public function setMutatorPrefixes(array $mutatorPrefixes)
Copy link
Member

Choose a reason for hiding this comment

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

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

@neemzy
Copy link
Contributor Author

neemzy commented May 16, 2017

@nicolas-grekas @dunglas Is that better?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

a few test cases and might be good :)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

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

Choose a reason for hiding this comment

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

same: null as default, then compare with null below

/**
* @return string[]
*/
public function getMutatorPrefixes()
Copy link
Member

Choose a reason for hiding this comment

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

not sure we need any accessor, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually we don't

@neemzy
Copy link
Contributor Author

neemzy commented May 16, 2017

@nicolas-grekas There you go ;)

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

Choose a reason for hiding this comment

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

yoda everywhere: null !== ...


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

Choose a reason for hiding this comment

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

'' !== ...

@neemzy
Copy link
Contributor Author

neemzy commented May 16, 2017

Once again, the CI results are beyond me.

@neemzy
Copy link
Contributor Author

neemzy commented May 22, 2017

@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 nicolas-grekas changed the base branch from master to 3.4 May 22, 2017 08:29
@nicolas-grekas
Copy link
Member

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

@neemzy
Copy link
Contributor Author

neemzy commented May 22, 2017

@nicolas-grekas Alright, done!


/**
* @param DocBlockFactoryInterface $docBlockFactory
* @param string[] $mutatorPrefixes
Copy link
Member

Choose a reason for hiding this comment

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

string[]|null in fact

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

public function __construct(DocBlockFactoryInterface $docBlockFactory = null)
/**
* @var string[]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@xabbuh
Copy link
Member

xabbuh commented May 27, 2017

PhpDocExtractor tests should probably be updated too.

…bles

This allows for easier, instance-specific overriding of said values.
@neemzy
Copy link
Contributor Author

neemzy commented May 29, 2017

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

@neemzy
Copy link
Contributor Author

neemzy commented May 31, 2017

@dunglas Is that OK for you? :)

@fabpot
Copy link
Member

fabpot commented Jun 7, 2017

Thank you @neemzy.

@fabpot fabpot merged commit 58e733b into symfony:3.4 Jun 7, 2017
fabpot added a commit that referenced this pull request Jun 7, 2017
… 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
@neemzy neemzy deleted the allow-reflection-extractor-extension branch June 9, 2017 08:51
This was referenced Oct 18, 2017
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

7 participants