Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Serializer] add method for preparing constructor argument#28263

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

Conversation

@komik966
Copy link

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRtodo

When we denormalize objects which classes have constructor, it becomes useful to add custom logic for creating constructor arguments from decoded data. E. g. constructor parameter is type hinted as class, but decoded data (coming from rest api) is IRI string.

Current implemetation ofSymfony\Component\Serializer\Normalizer\AbstractNormalizer::instantiateObject forces us to think about other aspects of instantiating object (e.g. discriminators, context) when overriding it.

Related discussions:api-platform/core#1749,api-platform/core#2178

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

I'm not fond of inheritance-based extension points but that's mostly how it is done in the Serializer component so far so 👍 for me.

$parameterData =$data[$key];
if (null ===$parameterData &&$constructorParameter->allowsNull()) {
// Don't run set for a parameter passed to the constructor
unset($data[$key]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The&$data argument makes little sense to me (especially given the name of the method). Can you moveunset($data[$key]) in the caller instead and directly pass$parameterData?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Fixed. Maybe w can do similar with$key parameter? It's only used in exception message. Unless it's important to show normalized constructor parameter name, we can use$constructorParameter->name.

@komik966
Copy link
Author

komik966 commentedAug 26, 2018•
edited
Loading

@sroze For me composition also would be better. I was thinking about introducing newInstantiatorInterface, but this may generate BC break / deprecations.

@komik966komik966force-pushed theserializer-extract-create-constructor-argument branch from366dd26 tobc24d62CompareAugust 26, 2018 14:18
@dunglas
Copy link
Member

👍 to move this in a dedicated class. It should be doable without BC break (initialize the new class in the constructor if not provided).

@komik966
Copy link
Author

komik966 commentedAug 26, 2018•
edited
Loading

When we introduceInstantiatorInterface::instantiateObject then we should deprecate\Symfony\Component\Serializer\Normalizer\AbstractNormalizer::instantiateObject and deprecate dependency toNameConverterInterface in\Symfony\Component\Serializer\Normalizer\AbstractNormalizer::__construct - name converter will be injected in implementation ofInstantiatorInterface

Second option is to createConstructorArgumentFactoryInterface, then we will move only algorithm of creating constructor arguments (not instantiating entire object). In this case we have no deprecations, but I'm not sure if it isn't too small functionality for separate class.

What are your thoughts?

@komik966komik966force-pushed theserializer-extract-create-constructor-argument branch frombc24d62 to6d471cfCompareAugust 28, 2018 04:50
@dunglas
Copy link
Member

I'm for option 1, it will be more flexible, and I want to break the serializer in smaller classes since a long time.

@komik966komik966force-pushed theserializer-extract-create-constructor-argument branch from6d471cf tod8905e2CompareAugust 29, 2018 06:53
@komik966
Copy link
Author

Ok, I'll prepare changes with this approach.

@komik966komik966force-pushed theserializer-extract-create-constructor-argument branch fromd8905e2 to7b77f35CompareSeptember 1, 2018 07:52
@komik966komik966force-pushed theserializer-extract-create-constructor-argument branch from7b77f35 to2c37a8bCompareSeptember 23, 2018 10:03
@fabpot
Copy link
Member

@dunglas The code has changed since your last review. Can you check it again please?

@dunglas
Copy link
Member

@fbourigault would you mind to take a look, as it is related with#28775

@fbourigault
Copy link
Contributor

What about introducing someObjectInstanciator actor to handle the wholeinstantiateObject stuff ?
If designed with the problem this PR is trying to solve, we could have a better extension point and continue the split of the ObjectNormalizer/AbstractObjectNormalizer/AbstractNormalizer into smaller classes.
This fit more in plans exposed by@dunglas in#19374 (comment).

@komik966
Copy link
Author

I tried something similar (komik966@d734f32) but ended with lot of deprecations.

@komik966
Copy link
Author

I noticed that this code:

protectedfunctioncreateConstructorArgument($parameterData,string$key,\ReflectionParameter$constructorParameter,array &$context,string$format =null)
has similar logic to:
privatefunctionvalidateAndDenormalize(string$currentClass,string$attribute,$data, ?string$format,array$context)

Should we treat creating arguments for constructor differently than for other methods (setters)?

* @param array $context
* @param string|null $format
*
* @return mixed value of constructor parameter - an argument

Choose a reason for hiding this comment

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

I'm not sure the docblock lines above provide enough value to be kept, I'd personally suggest to remove them.

returnnull;
}
try {
if (null !==$constructorParameter->getClass()) {

Choose a reason for hiding this comment

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

looks like this could be turned to

if (null === $constructorParameter->getClass()) {    return $parameterData;}

and allow unindenting the block currently inside the "if"?

@fabpot
Copy link
Member

What's the status here?

teohhanhui and michnaadam33 reacted with thumbs up emoji

@dunglas
Copy link
Member

@komik966 do you think it is worth it to finish this PR? If yes, could you rebase and fix the remaining comments please?

@dunglas
Copy link
Member

Status: Needs Work

@komik966
Copy link
Author

I hasn't followed what's happening in serializer and api-platform for one year. So it's safer to abandon this PR.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@dunglasdunglasdunglas approved these changes

+1 more reviewer

@srozesrozesroze requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

7 participants

@komik966@dunglas@fabpot@fbourigault@nicolas-grekas@sroze@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp