NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[PropertyInfo] Added support for extract type from default value#27570

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromtsantos84:extract-property-type-from-default-value

Feb 21, 2019

Merged

[PropertyInfo] Added support for extract type from default value#27570

fabpot merged 1 commit intosymfony:masterfromtsantos84:extract-property-type-from-default-value

Feb 21, 2019

Conversation

Copy link

Contributor

tsantos84 commentedJun 9, 2018•
edited
Loading

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.

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    }  }*/

carsonbot added Status: Needs Review PropertyInfo Feature labels

Jun 9, 2018

nicolas-grekas added this to thenext milestone

Jun 11, 2018

carsonbot removed the Status: Needs Review label

Jun 19, 2018

nicolas-grekas approved these changes

Jun 19, 2018

View reviewed changes

carsonbot added the Status: Reviewed label

Jun 19, 2018

fabpot approved these changes

Jun 25, 2018

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

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

with a minor comment

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

		returnnull;
		}

		$map =array(

Copy link

Member

Choose a reason for hiding this comment

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

What about storing this as a static property on the class instead?

Copy link

ContributorAuthor

tsantos84Jun 27, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Sure. Actually, quite obvious.

Copy link

Member

Choose a reason for hiding this comment

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

Can even be aprivate const as it targets master.

tsantos84 commented

Jun 27, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php OutdatedShow resolvedHide resolved

dunglas requested changes

Jun 28, 2018

View reviewed changes

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

		'double' => Type::BUILTIN_TYPE_FLOAT,
		);

		$type =gettype($defaultValues[$property]);

Copy link

Member

dunglasJun 28, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 whennull 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 isnull (another extractor may find the relevant type).

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Jun 28, 2018

Copy link

ContributorAuthor

tsantos84 commentedJul 3, 2018•
edited
Loading

@dunglas, anymore work should be done here?

dunglas requested changes

Jul 3, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php Outdated

		*/
		publicfunctiontestExtractWithDefaultValue($property,$type)
		{
		$this->assertEquals($type,$this->extractor->getTypes('Symfony\Component\PropertyInfo\Tests\Fixtures\DefaultValue',$property,array()));

Copy link

Member

dunglasJul 3, 2018

Choose a reason for hiding this comment

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

DefaultValue::class

Copy link

ContributorAuthor

tsantos84Jul 4, 2018

Choose a reason for hiding this comment

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

Done, but there are a lot of FQN refers in the test class. It's ok?

src/Symfony/Component/PropertyInfo/Tests/Fixtures/DefaultValue.phpShow resolvedHide resolved

Copy link

ContributorAuthor

tsantos84 commentedJul 10, 2018

Hi@dunglas, Anymore work to do here?

sroze approved these changes

Sep 18, 2018

View reviewed changes

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

Very nice 👍

carsonbot added Status: Reviewed and removed Status: Needs Work Status: Reviewed labels

Sep 18, 2018

dunglas requested changes

Sep 18, 2018

View reviewed changes

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

		*/
		publicstatic$defaultArrayMutatorPrefixes =array('add','remove');

		privatestatic$mapTypes =array(

Copy link

Member

dunglasSep 18, 2018

Choose a reason for hiding this comment

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

Could be aconst right?

Copy link

ContributorAuthor

tsantos84Sep 18, 2018

Choose a reason for hiding this comment

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

Sure.

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.phpShow resolvedHide resolved

carsonbot added the Status: Needs Work label

Sep 18, 2018

dunglas approved these changes

Sep 18, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Sep 18, 2018

Copy link

ContributorAuthor

tsantos84 commentedSep 18, 2018

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.

Copy link

Member

dunglas commentedSep 18, 2018

Failures look unrelated with your changes.

Copy link

ContributorAuthor

tsantos84 commentedSep 18, 2018

Nice. Thank you.

fabpot approved these changes

Oct 10, 2018

View reviewed changes

tsantos84 requested review fromlyrixx andxabbuh ascode owners

October 10, 2018 09:43

carsonbot added Status: Needs Review and removed Status: Reviewed labels

Oct 10, 2018

Copy link

Member

fabpot commentedOct 10, 2018

@tsantos84 Can you rebase this PR to get rid of the merge commits (this is a requirement before I can merge)? Thank you.

Copy link

ContributorAuthor

tsantos84 commentedOct 21, 2018

@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.

Copy link

ContributorAuthor

tsantos84 commentedNov 3, 2018

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.

Copy link

Member

xabbuh commentedNov 3, 2018

@tsantos84 I have rebased your PR. Can you double-check that I didn't mess up anything?

Copy link

ContributorAuthor

tsantos84 commentedNov 3, 2018

All commits are there. Many thanks for help me in this PR.

xabbuh approved these changes

Nov 5, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 5, 2018

Copy link

Member

xabbuh commentedNov 5, 2018•
edited
Loading

It could be worth to add an entry to the component's changelog file.

Copy link

Member

nicolas-grekas commentedDec 1, 2018

@tsantos84 could you please add a note in the changelog file of the component as suggested by@xabbuh?

Copy link

ContributorAuthor

tsantos84 commentedDec 2, 2018

Sure. Which version should I reference?

Copy link

ContributorAuthor

tsantos84 commentedDec 3, 2018

@nicolas-grekas?

Copy link

Member

chalasr commentedDec 12, 2018

@tsantos84 4.3.0 is the target here.

Copy link

ContributorAuthor

tsantos84 commentedJan 18, 2019

Done,@dunglas. Sorry for the very long time since your request for these changes.

[PropertyInfo] Added support for extract type from default value

f6510cd

Copy link

Member

fabpot commentedFeb 21, 2019

Thank you@tsantos84.

fabpot merged commitf6510cd intosymfony:master

Feb 21, 2019

fabpot added a commit that referenced this pull request

Feb 21, 2019

feature#27570[PropertyInfo] Added support for extract type from def…

5a3e894

…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.```phpclass 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

tsantos84 deleted the extract-property-type-from-default-value branch

February 21, 2019 14:15

nicolas-grekas modified the milestones:next,4.3

Apr 30, 2019

fabpot mentioned this pull request

May 9, 2019

Release v4.3.0-BETA1#31435

Merged