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

[Security] Replace serialization API#30052

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
nicolas-grekas merged 1 commit intosymfony:masterfromrenanbr:serialization-strategy
Feb 7, 2019
Merged

[Security] Replace serialization API#30052

nicolas-grekas merged 1 commit intosymfony:masterfromrenanbr:serialization-strategy
Feb 7, 2019

Conversation

@renanbr
Copy link

@renanbrrenanbr commentedJan 31, 2019•
edited by nicolas-grekas
Loading

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

NewgetState() andsetState() methods inAbstractToken andAuthenticationException allow users to append data to the serialization payload.

It allow us to have zero impact in user land when changing the serialization engine.

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.

Looks great thanks. Here are some minor comments.

@nicolas-grekas
Copy link
Member

Don't forget to update UPGRADE-4.3 and the CHANGELOG of the component also!

@nicolas-grekasnicolas-grekas added this to thenext milestoneFeb 1, 2019
@nicolas-grekas
Copy link
Member

BC breaks? | yes, it removes \Serializable interface from AbstractToken and AuthenticationException

it does not actually so there is no BC break (and serialized payloads are kept compatible) - the PR description should be updated.

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.

(with one minor comment)

Copy link
Member

@chalasrchalasr left a comment•
edited
Loading

Choose a reason for hiding this comment

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

once remaining comments addressed

@xabbuh
Copy link
Member

@renanbr Looks like this PR needs a rebase as it contains unrelated changes now.

renanbr reacted with thumbs up emoji

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolas-grekas
Copy link
Member

Thank you@renanbr.

@nicolas-grekasnicolas-grekas merged commit006c6dd intosymfony:masterFeb 7, 2019
nicolas-grekas added a commit that referenced this pull requestFeb 7, 2019
This PR was merged into the 4.3-dev branch.Discussion----------[Security] Replace serialization API| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aNew `getState()` and `setState()` methods in `AbstractToken` and `AuthenticationException` allow users to append data to the serialization payload.It allow us to have zero impact in user land when changing the serialization engine.Commits-------006c6dd makes serialize methods final
@renanbrrenanbr deleted the serialization-strategy branchFebruary 7, 2019 09:21
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@renanbr@nicolas-grekas@xabbuh@ro0NL@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp