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] Added support for extract type from default value #27570

Merged

Conversation

@tsantos84
Copy link
Contributor

commented Jun 9, 2018

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

fabpot approved these changes Jun 25, 2018

Copy link
Member

left a comment

with a minor comment

return null;
}
$map = array(

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 25, 2018

Member

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

This comment has been minimized.

Copy link
@tsantos84

tsantos84 Jun 27, 2018

Author Contributor

Sure. Actually, quite obvious.

This comment has been minimized.

Copy link
@dunglas

dunglas Jun 28, 2018

Member

Can even be a private const as it targets master.

'double' => Type::BUILTIN_TYPE_FLOAT,
);
$type = gettype($defaultValues[$property]);

This comment has been minimized.

Copy link
@dunglas

dunglas Jun 28, 2018

Member

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

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

@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()));

This comment has been minimized.

Copy link
@dunglas

dunglas Jul 3, 2018

Member

DefaultValue::class

This comment has been minimized.

Copy link
@tsantos84

tsantos84 Jul 4, 2018

Author Contributor

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

Show resolved Hide resolved src/Symfony/Component/PropertyInfo/Tests/Fixtures/DefaultValue.php
@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Hi @dunglas, Anymore work to do here?

@sroze

sroze approved these changes Sep 18, 2018

Copy link
Member

left a comment

Very nice 👍

@@ -42,6 +42,12 @@ class ReflectionExtractor implements PropertyListExtractorInterface, PropertyTyp
*/
public static $defaultArrayMutatorPrefixes = array('add', 'remove');
private static $mapTypes = array(

This comment has been minimized.

Copy link
@dunglas

dunglas Sep 18, 2018

Member

Could be a const right?

This comment has been minimized.

Copy link
@tsantos84

tsantos84 Sep 18, 2018

Author Contributor

Sure.

Show resolved Hide resolved src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Sep 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.

@dunglas

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

Failures look unrelated with your changes.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

Nice. Thank you.

@fabpot

fabpot approved these changes Oct 10, 2018

@fabpot fabpot force-pushed the tsantos84:extract-property-type-from-default-value branch from 0ccade1 to 86e7340 Oct 10, 2018

@tsantos84 tsantos84 requested a review from lyrixx as a code owner Oct 10, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 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.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Oct 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.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Nov 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.

@xabbuh xabbuh force-pushed the tsantos84:extract-property-type-from-default-value branch from 764b5b7 to 01d4016 Nov 3, 2018

@xabbuh

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

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

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

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

@xabbuh xabbuh force-pushed the tsantos84:extract-property-type-from-default-value branch from 01d4016 to 2114df9 Nov 5, 2018

@xabbuh

xabbuh approved these changes Nov 5, 2018

@xabbuh

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

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

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

Sure. Which version should I reference?

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

@chalasr

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@tsantos84 4.3.0 is the target here.

@tsantos84 tsantos84 force-pushed the tsantos84:extract-property-type-from-default-value branch from 2114df9 to e11bc00 Jan 18, 2019

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

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

@fabpot fabpot force-pushed the tsantos84:extract-property-type-from-default-value branch from e9079bd to f6510cd Feb 21, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Thank you @tsantos84.

@fabpot fabpot merged commit f6510cd into symfony:master Feb 21, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 21, 2019

feature #27570 [PropertyInfo] Added support for extract type from def…
…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

@tsantos84 tsantos84 deleted the tsantos84:extract-property-type-from-default-value branch Feb 21, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.