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

Allow precognition dispatchers to be bound by name for overriding #54514

Draft
wants to merge 1 commit into
base: 11.x
Choose a base branch
from

Conversation

Spice-King
Copy link

Hi, I'm working on a PR for Laravel Actions with the goal of enabling Precognition for Actions, as I'd like to start using Precognition within Actions. While I've made this work with an extension of the HandlePrecognitiveRequests middleware for backwards compatibility that people would have to remember to use over the built in one, this is a cleaner way of doing and enables It Just Works™ compatibility.

This change is what allows Spice-King/laravel-actions@4c4720a to make it seamless. I only really need ControllerDispatcher done for what I've done, but there was no compelling reason to not make them match and CallableDispatcher might be needed solve how WithAttributes functions without FormRequests.

@taylorotwell
Copy link
Member

I don't know what this fixes for you? You can still override them even without this change?

@taylorotwell taylorotwell marked this pull request as draft February 10, 2025 11:21
@Spice-King
Copy link
Author

Hi Taylor, thanks for your time on this and the Laravel framework as a whole.

It changes how the Dispatcher instance is instantiated, rather than a callback closure where it returns a fixed result of an instance and ends the resolution there, it shifts the creation back into the Container's system of resolution where I can say a more specific Dispatcher is valid. I could run something like the following code in a package somewhere appropriate (a Service or mid resolution of a route's action) and then not have to question if the developer tagged their route with 'precognitive'/HandlePrecognitiveRequests::class by trying to inspect an incomplete stack of middlewares from multiple places.

app()->bind(PrecognitionControllerDispatcher::class, MyPackagePrecognitionControllerDispatcher::class);

While I can (and do, more for backwards compatibility and testing on older Laravel versions) have a custom version of the middleware in the PR for Laravel Actions which would solve this, it felt like bad DX in needing to remember to use the specific correct version when needed, something that would be worse if another package also need to do the same thing. I wanted to improve the DX to not require it and be more eloquent and inline with how Laravel's magic works.

Is there some considerations, like performance, I've overlooked that would be relevant here in making the Container class evaluate bindings recursively? It seemed like a well written and performant enough chunk of code to me, or at least well enough to not be hot spot needing to be used sparingly.

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.

2 participants