Treblle / treblle-laravel

The official Treblle SDK for Laravel. Seamlessly integrate Treblle to manage communication with your dashboard, send errors, and secure sensitive data.
https://www.treblle.com/
MIT License
129 stars 21 forks source link

Fix call to pushMiddleware #65

Closed ejunker closed 1 year ago

ejunker commented 1 year ago

pushMiddleware only accepts one argument

Without this change I get error:

Illuminate\Contracts\Container\BindingResolutionException: Target class [treblle] does not exist.
ejunker commented 1 year ago

Though I am not sure what you are attempting to do. The documentation says to add the middleware ->middleware('treblle') to the route group you want. When you use pushMiddleware I think it adds the middleware to ALL routes which I do not think is your intent.

In one of your previous commits you had:

$this->app['router']->aliasMiddleware('treblle', TreblleMiddleware::class);

I think that is what you want.

JustSteveKing commented 1 year ago

Unfortunately from my investigations, this is no longer supported so the middleware has to be registered by the end user ow

ejunker commented 1 year ago

@JustSteveKing I'm not sure if you understand what I was trying to say. Currently in the TrebelleServiceProvider.php you have

https://github.com/Treblle/treblle-laravel/blob/master/src/TreblleServiceProvider.php#L58

$this->app[Kernel::class]->pushMiddleware('treblle', TreblleMiddleware::class);

You are passing 2 arguments to pushMiddleware() but it only accepts 1 argument. That is the first issue. The second issue is that I am not sure that you are aware that pushMiddleware() will add the middleware to the stack but you do not want to do that because the user needs to add the middleware. I think your intention was to register the middleware alias which is done by using the aliasMiddleware() method which you had in a previous commit and then you removed it.

JustSteveKing commented 1 year ago

@ejunker ahh! This makes a lot more sense!

JustSteveKing commented 1 year ago

@ejunker I will schedule this for the next release in a couple of days

ziming commented 1 year ago

yes this is the same error im getting in staging, glad to see it merged