[PropertyInfo] Added support for extract type from default value #27570
Conversation
| return null; | ||
| } | ||
|
|
||
| $map = array( |
There was a problem hiding this comment.
What about storing this as a static property on the class instead?
There was a problem hiding this comment.
Sure. Actually, quite obvious.
There was a problem hiding this comment.
Can even be a private const as it targets master.
src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
Outdated
Show resolved
Hide resolved
| 'double' => Type::BUILTIN_TYPE_FLOAT, | ||
| ); | ||
|
|
||
| $type = gettype($defaultValues[$property]); |
There was a problem hiding this comment.
I think it's safer to pass (return null) if the type is null.
Most of the time, it will mean that the property isn't initialized (and it may be initialized in the constructor, with null values not allowed). Even when null is intended, it's very unlikely that it will be the only type allowed.
Returning an array means that next extractors in the chain will be skipped, it shouldn't be the case if the value is null (another extractor may find the relevant type).
|
@dunglas, anymore work should be done here? |
| */ | ||
| public function testExtractWithDefaultValue($property, $type) | ||
| { | ||
| $this->assertEquals($type, $this->extractor->getTypes('Symfony\Component\PropertyInfo\Tests\Fixtures\DefaultValue', $property, array())); |
There was a problem hiding this comment.
Done, but there are a lot of FQN refers in the test class. It's ok?
|
Hi @dunglas, Anymore work to do here? |
| @@ -42,6 +42,12 @@ class ReflectionExtractor implements PropertyListExtractorInterface, PropertyTyp | |||
| */ | |||
| public static $defaultArrayMutatorPrefixes = array('add', 'remove'); | |||
|
|
|||
| private static $mapTypes = array( | |||
|
Could someone point me how to fix the broken tests on PHP 7.1? I didn't find out how these last changes has impacted on other SF components. |
|
Failures look unrelated with your changes. |
|
Nice. Thank you. |
|
@tsantos84 Can you rebase this PR to get rid of the merge commits (this is a requirement before I can merge)? Thank you. |
|
@fabpot, I've got all this commits after rebasing the branch. Is this the expected result? Sounds weird for me. I've never use rebase before to be honest. |
|
Github had bad moments exactly when I've tried to call you to help me with rebase. I think its better to create a new branch and cherry pick only the necessary commits. |
|
@tsantos84 I have rebased your PR. Can you double-check that I didn't mess up anything? |
|
All commits are there. Many thanks for help me in this PR. |
|
It could be worth to add an entry to the component's changelog file. |
|
@tsantos84 could you please add a note in the changelog file of the component as suggested by @xabbuh? |
|
Sure. Which version should I reference? |
|
@tsantos84 4.3.0 is the target here. |
|
Done, @dunglas. Sorry for the very long time since your request for these changes. |
|
Thank you @tsantos84. |
…ault value (tsantos84) This PR was squashed before being merged into the 4.3-dev branch (closes #27570). Discussion ---------- [PropertyInfo] Added support for extract type from default value | Q | A | ------------- | --- | Branch? | master | | Bug fix? | no | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | - | | License | MIT | | Doc PR | - | Added support for extract type from property's default value. ```php class Dummy { private $age = 30; } $types = $propertyInfo->getTypes('Dummy', 'age'); /* Example Result -------------- array(1) { [0] => class Symfony\Component\PropertyInfo\Type (6) { private $builtinType => string(3) "int" private $nullable => bool(false) private $class => 'Dummy' private $collection => bool(false) private $collectionKeyType => NULL private $collectionValueType => NULL } } */ ``` Commits ------- f6510cd [PropertyInfo] Added support for extract type from default value
Added support for extract type from property's default value.