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

Package breaks applications using the HasMiddleware interface #662

Closed
jason-nabooki opened this issue Mar 17, 2023 · 6 comments
Closed

Package breaks applications using the HasMiddleware interface #662

jason-nabooki opened this issue Mar 17, 2023 · 6 comments

Comments

@jason-nabooki
Copy link

Environment

Laravel v9
Sentry Laravel v3.3

Steps to Reproduce

  1. Install sentry-laravel package
  2. Navigate to a route where the controller is using the HasMiddleware interface and has at least one middleware defined in the controller middleware() method.

Expected Result

The route should load.

Actual Result

Exception is thrown.

Call to undefined method Sentry\Laravel\Tracing\Routing\TracingControllerDispatcherTracing::methodExcludedByOptions()

Links

laravel/framework#44516
https://www.amitmerchant.com/resolving-middlewares-statically-from-controllers-in-laravel-9x/

@jason-nabooki
Copy link
Author

Is there a way to disable tracing to prevent the exception from being thrown? Setting traces_sample_rate to null or 0.0 does not help.

@stayallive
Copy link
Collaborator

To answer your question, you can disable autodiscovery for sentry/sentry-laravel and manually add Sentry\Laravel\ServiceProvider::class to the config/app.php providers array to do enable error reporting but completely disable the tracing code.

I am investigating how to fix this... stay tuned!

@stayallive
Copy link
Collaborator

So I've been diving in the Laravel source for this, HasMiddleware is not documented and the implementation is... interesting to say the least 😄

For reference, this is the interface: https://github.com/laravel/framework/blob/136ed754f2d80e5c7cd407a4262eb2eac478e7e8/src/Illuminate/Routing/Controllers/HasMiddleware.php and you can return middleware extending this class from the implemented method: https://github.com/laravel/framework/blob/136ed754f2d80e5c7cd407a4262eb2eac478e7e8/src/Illuminate/Routing/Controllers/Middleware.php.

The "problem" is that in the Route class Laravel calls a static method on the controller dispatcher class we are decorating: https://github.com/laravel/framework/blob/136ed754f2d80e5c7cd407a4262eb2eac478e7e8/src/Illuminate/Routing/Route.php#L1107-L1121. However this methodExcludedByOptions is not part of the ControllerDispatcher contract which is why we have not decorated that static method.

It seems like this is an oversight on the Laravel implementation where this method should not live on the dispatcher class, we probably implement a fix for this to proxy through the static method call, and file a PR to the Laravel core to refactor this because I don't think we are doing anything wrong.

@stayallive
Copy link
Collaborator

I think this is a Laravel bug, so I've submitted a PR: laravel/framework#46498.

I also have a fix ready to work around this if needed but I feel like this is more of a hack than a good solution so I'd rather see the fix implemented in the Laravel framework instead if possible.

@stayallive
Copy link
Collaborator

stayallive commented Mar 18, 2023

The PR to the Laravel framework has been merged, I'll keep this open for reference a bit until it's released but I would assume this will get released with the next Laravel patch release somewhere next week and then it should be all good.

It's fixed with Laravel 10.4.0 🎉

Thanks for reporting!

@jason-nabooki
Copy link
Author

Thank you so much for investigating this in detail and submitting the PR into the Laravel Framework. Much appreciated @stayallive

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

No branches or pull requests

3 participants