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] deprecated normalizers and encoders who dont implement the base interfaces#27819

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

Merged
fabpot merged 1 commit intosymfony:masterfromowsy:serializer-exception
Sep 25, 2018
Merged

Conversation

@rodnaph
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsNone
LicenseMIT
Doc PRNone

Currently theSerializer can be constructed with any object regardless of whether or not it implementsNormalizerInterface orDenormalizerInterface. 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?

apetitpa and jvasseur reacted with thumbs up emoji
Copy link
Member

@dunglasdunglas left a comment

Choose a reason for hiding this comment

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

Why not

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 3, 2018•
edited
Loading

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.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(see comment)

@rodnaph
Copy link
ContributorAuthor

@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
Copy link
Member

LGTM, let's propose this yes.

@rodnaph
Copy link
ContributorAuthor

@nicolas-grekas Switched to deprecation with docs.

@nicolas-grekasnicolas-grekas changed the titleInvalid Normalizer Exception[Serializer] Invalid Normalizer ExceptionJul 4, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

just some minor comment

* 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`.

Choose a reason for hiding this comment

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

missing indentation

----------

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

Choose a reason for hiding this comment

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

missing indentation

-----

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

Choose a reason for hiding this comment

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

indentation


if (!($normalizerinstanceof NormalizerInterface ||$normalizerinstanceof 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));

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

it should have been :)

@rodnaph
Copy link
ContributorAuthor

@nicolas-grekas Done, thanks.

}

if (!($normalizerinstanceof NormalizerInterface ||$normalizerinstanceof 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);

Choose a reason for hiding this comment

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

has been deprecated since Symfony 4.2.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Doh :(

@ogizanagi
Copy link
Contributor

Injecting something else than aNormalizerInterface orDenormalizerInterface 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 useinstanceof checks ingetNormalizer/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 reacted with thumbs up emoji

@dunglas
Copy link
Member

dunglas commentedJul 4, 2018•
edited
Loading

Altenative proposal:

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

No deprecation, no overhead.

@dunglas
Copy link
Member

Should be done for encoders too btw.

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

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
Copy link
Member

nicolas-grekas commentedJul 5, 2018•
edited
Loading

Still let's add them also :)

@rodnaph
Copy link
ContributorAuthor

👍

private$normalizerCache =array();

/**
* @param NormalizerInterface[]|DenormalizerInterface[] $normalizers
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@dunglas
Copy link
Member

Can you rebase please?

@nicolas-grekasnicolas-grekas changed the title[Serializer] Invalid Normalizer Exception[Serializer] deprecated normalizers and encoders who dont implement the base interfacesSep 21, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(rebased)

@fabpot
Copy link
Member

Thank you@rodnaph.

@fabpotfabpot merged commitcbc2be8 intosymfony:masterSep 25, 2018
fabpot added a commit that referenced this pull requestSep 25, 2018
…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        | NoneCurrently 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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
nicolas-grekas added a commit that referenced this pull requestMay 29, 2019
…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 <!-- please update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#27819   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | N/A <!-- required for new features -->As planned in#27819Commits-------5ab6ad4 [Serializer] Throw exception on invalid normalizers/encoders passed to Serializer
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+4 more reviewers

@bendaviesbendaviesbendavies left review comments

@mcfedrmcfedrmcfedr left review comments

@srozesrozesroze approved these changes

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

9 participants

@rodnaph@nicolas-grekas@ogizanagi@dunglas@fabpot@bendavies@mcfedr@sroze@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp