-
Notifications
You must be signed in to change notification settings - Fork 75
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
Drop @ratelimit Annotation, Create #[RateLimit] Attribute, Drop php7 support, Drop symfony 3 & 4 support #132
Conversation
Also forget to mention "Drop sensio/framework-extra-bundle" |
Hum, looks like upload coverage is also broken for main repository... |
Something wrong with that pull request ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great! just two minor comments on my side
…support, Drop symfony 3 & 4 support
@axi thanks a lot. while solving the conflicts i had to rebase your branch, but now somehow github show my face on the commit despite the fact that the commit is authored by you. 😕 |
@goetas no worries, thanks for mentioning it |
Should fix #130.
There is still (at least) one thing I would like somebody to check: the way I get Attributes in
RateLimitAnnotationListener::getRateLimitsFromAttributes()
. I kind of mixed the ways I found inSymfony\Component\HttpKernel\Event\ControllerEvent::setController()
&Symfony\Component\HttpKernel\Event\ControllerEvent::getAttributes()
(sf6). I should maybe try to use getAttributes() method from ControllerEvent if it exists (sf6) and use my current implementation as a fallback for Sf5.4 ?I tried to make it the most compatible possible.
Also, I added a condition for uploading coverage in ci but I must verify it still works for non forks.
Let me know what you think.