[Serializer] deprecated normalizers and encoders who dont implement the base interfaces #27819

rodnaph · 2018-07-03T11:14:40Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	None
License	MIT
Doc PR	None

Currently the Serializer can be constructed with any object regardless of whether or not it implements NormalizerInterface or DenormalizerInterface. This object will then be ignored when getting a normalizer/denormalizer, so in effect silently ignored for serializer operations.

This change throws an exception on construct if a given normalizer object does not implement one of these interfaces - are there use cases where this would not be true?

dunglas

Why not

nicolas-grekas · 2018-07-03T15:42:05Z

Has this been introduced for 4.2? If not, this should throw a deprecation instead I suppose?
Note that the exception should be one line per our CS.

nicolas-grekas

(see comment)

rodnaph · 2018-07-03T16:32:03Z

@nicolas-grekas Ok I'll switch the exception to a deprecation warning - can you advise which since/to versions this warning should read?

eg. Passing normalizers which don't implement either NormalizerInterface or DenormalizerInterface has been deprecated since X and will throw an exception in Y

nicolas-grekas · 2018-07-03T20:58:46Z

LGTM, let's propose this yes.

rodnaph · 2018-07-04T09:35:06Z

@nicolas-grekas Switched to deprecation with docs.

nicolas-grekas

just some minor comment

nicolas-grekas · 2018-07-04T09:38:23Z

UPGRADE-4.2.md

@@ -64,3 +64,5 @@ Serializer

 * Relying on the default value (false) of the "as_collection" option is deprecated since 4.2.
   You should set it to false explicitly instead as true will be the default value in 5.0.
+ * Deprecated creating a `Serializer` with normalizers which do not implement
+ either `NormalizerInterface` or `DenormalizerInterface`.


missing indentation

nicolas-grekas · 2018-07-04T09:38:43Z

UPGRADE-5.0.md

+----------
+
+ * Creating a `Serializer` with normalizers that do not implement either
+ `NormalizerInterface` or `DenormalizerInterface` will now throw an exception.


missing indentation

nicolas-grekas · 2018-07-04T09:38:54Z

src/Symfony/Component/Serializer/CHANGELOG.md

+-----
+
+* deprecated creating a `Serializer` with normalizers which do not implement
+ either `NormalizerInterface` or `DenormalizerInterface`.


indentation

nicolas-grekas · 2018-07-04T09:39:45Z

src/Symfony/Component/Serializer/Serializer.php

+
+            if (!($normalizer instanceof NormalizerInterface || $normalizer instanceof DenormalizerInterface)) {
+                @trigger_error(\sprintf('Passing normalizers (%s) which do not implement either %s or %s has been deprecated since 4.2 and will throw an exception in 5.0', get_class($normalizer), NormalizerInterface::class, DenormalizerInterface::class), E_USER_DEPRECATED);
+                // throw new \InvalidArgumentException(\sprintf('The class %s does not implement %s or %s', get_class($normalizer), NormalizerInterface::class, DenormalizerInterface::class));


please add double quotes around all the "%s"
the " and will throw an exception in 5." part should be removed
and a final dot added

@nicolas-grekas removing the 5.0 part is not done in other places, btw? https://github.com/symfony/symfony/pull/26564/files#diff-9d63a61ac1b3720a090df6b1015822f2R729

it should have been :)

rodnaph · 2018-07-04T09:55:13Z

@nicolas-grekas Done, thanks.

nicolas-grekas · 2018-07-04T09:57:11Z

src/Symfony/Component/Serializer/Serializer.php

@@ -78,6 +78,11 @@ public function __construct(array $normalizers = array(), array $encoders = arra
            if ($normalizer instanceof NormalizerAwareInterface) {
                $normalizer->setNormalizer($this);
            }
+
+            if (!($normalizer instanceof NormalizerInterface || $normalizer instanceof DenormalizerInterface)) {
+                @trigger_error(\sprintf('Passing normalizers ("%s") which do not implement either "%s" or "%s" has been deprecated.', get_class($normalizer), NormalizerInterface::class, DenormalizerInterface::class), E_USER_DEPRECATED);


has been deprecated since Symfony 4.2.

ogizanagi · 2018-07-04T22:20:03Z

Injecting something else than a NormalizerInterface or DenormalizerInterface wouldn't make sense and would be a misuse in the first place. So I don't understand why we should trigger a deprecation here rather than throw the exception directly. It works and is only ignored by luck, as we share the same property for normalizers and denormalizers and use instanceof checks in getNormalizer/getDenormalizer.
If any app is misusing it, this exception would be raised by any test covering part of the app using the serializer and should be fixed right away.

dunglas · 2018-07-04T22:40:17Z

Altenative proposal:

    /**
     * @param NormalizerInterface[]|DenormalizerInterface[] $normalizers
     * @param EncoderInterface[]|DecoderInterface[] $encoders
     */
    public function __construct(array $normalizers = array(), array $encoders = array())

No deprecation, no overhead.

dunglas · 2018-07-04T22:41:44Z

Should be done for encoders too btw.

nicolas-grekas · 2018-07-05T06:21:39Z

Good idea to add the docblock @dunglas, let's do that. The instanceof checks are mostly free not sure that's overhead. Don't you want to have an exception in 5.0?
@ogizanagi let's not argue about when we're ok to break BC, we already made the mistake. A deprecation is not the end of the world.

rodnaph · 2018-07-05T06:44:49Z

Regards docblock... My reason for adding this check was having lost time debugging a mistake with a normalizer service I was injecting, a runtime exception would have be the quickest notification (docs or static analysis probably wouldn't have highlighted it for me in that instance, but of course worth having).

nicolas-grekas · 2018-07-05T07:09:55Z

Still let's add them also :)

rodnaph · 2018-07-05T07:20:16Z

👍

ogizanagi · 2018-07-05T07:28:31Z

src/Symfony/Component/Serializer/Serializer.php

@@ -64,6 +64,10 @@ class Serializer implements SerializerInterface, ContextAwareNormalizerInterface
    private $denormalizerCache = array();
    private $normalizerCache = array();

+    /**
+     * @param NormalizerInterface[]|DenormalizerInterface[] $normalizers


The syntax we previously used for such arrays is (NormalizerInterface|DenormalizerInterface)[], like in

symfony/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php

Line 30 in 04b2c2d

* @param (Role|string)[] $roles An array of roles

But as it's still not supported by IDEs AFAIK 👍 to keep as is.

The current syntax is a bit miss leading as it suggests either array of NormalizerInterface or an array of DenormalizerInterface but not mixed.

dunglas · 2018-09-05T05:44:16Z

Can you rebase please?

…he base interfaces

nicolas-grekas

(rebased)

fabpot · 2018-09-25T09:53:33Z

Thank you @rodnaph.

…ont implement the base interfaces (rodnaph) This PR was merged into the 4.2-dev branch. Discussion ---------- [Serializer] deprecated normalizers and encoders who dont implement the base interfaces | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | None | License | MIT | Doc PR | None Currently the `Serializer` can be constructed with any object regardless of whether or not it implements `NormalizerInterface` or `DenormalizerInterface`. This object will then be ignored when getting a normalizer/denormalizer, so in effect silently ignored for serializer operations. This change throws an exception on construct if a given normalizer object does not implement one of these interfaces - are there use cases where this would not be true? Commits ------- cbc2be8 [Serializer] deprecated normalizers and encoders who dont implement the base interfaces

…ders passed to Serializer (ogizanagi) This PR was merged into the 5.0-dev branch. Discussion ---------- [Serializer] Throw exception on invalid normalizers/encoders passed to Serializer | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #27819  | License | MIT | Doc PR | N/A  As planned in #27819 Commits ------- 5ab6ad4 [Serializer] Throw exception on invalid normalizers/encoders passed to Serializer

rodnaph requested a review from dunglas as a code owner July 3, 2018 11:14

carsonbot added Status: Needs Review Feature labels Jul 3, 2018

dunglas approved these changes Jul 3, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 3, 2018

sroze approved these changes Jul 3, 2018

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Reviewed labels Jul 3, 2018

nicolas-grekas added this to the next milestone Jul 3, 2018

nicolas-grekas requested changes Jul 3, 2018

View reviewed changes

nicolas-grekas changed the title ~~Invalid Normalizer Exception~~ [Serializer] Invalid Normalizer Exception Jul 4, 2018

nicolas-grekas reviewed Jul 4, 2018

View reviewed changes

ogizanagi approved these changes Jul 5, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jul 5, 2018

nicolas-grekas changed the title ~~[Serializer] Invalid Normalizer Exception~~ [Serializer] deprecated normalizers and encoders who dont implement the base interfaces Sep 21, 2018

[Serializer] deprecated normalizers and encoders who dont implement t…

cbc2be8

…he base interfaces

nicolas-grekas approved these changes Sep 21, 2018

View reviewed changes

fabpot approved these changes Sep 25, 2018

View reviewed changes

fabpot merged commit cbc2be8 into symfony:master Sep 25, 2018

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

ogizanagi mentioned this pull request May 29, 2019

[Serializer] Throw exception on invalid normalizers/encoders passed to Serializer #31693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] deprecated normalizers and encoders who dont implement the base interfaces #27819

[Serializer] deprecated normalizers and encoders who dont implement the base interfaces #27819

rodnaph commented Jul 3, 2018

dunglas left a comment

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

nicolas-grekas left a comment

rodnaph commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

rodnaph commented Jul 4, 2018

nicolas-grekas left a comment

nicolas-grekas Jul 4, 2018

nicolas-grekas Jul 4, 2018

nicolas-grekas Jul 4, 2018

nicolas-grekas Jul 4, 2018

bendavies Jul 4, 2018

nicolas-grekas Jul 4, 2018

rodnaph commented Jul 4, 2018

nicolas-grekas Jul 4, 2018

rodnaph Jul 4, 2018

ogizanagi commented Jul 4, 2018

dunglas commented Jul 4, 2018 •

edited

Loading

dunglas commented Jul 4, 2018

nicolas-grekas commented Jul 5, 2018

rodnaph commented Jul 5, 2018

nicolas-grekas commented Jul 5, 2018 •

edited

Loading

rodnaph commented Jul 5, 2018

ogizanagi Jul 5, 2018

mcfedr Sep 5, 2018

dunglas commented Sep 5, 2018

nicolas-grekas left a comment

fabpot commented Sep 25, 2018

[Serializer] deprecated normalizers and encoders who dont implement the base interfaces #27819

[Serializer] deprecated normalizers and encoders who dont implement the base interfaces #27819

Conversation

rodnaph commented Jul 3, 2018

dunglas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Jul 3, 2018 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

rodnaph commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

rodnaph commented Jul 4, 2018

nicolas-grekas 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodnaph commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Jul 4, 2018

dunglas commented Jul 4, 2018 • edited Loading

dunglas commented Jul 4, 2018

nicolas-grekas commented Jul 5, 2018

rodnaph commented Jul 5, 2018

nicolas-grekas commented Jul 5, 2018 • edited Loading

rodnaph commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Sep 5, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Sep 25, 2018

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

dunglas commented Jul 4, 2018 •

edited

Loading

nicolas-grekas commented Jul 5, 2018 •

edited

Loading