Skip to content
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

[FrameworkBundle] Allow to avoid limit definition in a RateLimiter configuration when using the no_limit policy #48275

Merged

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Nov 21, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #48268
License MIT
Doc PR N/A

As stated in the issue, DX will be improved if we can avoid setting a limit when using no_limit policy.
Targets 6.3, as it can be considered as a feature.

@carsonbot carsonbot added Status: Needs Review Bug DX DX = Developer eXperience (anything that improves the experience of using Symfony) FrameworkBundle labels Nov 21, 2022
@carsonbot carsonbot added this to the 5.4 milestone Nov 21, 2022
@carsonbot carsonbot changed the title [FrameworkBundle][DX] Allow to avoid limit definition in a RateLimiter configuration when using the no_limit policy [FrameworkBundle] Allow to avoid limit definition in a RateLimiter configuration when using the no_limit policy Nov 21, 2022
@fabpot
Copy link
Member

fabpot commented Nov 22, 2022

This PR should target 6.3.

@alexandre-daubois alexandre-daubois changed the base branch from 5.4 to 6.2 Nov 22, 2022
@alexandre-daubois alexandre-daubois force-pushed the fix/ratelimiter-no-limit branch 2 times, most recently from ae43058 to 38448c5 Compare Nov 22, 2022
@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Nov 22, 2022

Updated base branch + added the related CHANGELOG.md entry.

@MatTheCat
Copy link
Contributor

MatTheCat commented Nov 22, 2022

This seems like a bugfix to me 🤔

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Nov 22, 2022

@MatTheCat I understand your point of view and went for a bugfix as well at first sight. Fundamentally, the no_limit policy works well without this PR. Here, we actually "just" improve the developer experience. I think this is why it is more considered as a feature, rather than a bugfix.

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 6.3 Dec 13, 2022
@@ -2119,6 +2118,10 @@ private function addRateLimiterSection(ArrayNodeDefinition $rootNode, callable $
->end()
->end()
->end()
->validate()
->ifTrue(function ($v) { return 'no_limit' !== $v['policy'] && null === ($v['limit'] ?? null); })
Copy link
Member

@nicolas-grekas nicolas-grekas Dec 13, 2022

Choose a reason for hiding this comment

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

!isset() is just fine, let's not use fancy syntax when not needed.

…ter configuration when using the `no_limit` policy
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 13, 2022

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit c1d281d into symfony:6.3 Dec 13, 2022
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug DX DX = Developer eXperience (anything that improves the experience of using Symfony) FrameworkBundle Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FrameworkBundle][RateLimiter] no_limit policy requires a limit to be configured
5 participants