When dealing with value objects, being able to detect if a property can be initialized using the constructor is a very valuable information. It's mandatory to add a proper value object support in API Platform and in the Serializer component.

Seeapi-platform/core#1749 andapi-platform/core#1843 for the related discussions, extended use cases and proof of concepts.

This PR adds a new interface to guess if a property can be initialized through the constructor, and an implementation using the reflection (inReflectionExtractor).

dunglas requested a review fromlyrixx

April 21, 2018 10:20

carsonbot added Status: Needs Review PropertyInfo Feature labels

Apr 21, 2018

dunglas force-pushed thepropertyinfo-constructor branch froma95d0a8 to1f2103eCompare

April 21, 2018 10:21

This was referencedApr 21, 2018

Make resource constructor parameters writablesapi-platform/core#1749

Merged

value object as ApiResourceapi-platform/core#1843

Closed

bendavies reviewed

Apr 21, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Fixtures/NullExtractor.php Outdated

		*/
		publicfunctionisInitializable(string$class,string$property,array$context =array()): ?bool
		{
		$this->assertIsString($class);

Copy link

Contributor

bendaviesApr 21, 2018

Choose a reason for hiding this comment

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

What's the point of this?

Copy link

MemberAuthor

dunglasApr 22, 2018

Choose a reason for hiding this comment

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

I added this for consistency with other methods, I'm not sure of the origin intent...

Copy link

MemberAuthor

dunglasApr 22, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Here is the original intent:#19437

Copy link

Contributor

bendaviesApr 22, 2018

Choose a reason for hiding this comment

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

Right. So type hints are added now so these checks are redundant

sroze reviewed

Apr 21, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

		/**
		* Is the property initializable?
		*/
		publicfunctionisInitializable(string$class,string$property,array$context =array()): ?bool;

Copy link

Contributor

srozeApr 21, 2018

Choose a reason for hiding this comment

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

What's the use-case for returningnull? Also, shouldn't we explicit within the method name that we are talking about the constructor in here?isInitializableViaConstructor or something like that?

Copy link

Contributor

teohhanhuiApr 22, 2018

Choose a reason for hiding this comment

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

null means "we don't know"?

Copy link

MemberAuthor

dunglasApr 22, 2018

Choose a reason for hiding this comment

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

@teohhanhui is right. It means "this extractor cannot guess", try the next registered in the chain. It's mandatory to keep this behavior for consistency with existing interfaces and to provide an extension point.

Copy link

MemberAuthor

dunglasJun 30, 2018

Choose a reason for hiding this comment

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

@sroze I prefer the current name,isInitializableViaConstructor is too verbose and brings nothing much.

Copy link

Contributor

srozeJun 30, 2018

Choose a reason for hiding this comment

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

My 2cts are that I didn't understand whatisInitializable meant before reading more about your PR. I can tell you that if I saw this interface I'd think "wait. What is that about?". While theisInitializableViaConstructor clarifies it and is not too verbose.

Copy link

Contributor

srozeJun 30, 2018

Choose a reason for hiding this comment

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

I wouldn't, the PhpDoc that you've put is enough (to me).

Copy link

Contributor

teohhanhuiJun 30, 2018•
edited
Loading

Choose a reason for hiding this comment

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

"Property initializable" on its own indeed makes no sense to me. PerhapsConstructorInitializableExtractorInterface::isPropertyInitializable?

Copy link

MemberAuthor

dunglasJul 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

PropertyInitializableViaConstructorExtractorInterface::isInitializable?

Copy link

Contributor

teohhanhuiJul 4, 2018

Choose a reason for hiding this comment

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

I guess we could just stick with the current names. Lol...

Copy link

MemberAuthor

dunglasJul 4, 2018•
edited
Loading

Choose a reason for hiding this comment

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

@sroze @fabpot ok for you to keep the current names?

nicolas-grekas added this to thenext milestone

Apr 22, 2018

lyrixx reviewed

Apr 23, 2018

View reviewed changes

Copy link

Member

lyrixx left a comment

Choose a reason for hiding this comment

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

Looks good to me

Taluu reviewed

May 2, 2018

View reviewed changes

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

		continue;
		if ($property===$parameter->name) {
		returnarray($this->extractFromReflectionType($parameter->getType()));
		}

Copy link

Contributor

TaluuMay 2, 2018

Choose a reason for hiding this comment

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

what's the point of this change ? I know it does the same, but with a different style, but then ?

Copy link

MemberAuthor

dunglasMay 2, 2018

Choose a reason for hiding this comment

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

Removing 2 lines and making the code more explicit, but yes it's not mandatory.

Taluu approved these changes

May 2, 2018

View reviewed changes

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

May 2, 2018

dunglas mentioned this pull request

May 23, 2018

Non anemic doctrine models are not supportedapi-platform/core#1747

Closed

meyerbaptiste reviewed

May 29, 2018

View reviewed changes

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

		returnnull;
		}

		if ($constructor =$reflectionClass->getConstructor()) {

Copy link

Contributor

meyerbaptisteMay 29, 2018

Choose a reason for hiding this comment

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

Why don't you test if the constructor is instantiable first?

Copy link

MemberAuthor

dunglasJun 1, 2018

Choose a reason for hiding this comment

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

Good idea, I'll update the code.

Copy link

MemberAuthor

dunglasJun 30, 2018

Choose a reason for hiding this comment

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

done

nicolas-grekas reviewed

Jun 19, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php Outdated

		interface PropertyInitializableExtractorInterface
		{
		/**
		* Is the property initializable?

Copy link

Member

nicolas-grekasJun 19, 2018

Choose a reason for hiding this comment

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

not sure this is useful :)

Copy link

MemberAuthor

dunglasJun 30, 2018

Choose a reason for hiding this comment

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

It's for consistency with other interfaces.

Copy link

Member

nicolas-grekas commentedJun 19, 2018

rebase needed (and some comments to address)

dunglas force-pushed thepropertyinfo-constructor branch from88b2286 to17cf6a1Compare

June 30, 2018 08:59

Copy link

MemberAuthor

dunglas commentedJun 30, 2018

rebased and comments addressed

sroze reviewed

Jun 30, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php

		/**
		* Is the property initializable?
		*/
		publicfunctionisInitializable(string$class,string$property,array$context =array()): ?bool;

Copy link

Contributor

srozeJun 30, 2018

Choose a reason for hiding this comment

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

src/Symfony/Bundle/FrameworkBundle/Resources/config/property_info.xmlShow resolvedHide resolved

dunglas force-pushed thepropertyinfo-constructor branch fromfced167 to6737e71Compare

July 13, 2018 11:48

Copy link

MemberAuthor

dunglas commentedJul 13, 2018

Rebased, I suggest to keep the current name as there are no consensus on another name.

fabpot requested changes

Jul 13, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/CHANGELOG.md Outdated

		4.2.0
		-----

		* added`PropertyInitializableExtractorInterface` to test if a property can be initialized through the constructor and an implementation is`ReflectionExtractor`

Copy link

Member

fabpotJul 13, 2018

Choose a reason for hiding this comment

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

if a property can be initialized through the constructor (implemented byReflectionExtractor)

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


		if ($constructor =$reflectionClass->getConstructor()) {
		foreach ($constructor->getParameters()as$parameter) {
		if ($property ===$parameter->name) {

Copy link

Member

fabpotJul 13, 2018

Choose a reason for hiding this comment

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

The fact that the constructor parameter name must match the property one should be documented somewhere.

src/Symfony/Component/PropertyInfo/PropertyInitializableExtractorInterface.php Outdated

		interface PropertyInitializableExtractorInterface
		{
		/**
		* Is the property initializable?

Copy link

Member

fabpotJul 13, 2018

Choose a reason for hiding this comment

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

This is probably where we need more docs about the naming convention.

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

Jul 13, 2018

Copy link

Member

fabpot commentedSep 4, 2018

@dunglas Do you have time to have a look at my comments?

dunglas force-pushed thepropertyinfo-constructor branch 2 times, most recently fromd125f25 to4a0483dCompare

September 4, 2018 11:00

Copy link

MemberAuthor

dunglas commentedSep 4, 2018

@fabpot done

Copy link

MemberAuthor

dunglas commentedSep 4, 2018

CI errors look unrelated.

fabpot approved these changes

Sep 4, 2018

View reviewed changes

src/Symfony/Component/PropertyInfo/CHANGELOG.mdShow resolvedHide resolved

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

Sep 4, 2018

[PropertyInfo] Add an extractor to guess if a property is initializable

9d2ab9e

fabpot force-pushed thepropertyinfo-constructor branch froma8fb40e to9d2ab9eCompare

September 4, 2018 15:17

Copy link

Member

fabpot commentedSep 4, 2018

Thank you@dunglas.

fabpot merged commit9d2ab9e intosymfony:master

Sep 4, 2018

fabpot added a commit that referenced this pull request

Sep 4, 2018

feature#26997[PropertyInfo] Add an extractor to guess if a property…

4401f2f

… is initializable (dunglas)This PR was squashed before being merged into the 4.2-dev branch (closes#26997).Discussion----------[PropertyInfo] Add an extractor to guess if a property is initializable| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | todoWhen dealing with value objects, being able to detect if a property can be initialized using the constructor is a very valuable information. It's mandatory to add a proper value object support in API Platform and in the Serializer component.Seeapi-platform/core#1749 andapi-platform/core#1843 for the related discussions, extended use cases and proof of concepts.This PR adds a new interface to guess if a property can be initialized through the constructor, and an implementation using the reflection (in `ReflectionExtractor`).Commits-------9d2ab9e [PropertyInfo] Add an extractor to guess if a property is initializable

Copy link

Member

nicolas-grekas commentedSep 5, 2018•
edited
Loading

This PR breaks the CI by breaking BC somehow, see deps=high failures (which mean that 4.1 is broken when using PropertyInfo master)
@dunglas could you have a look please?
(note that we should be careful when merging PRs which have failures)

Copy link

Member

nicolas-grekas commentedSep 5, 2018•
edited
Loading

(fixed in5557e38, we'll have to deal with this kind of BC breaks now that we put DI passes in components and not in bundles anymore.)

dunglas deleted the propertyinfo-constructor branch

September 5, 2018 14:23

Copy link

MemberAuthor

dunglas commentedSep 5, 2018

Thanks for the fix@nicolas-grekas

javiereguiluz mentioned this pull request

Sep 7, 2018

[PropertyInfo] Add an extractor to guess if a property is initializablesymfony/symfony-docs#10272

Closed

Copy link

Member

javiereguiluz commentedSep 7, 2018

I've createdsymfony/symfony-docs#10272 to document this new feature. Please, don't forget to create a doc issue for every new feature.

Kévin, if you don't have time to contribute the docs, we'll need some code examples of using this feature in action. Thanks!

dunglas mentioned this pull request

Sep 10, 2018

[PropertyInfo] Add an extractor to guess if a property is initializablesymfony/symfony-docs#10319

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Sep 11, 2018

minor#10319[PropertyInfo] Add an extractor to guess if a property i…

5bc7ac0

…s initializable (dunglas, javiereguiluz)This PR was merged into the master branch.Discussion----------[PropertyInfo] Add an extractor to guess if a property is initializablesymfony/symfony#26997symfony/symfony#24571Closes#10272.Commits-------8415f96 Minor tweakseebca4a RSTf7ed144 [PropertyInfo] Add an extractor to guess if a property is initializable