[serializer] extract normalizer tests to traits #30888

dbu · 2019-04-06T11:55:42Z

eufossa

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	Relates to #30818
License	MIT
Doc PR	-

As discussed with @joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers.

Rebase when [serializer] prevent mixup in normalizer of the object to populate #30977, [Serializer] Also validate callbacks when given in the normalizer context #30950 and [Serializer] Respect ignored attributes in cache key of normalizer #30907 are merged to master blocker
Clean up order of trait inclusion and methods in the tests
Clean up fixture classes of the traits. I started having one class named the same as the trait, where possible

Stuff that we should do eventually, but can also do in separate pull requests, after this one has been merged:

Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait)
Run test coverage and increase coverage so that we cover all important features and all relevant error cases.

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

dunglas

What a work! This looks very promising.

dunglas · 2019-04-07T10:27:53Z

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php

+     * You can raise this value for special cases, e.g. in combination with the
+     * max depth setting of the object normalizer.
+     */
+    public const CIRCULAR_REFERENCE_LIMIT = 'circular_reference_limit';


What do you think about moving all these constants in the interface, and to add a builder object for better autocompletion in IDEs (exactly as done in HttpClient)?

i agree that they should be in the interface. otherwise the interface is a lie. and we should define that when an option is not known by the normalizer implementation, it has to throw an exception. otherwise the behaviour is very undefined and e.g. typos are not spotted.

i also think we could use the OptionsResolver on them to validate, instead of the case-by-case validation we do at the moment (see #30907, #30950, its too easy to forget to validate things)

@joelwurtz should we move the constants in this PR? i feel we should probably do that separately, also reviewing if they all stay the same or we want some different options and only keep some for legacy support.

Like explained in #30818 we should have a global politic about context and where to put const of this context. I would be in favor of @dunglas proposal in order to be consistent with how others components works.

But it's clearly not in the scope of this PR. And this is something that should be done once we are satisified with the overall implementation. First let's do a good implementation, then we focus on DX for that, WDYT @dunglas ?

otherwise the behaviour is very undefined and e.g. typos are not spotted

It’s not really doable because most likely the context will be passed to the Serializer instance, that will call many normalizers (another normalizer can be used for relations for instance).

+1 to move these constants in another PR.

could we then make the serializer validate the context? e.g. by collecting from all its registered normalizers what options they support?

i am just very unhappy with these kind of unvalidated array configuration, as its super hard to debug when you make mistakes.

is the context supposed to also contain custom options for specific normalizers or is it a closed list of features? if it is closed, we could change from array to a value object that does the validation and defines what keys are possible. then we could do the validation once in that object and all normalizers can rely on only getting valid information.

is the context supposed to also contain custom options for specific normalizers or is it a closed list of features?

Yes, it is an extension point for custom normalizers and encoders too. It’s used (and sometimes abused) a lot by API Platform and many other tools using this component.

What we would to is to validate when using the new « context builder » as in HttpClient (when using this builder, we know the list of keys) and don’t validate anything (as currently) in the normalizers.
Another alternative would be to introduce a new parameter for « built-in » options that would be a class, but keep the context as an extension point (I’m less fond of this one because it will make the public API harder to learn).

could we make the $context into a value object with explicit methods (and validation) for the known options, and a generic getExtra(string $name), setExtra(string $name, mixed $thing) method? this would make a good compromise to solve validation and the typo risk issue while keeping flexibility.

i dislike that every context validates the context parameters again, as they can be called repeatedly.

this is out of scope for this pull request. i don't have time for another pull request for it, but if somebody wants to do it, please do

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

nicolas-grekas · 2019-04-07T21:12:25Z

Please note that we will ask you to squash the git history of the PR before merging, to keep at most one commit per author.

joelwurtz · 2019-04-08T06:57:55Z

About squashing, if it's easier for you @dbu i don't mind having my name removed from the commit list

dbu · 2019-04-08T07:37:08Z

i squashed. i don't feel much of an author on those, mostly moved the tests from elsewhere :-D

fabpot · 2019-04-08T09:02:42Z

@dbu Is it still WIP?

dbu · 2019-04-08T09:12:44Z

yes, still WIP. i want to go over the changes to streamline them a bit, and i want to wait for the bugfixes #30977, #30950 and #30907 landing in master to rebase and solve the conflicts that will happen.

…malizer context (dbu) This PR was merged into the 4.2 branch. Discussion ---------- [Serializer] Also validate callbacks when given in the normalizer context | Q | A | ------------- | --- | Branch? | 4.2 (callbacks are handled differently in 3.4) | Bug fix? | yes | New feature? | no | BC breaks? | no (unless somebody relied on this bug ignoring `null` as callback | Deprecations? | no | Tests pass? | yes | Fixed tickets | Related to #30888 | License | MIT | Doc PR | - callbacks configuration for the normalizer is validated to be valid callbacks when using setCallbacks or using the callbacks field in the default options. however, it was not validated when using the callbacks field in a context passed to `normalize()` Commits ------- 3789152 [serializer] validate that the specified callbacks and max_depth_handler are actually callable

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php

dbu · 2019-04-11T05:55:22Z

I now consider this ready for final review /cc @joelwurtz @dunglas

the open points can be done in follow-up pull requests and i don't have any more time to start fresh things (but will of course wrap up if you have feedback on this PR so that we can merge it)

… isAllowedAttribute (dbu) This PR was merged into the 3.4 branch. Discussion ---------- property normalizer should also pass format and context to isAllowedAttribute | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | found while working on #30888 | License | MIT | Doc PR | - the context and format are optional parameters to `isAllowedAttribute`, but should be forwarded. due to this omission, the PropertyNormalizer was ignoring the 'attributes' context option (and does in version 4 also ignore the 'ignore_attributes' context option - that one is a property on the normalizer class in version 3 and therefore not ignored here) Commits ------- 13e2fb7 property normalizer should also pass format and context to isAllowedAttribute

dbu · 2019-04-26T11:03:27Z

what do we need to be able to merge this?

fabpot · 2019-04-27T16:25:58Z

Thank you @dbu.

@joelwurtz

This PR was squashed before being merged into the 4.3-dev branch (closes #30888). Discussion ---------- [serializer] extract normalizer tests to traits eufossa | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | Relates to #30818 | License | MIT | Doc PR | - As discussed with @joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers. * [x] Rebase when #30977, #30950 and #30907 are merged to master **blocker** * [x] Clean up order of trait inclusion and methods in the tests * [x] Clean up fixture classes of the traits. I started having one class named the same as the trait, where possible Stuff that we should do eventually, but can also do in separate pull requests, after this one has been merged: * [ ] Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait) * [ ] Run test coverage and increase coverage so that we cover all important features and all relevant error cases. Commits ------- 2b6ebea [serializer] extract normalizer tests to traits

This PR was merged into the 3.4 branch. Discussion ---------- [Serializer] Fix tests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | n/a | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a #30888 added some tests on master which fail when `property-info <3.4.13` is installed. This bumps the constraint on 3.4 which will fix the low-deps build once merged up to master. Commits ------- d25f650 [Serializer] Fix tests

joelwurtz · 2019-04-29T10:29:42Z

Thanks you @dbu will update my prs with those test

…c (dunglas) This PR was merged into the 4.3 branch. Discussion ---------- [Serializer] Fix BC break: DEPTH_KEY_PATTERN must be public | Q | A | ------------- | --- | Branch? | 4.3 Bug fix? | yes | New feature? |no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | n/a | License | MIT | Doc PR | n/a Fix a BC break introduced by #30888. This constant is used by API Platform. Commits ------- 1bdf2af [Serializer] Fix BC break: DEPTH_KEY_PATTERN must be public

carsonbot added the Status: Needs Review label Apr 6, 2019

dbu force-pushed the refactor-normalizer-tests branch 5 times, most recently from 50d6a38 to 37adf7f Compare April 6, 2019 15:45

joelwurtz mentioned this pull request Apr 6, 2019

[Serializer][PropertyInfo][PropertyAccess] Refactoring plan for AbstractObjectNormalizer and related code #30818

Open

16 tasks

nicolas-grekas added this to the next milestone Apr 6, 2019

dbu mentioned this pull request Apr 7, 2019

[Serializer] Also validate callbacks when given in the normalizer context #30950

Merged

dbu force-pushed the refactor-normalizer-tests branch from 1388449 to bc48707 Compare April 7, 2019 08:12

dbu mentioned this pull request Apr 7, 2019

[Serializer] Add Support of recursive denormalization on object_to_populate #30607

Merged

dbu force-pushed the refactor-normalizer-tests branch from d1986aa to e5da4cd Compare April 7, 2019 09:32

jewome62 reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Show resolved Hide resolved

dbu force-pushed the refactor-normalizer-tests branch from 97741e2 to 526dbde Compare April 7, 2019 10:03

dunglas reviewed Apr 7, 2019

View reviewed changes

michaelcullum added the ⭐️ EUFOSSA Hackathon label Apr 7, 2019

nicolas-grekas reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated Show resolved Hide resolved

dbu force-pushed the refactor-normalizer-tests branch from 3f46692 to cbbda0b Compare April 8, 2019 07:35

dbu changed the title ~~WIP: extract normalizer tests to traits~~ WIP: [serializer] extract normalizer tests to traits Apr 8, 2019

maxhelias mentioned this pull request Apr 8, 2019

Allow serializer context configuration #28927

Closed

dbu force-pushed the refactor-normalizer-tests branch 2 times, most recently from 73d435f to 3dfd83b Compare April 9, 2019 06:00

dbu commented Apr 11, 2019

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/PropertyNormalizer.php Outdated Show resolved Hide resolved

dbu changed the title ~~WIP: [serializer] extract normalizer tests to traits~~ [serializer] extract normalizer tests to traits Apr 11, 2019

dbu force-pushed the refactor-normalizer-tests branch from 4fb71a7 to 22c1092 Compare April 11, 2019 05:53

dbu mentioned this pull request Apr 11, 2019

property normalizer should also pass format and context to isAllowedAttribute #31071

Merged

dbu force-pushed the refactor-normalizer-tests branch from 22c1092 to c83ea84 Compare April 26, 2019 11:01

carsonbot removed the Status: Needs Review label Apr 27, 2019

fabpot approved these changes Apr 27, 2019

View reviewed changes

carsonbot added the Status: Reviewed label Apr 27, 2019

[serializer] extract normalizer tests to traits

2b6ebea

fabpot force-pushed the refactor-normalizer-tests branch from c83ea84 to 2b6ebea Compare April 27, 2019 16:25

fabpot merged commit 2b6ebea into symfony:master Apr 27, 2019

chalasr mentioned this pull request Apr 27, 2019

[Serializer] Fix tests #31296

Merged

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

dunglas mentioned this pull request May 10, 2019

[Serializer] Fix BC break: DEPTH_KEY_PATTERN must be public #31465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serializer] extract normalizer tests to traits #30888

[serializer] extract normalizer tests to traits #30888

dbu commented Apr 6, 2019 •

edited

dunglas left a comment

dunglas Apr 7, 2019

dbu Apr 7, 2019

dbu Apr 7, 2019

joelwurtz Apr 7, 2019

dunglas Apr 7, 2019 •

edited

dbu Apr 7, 2019

dunglas Apr 7, 2019

dbu Apr 8, 2019

dbu Apr 11, 2019

nicolas-grekas commented Apr 7, 2019

joelwurtz commented Apr 8, 2019

dbu commented Apr 8, 2019

fabpot commented Apr 8, 2019

dbu commented Apr 8, 2019 •

edited

dbu commented Apr 11, 2019

dbu commented Apr 26, 2019

fabpot commented Apr 27, 2019

joelwurtz commented Apr 29, 2019

[serializer] extract normalizer tests to traits #30888

[serializer] extract normalizer tests to traits #30888

Conversation

dbu commented Apr 6, 2019 • edited

dunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Apr 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 7, 2019

joelwurtz commented Apr 8, 2019

dbu commented Apr 8, 2019

fabpot commented Apr 8, 2019

dbu commented Apr 8, 2019 • edited

dbu commented Apr 11, 2019

dbu commented Apr 26, 2019

fabpot commented Apr 27, 2019

joelwurtz commented Apr 29, 2019

dbu commented Apr 6, 2019 •

edited

dunglas Apr 7, 2019 •

edited

dbu commented Apr 8, 2019 •

edited