Improve Middleware except

[asbiin]

This is inspired by https://github.com/laravel/framework/blob/master/src/Illuminate/Foundation/Http/Middleware/Concerns/ExcludesPaths.php trait (from Laravel 11.x) used in TrustProxies middleware.

This allows you to extends TrackPageview middleware like:

namespace App\Http\Middleware;

use Pirsch\Http\Middleware\TrackPageview as Middleware;

class TrackPageview extends Middleware
{
    /**
     * The URIs that should be excluded from tracking.
     *
     * @var array<int,string>
     */
    protected array $except = [
        'my/url',
    ];
}

[zepfietje]

Thanks, @asbiin!

I've just closed #3 as this PR essentially is a replacement of that too. Could you add horizon to the default ignored paths?

[asbiin]

@zepfietje thank you! and done.

[adampatterson]

Mine offered a way to block other headers by using config/pirsch.php. This implementation is hard coding the headers in the Package.

https://github.com/pirsch-analytics/laravel-pirsch/pull/3/files

[zepfietje]

What isn't achievable with this implementation that was possible with yours, @adampatterson?

[adampatterson]

@zepfietje It's probably fine, I can't think of any other existing Laravel packages that I'd want to exclude but I went with the config approach just in case there were other routes that should be ignored.

I don't know why you would add Pirsch to web hook group but you could for instance exclude the Cashier route by modifying the config.

Maybe to track events

[zepfietje]

This PR still allows for complete configuration of excluded routes, right @asbiin?
Or am I missing something that this PR doesn't enable that yours did, @adampatterson?

[zepfietje]

Could you please add some docs around this new feature to the README, @asbiin? 🙂

[adampatterson]

That's how I'm reading the code, the excluded headers and routes are in protected methods.

I think it would be possible to add a param to handle that could be used to exclude something in situation.

By the way, I like the idea of excluding headers, and the param might be a nice touch.

https://laravel.com/docs/11.x/middleware#middleware-parameters

[zepfietje]

@adampatterson it doesn't matter that they're protected, since you're extending the base middleware class and overriding those properties.

[asbiin]

I think it would be possible to add a param to handle that could be used to exclude something in situation.

I like the idea.
Can we try that in a separate PR?

[zepfietje]

Sure, go ahead and create another PR so we can compare before merging one of them, @asbiin.

[asbiin]

Sure, go ahead and create another PR so we can compare before merging one of them, @asbiin.

Well, I'm done with this PR here. I expected @adampatterson to create another base on his proposal?

[adampatterson]

I can do that, I think after this one is merged in.

[zepfietje]

Ideally I'd review the two approaches and only merge the one that we think is best.

[asbiin]

@zepfietje @adampatterson I had a more close lock at the Middleware Parameters option.
I've added a parameter to allow exclusion of urls. However we can't have both url and headers exception, as it is not possible to define multiple array parameters in the handle method.

Please review the README changes, you may want to rephrase some of them...

I also fixed the tests which wasn't accurate !

I hope this will help you ...

[zepfietje]

Shouldn't it be possible when using array $except instead of string ...$except, @asbiin?

[asbiin]

Shouldn't it be possible when using array $except instead of string ...$except, @asbiin?

You can see in the documentation:

Multiple parameters may be delimited by commas:
Route::put('/post/{id}', function (string $id) {
// ...
})->middleware('role:editor,publisher');

So the parameters are strings separated by commas.
You could have 1 string, then an array of strings: handle(Request $request, Closure $next, string $role, array $args).
However, you can't have multiples arrays, because it wouldn't be possible de define such middleware with the : syntax, separated by commas.

[zepfietje]

Alright, I think I don't like this approach to exclude URLs then.
Also, what's the benefit compared to excluding the URLs on the actual route definition?

[adampatterson]

The middleware name space may not be correctly documented. This is also a Laravel 11 specific configuration.

We may want to include an example for previous versions.

[adampatterson]

Unless the middleware is published your name space should probably remain \Pirsch\Http\Middleware\TrackPageview.

And one other small note is that this registration I think will only work with Laravel 11.

Laravel 10 and below would be in \App\Http\Kernel

You may need to bump the composer.json minimum framework version requirement.

[asbiin]

Unless the middleware is published your name space should probably remain

No, this is your local file created just below, see line 116.

[adampatterson]

@asbiin I haven't tried this, but Chat GPT showed basically what you have except they didn't typecast with string.

I wonder if adding string ...$roles is type casting the params. Can you see if it remains an array without.

public function handle($request, Closure $next, ...$roles)
    {
        // $roles is an array of the provided parameters
        if (!in_array($request->user()->role, $roles)) {
            // Redirect or return a response if the role doesn't match
            return redirect('home');
        }

        return $next($request);
    }