[11.x] Allow list of rate limiters without requiring unique keys

[timacdonald]

This PR ensures that rate limiters specifying multiple limits will always have unique keys.

The issue.

RateLimiter::for('limiter-name', fn () => [
    Limit::perSecond(1),
    Limit::perMinute(30),
]);

This rate limiter will always fail. No request would ever get through. Zero. Nada.

This is because both limits share the same key. The docs specify that If you're assigning multiple rate limits segmented by identical by values, you should ensure that each by value is unique

This is fine, but also can be something easily forgotten; bit of a footgun.

The improvement

This PR attempts to improve the situation by ensuring that when a list of limits has duplicate keys they are modified to be unique.

RateLimiter::for('limiter-name', fn () => [
    Limit::perSecond(1),
    Limit::perMinute(30)->by(Auth::user()),
    Limit::perDay(500),
]);

In this above example, the first and the last limits would have their key modified to be unique.

Although this is a breaking change, it only breaks in already broken situations, so I see this as an overall improvement.

I don't feel like we need to update the docs. This is just a fallback fix for those rare times it is held wrong.

---

fixes #53157

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[gcg]

After deploying our app with laravel 11.29 (which this pr is a part of) today, all our users started getting rate limit errors for 40 minutes (until we discovered the problem). I temporarily downgraded and deployed which fixed our problem and will investigate further on what combination was the actual cause.

But i am leaving this clue here so if someone searches for rate limit and laravel 11.29 maybe they will find the issue faster.

edit#1: only grouped rate limits failed, which this pr also targets so i am 99% sure this is breaking some live apps.

update#2: probably my bad. missed in the docs that we need to uniquely name our by limits part. I will name them and test it again.

[NickSdot]

@gcg wasn't this PR all about that you no longer need to give unique names?

[gcg]

@gcg wasn't this PR all about that you no longer need to give unique names?

Still did not test ALL possible combinations but I was using the exact same example w/out giving unique names and after this PR, those 2 limiters that returns multiple limits in an array w/out unique names started failing all requests.

I will share more information but giving unique key names manually seems to be working and it's clearly documented so I am in the blame here :) I am reporting live here just in case it happens to someone else.

[timacdonald]

Thanks for letting us know, @gcg.

Please let us know what you find out. My gut feeling is that your previous set up, sharing the same keys, was likely not working as expected even if it seemed to be working.

Keep me posted.

[timacdonald]

And sorry for the troubles!

[gcg]

Please let us know what you find out. My gut feeling is that your previous set up, sharing the same keys, was likely not working as expected even if it seemed to be working.

yeah, you are probably right. I had 2 limiters that returns list limits, one of them is so rarely used and the other route has it's own custom rate limits built in predating laravel mw, even if the ratelimiter didn't work before for that case, I probably didn't notice and only noticed once we started failing requests.

Keep me posted.

I redeployed rate limiters after uniquely naming everything and now it's back to normal.

And sorry for the troubles!

It's not your fault that I missed the warning in the documentation, as u stated, it wasn't probably working as it should from the beginning.