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

[RateLimiter] add the ability to use aClock inside theRateLimiter#49222

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

Open
xabbuh wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromxabbuh:issue-47999

Conversation

xabbuh
Copy link
Member

@xabbuhxabbuh commentedFeb 3, 2023•
edited by yceruto
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#47999
LicenseMIT
Doc PR

javiereguiluz reacted with rocket emoji
@xabbuhxabbuhforce-pushed theissue-47999 branch 4 times, most recently fromb4b0904 toc1b4bdeCompareFebruary 7, 2023 15:24
@nicolas-grekas
Copy link
Member

As discussed on Slack some classes are meant to be serialized (Window, SlidingWindow, TokenBucket, maybe others).
We need to figure out a way to use the current clock for them.
I think this means we shouldn't change their constructor but instead add and use a withClock method.
WDYT?

@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
@fabpot
Copy link
Member

@nicolas-grekas@xabbuh What are the next steps here?

@xabbuhxabbuh modified the milestones:7.1,7.2Apr 25, 2024
@xabbuhxabbuhforce-pushed theissue-47999 branch 2 times, most recently fromd854101 toa7af865CompareJuly 30, 2024 09:44
@xabbuhxabbuhforce-pushed theissue-47999 branch 2 times, most recently fromcaefae2 to85a6a5dCompareJuly 30, 2024 11:36
@xabbuh
Copy link
MemberAuthor

I have updated the PR to explicitly set the clock forLimiterStateInterface implementations after they have been unserialised. It's ready from my side.

reviews welcome

@OskarStarkOskarStark changed the title[RateLimiter] add the ability to use a Clock inside the RateLimiter[RateLimiter] add the ability to use aClock inside theRateLimiterJul 30, 2024
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.

I think this PR should be revised with this in mind: value objects should remain free from services.
Putting a clock service in a CacheStorage doesn't make sense to me.
e.g. for the Window class, there's already a way to define the current time: always pass $now to the add() method.
This logic should be applied to challenge all added clock services, so that we add it only to services.

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@maxbeckersmaxbeckersmaxbeckers left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas requested changes

@evertharmelingevertharmelingevertharmeling requested changes

@OskarStarkOskarStarkOskarStark approved these changes

@wouterjwouterjAwaiting requested review from wouterj

@chalasrchalasrAwaiting requested review from chalasr

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[RateLimiter] Make time dependency explicit and mockable
8 participants
@xabbuh@nicolas-grekas@fabpot@evertharmeling@stof@OskarStark@maxbeckers@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp