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

[Security] Replace serialization API #30052

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
6 participants
@renanbr
Copy link

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.

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

Looks great thanks. Here are some minor comments.

Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php Outdated
Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Component/Security/Core/Exception/AuthenticationException.php Outdated
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 1, 2019

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 1, 2019

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.

@nicolas-grekas nicolas-grekas added Deprecation and removed BC Break labels Feb 1, 2019

@renanbr renanbr force-pushed the renanbr:serialization-strategy branch 3 times, most recently from 9ff7631 to 0efbcc5 Feb 1, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

(with one minor comment)

@renanbr renanbr force-pushed the renanbr:serialization-strategy branch 2 times, most recently from 3f21405 to e7e9a04 Feb 1, 2019

Show resolved Hide resolved UPGRADE-4.3.md Outdated
Show resolved Hide resolved src/Symfony/Component/Security/CHANGELOG.md Outdated
@chalasr

chalasr approved these changes Feb 4, 2019

Copy link
Member

chalasr left a comment

once remaining comments addressed

@renanbr renanbr force-pushed the renanbr:serialization-strategy branch 2 times, most recently from 7a8123e to f085b97 Feb 5, 2019

Show resolved Hide resolved UPGRADE-5.0.md Outdated

@renanbr renanbr force-pushed the renanbr:serialization-strategy branch from f085b97 to dcb8568 Feb 6, 2019

@renanbr renanbr requested a review from xabbuh as a code owner Feb 6, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 7, 2019

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

renanbr

@renanbr renanbr force-pushed the renanbr:serialization-strategy branch from dcb8568 to 006c6dd Feb 7, 2019

@xabbuh

xabbuh approved these changes Feb 7, 2019

Copy link
Member

xabbuh left a comment

LGTM

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2019

Thank you @renanbr.

@nicolas-grekas nicolas-grekas merged commit 006c6dd into symfony:master Feb 7, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 7, 2019

feature #30052 [Security] Replace serialization API (renanbr)
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 renanbr:serialization-strategy branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment