8000 [RateLimiter] Fix sleep value by jderusse · Pull Request #40658 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RateLimiter] Fix sleep value #40658

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
merged 1 commit into from
Apr 1, 2021
Merged

[RateLimiter] Fix sleep value #40658

merged 1 commit into from
Apr 1, 2021

Conversation

jderusse
Copy link
Member
Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #40656
License MIT
Doc PR

sleep vs usleep

@jderusse jderusse requested a review from wouterj March 31, 2021 17:11
@carsonbot carsonbot added this to the 5.2 milestone Mar 31, 2021
Nyholm
Nyholm requested changes Mar 31, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Your change is correct. But I have one suggestion.

@jderusse jderusse requested a review from Nyholm March 31, 2021 19:33
Nyholm
Nyholm approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@Nyholm
Copy link
Member
Nyholm commented Mar 31, 2021

Btw, doing sleep(-3) is a fatal error in PHP8. (https://3v4l.org/lK3uR)

Are we sure this method is always called when there are time left to wait? Or should we do max(0, ...) just to be sure?

Nyholm
Nyholm approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

Sorry for all the comments on this small patch.

@Nyholm Nyholm added the Ready label Mar 31, 2021
@Nyholm Nyholm mentioned this pull request Mar 31, 2021
Closed
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 4d23fec into symfony:5.2 Apr 1, 2021
@fabpot fabpot mentioned this pull request May 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Bug RateLimiter Ready Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
4 participants
@jderusse @Nyholm @nicolas-grekas @carsonbot
0