Skip to content

[RateLimiter] Fix cache storage (use namespaced pool + remove \Serializable) #38367

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 2 commits into from
Oct 2, 2020
Merged

[RateLimiter] Fix cache storage (use namespaced pool + remove \Serializable) #38367

fabpot merged 2 commits into from
Oct 2, 2020

Conversation

@wouterj
Copy link
Member

@wouterj wouterj commented Oct 1, 2020

Q A
Branch? master
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #38365, fix #38338
License MIT
Doc PR -

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 1, 2020
@wouterj wouterj force-pushed the issue-38365-38338/cache-storage-serializer branch 2 times, most recently from 6e26480 to 643840e Compare October 1, 2020 10:52
nicolas-grekas
nicolas-grekas reviewed Oct 1, 2020
View reviewed changes
@wouterj wouterj force-pushed the issue-38365-38338/cache-storage-serializer branch 3 times, most recently from 4a3eff0 to 64809db Compare October 1, 2020 11:28
nicolas-grekas
nicolas-grekas reviewed Oct 1, 2020
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Oct 1, 2020
View reviewed changes
@wouterj wouterj force-pushed the issue-38365-38338/cache-storage-serializer branch from 64809db to 181e6dc Compare October 1, 2020 11:42
nicolas-grekas
nicolas-grekas approved these changes Oct 1, 2020
View reviewed changes
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)

@wouterj wouterj force-pushed the issue-38365-38338/cache-storage-serializer branch from 181e6dc to 027305e Compare October 1, 2020 11:52
stof
stof reviewed Oct 1, 2020
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

shouldn't we even have a separate cache pool per limiter to avoid cache key conflicts ?

@wouterj wouterj force-pushed the issue-38365-38338/cache-storage-serializer branch from 027305e to 251c202 Compare October 1, 2020 12:20
@wouterj
Copy link
Member Author

wouterj commented Oct 1, 2020

shouldn't we even have a separate cache pool per limiter to avoid cache key conflicts ?

The cache keys are prefixed by the limiter ID - which is unique for each limiter. So key conflicts should not be possible.

@fabpot
Copy link
Member

fabpot commented Oct 2, 2020

Thank you @wouterj.

@fabpot fabpot merged commit d0ded92 into symfony:master Oct 2, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees

No one assigned

Labels

Bug Status: Reviewed

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

[RateLimiter] Create & use a custom cache pool [RateLimiter] LimiterStateInterface should not extend Serializable

5 participants

@wouterj @fabpot @nicolas-grekas @stof @carsonbot