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

[11.x] Throw exception if named rate limiter and model property do not exist #50908

Conversation

mateusjatenee
Copy link
Contributor

@mateusjatenee mateusjatenee commented Apr 3, 2024

Based on what we talked regarding #50818

Handling strict mode is a bit annoying — if you have it set, it'd throw a bit of a cryptic error message since it'd fail with a different error. Another thing is that the array_key_exists solution doesn't deal with custom attributes.
Maybe it's an opportunity to introduce a strict-safe hasAttribute method within Eloquent?

@taylorotwell
Copy link
Member

I need to think about this one a bit. There could still be a virtual accessor for maxAttempts.

@taylorotwell taylorotwell marked this pull request as draft April 5, 2024 14:16
@mateusjatenee mateusjatenee force-pushed the feature/throw-if-limiter-does-not-exist branch from d31af36 to 4d763f4 Compare April 5, 2024 15:33
@mateusjatenee mateusjatenee marked this pull request as ready for review April 5, 2024 15:34
@mateusjatenee
Copy link
Contributor Author

@taylorotwell thanks for merging that in. this one should be good to go.

@mateusjatenee
Copy link
Contributor Author

Not entirely sure why AboutCommandTest is failing on 8.2. Seems unrelated as it's happening to other PRS too, but I'm seeing if I can figure out what's going on

@driesvints
Copy link
Member

Heya! We were having some failures in the testsuite at the base branch. Could you rebase this PR, make sure tests pass and mark for review again? Thanks!

@driesvints driesvints marked this pull request as draft April 8, 2024 10:06
@mateusjatenee mateusjatenee force-pushed the feature/throw-if-limiter-does-not-exist branch from ab0772c to 9e02b18 Compare April 8, 2024 16:38
@mateusjatenee
Copy link
Contributor Author

@driesvints yessir!

@taylorotwell taylorotwell merged commit 62477b3 into laravel:11.x Apr 9, 2024
27 of 28 checks passed
@saade
Copy link

saade commented Apr 15, 2024

@driesvints @taylorotwell The deprecation of the api rate limiter is not documented anywhere? Or am i just being dumb here?

Getting: Rate limiter [api] is not defined. on a Laravel 11 app using ->throttleApi() + api middleware. It was working before on a v11.1.0 installation. Just updated to v11.3.1.

@mateusjatenee
Copy link
Contributor Author

@saade is this an app you upgraded to 11? Do you have the api rate limiter defined in any service providers?
Prior to L11, it shipped by default on RouteServiceProvider

@saade
Copy link

saade commented Apr 15, 2024

@saade is this an app you upgraded to 11? Do you have the api rate limiter defined in any service providers? Prior to L11, it shipped by default on RouteServiceProvider

@mateusjatenee It is upgraded from L10 (using Shift), but it was running L11 perfectly in the last few weeks on a v11.1.0 installation. I think this is a BC for non-new apps?

@mateusjatenee
Copy link
Contributor Author

mateusjatenee commented Apr 15, 2024

@saade see this PR: #50818
The problem is it was previously just silently introducing undesired behavior. If a named limiter did not exist, it'd just default to 0 -- now it throws an exception.

@mateusjatenee mateusjatenee deleted the feature/throw-if-limiter-does-not-exist branch April 15, 2024 18:20
@im-aeo
Copy link

im-aeo commented Jul 8, 2024

RouteServiceProvider

Hey, Did you call your RouteServiceProvider in the bootstrap/providers.php?

bootstrap/providers.php

return [
    App\Providers\AppServiceProvider::class,
    App\Providers\RouteServiceProvider::class, // <--
];


RouteServiceProvider.php
<?php

namespace App\Providers;
use Illuminate\Cache\RateLimiting\Limit;
use Illuminate\Foundation\Support\Providers\RouteServiceProvider as ServiceProvider;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\RateLimiter;
use Illuminate\Support\Facades\Route;

use function PHPSTORM_META\elementType;

class RouteServiceProvider extends ServiceProvider
{
    /**
     * The path to your application's "home" route.
     *
     * Typically, users are redirected here after authentication.
     *
     * @var string
     */
    public const HOME = '/my/dashboard';

    /**
     * Define your route model bindings, pattern filters, and other route configuration.
     */
    public function boot(): void
    {
        $this->configureRateLimiter();

        $this->routes(function () {
            Route::middleware('api')
                ->prefix('api')
                ->as('api.')
                ->group(base_path('routes/api.php'));

            Route::middleware('web')
                ->group(base_path('routes/careers.php'));


            Route::middleware('web')
                ->group(base_path('routes/web.php'));
        });
    }

    protected function configureRateLimiter()
    {
        RateLimiter::for('api', function (Request $request) {
            return Limit::perMinute(60)->by($request->user()?->id ?: $request->ip());
        });
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants