Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 7, 2019
Merged

[Security] Replace serialization API #30052

merged 1 commit into from
Feb 7, 2019

Conversation

renanbr
Copy link
Contributor

@renanbr renanbr commented Jan 31, 2019

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/a

New 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.

Copy link
Member

@nicolas-grekas nicolas-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-grekas nicolas-grekas added this to the next milestone Feb 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-grekas nicolas-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

@chalasr chalasr left a comment

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

xabbuh commented Feb 7, 2019

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

Copy link
Member

@xabbuh xabbuh 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-grekas nicolas-grekas merged commit 006c6dd into symfony:master Feb 7, 2019
nicolas-grekas added a commit that referenced this pull request Feb 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/a

New `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
@renanbr renanbr deleted the serialization-strategy branch February 7, 2019 09:21
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants