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

Enable Precogitition support for ActionRequests #307

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Spice-King
Copy link

Hey, I've spent the last week poking and prodding at supporting Precognition in Laravel-Actions in a clean enough way. I believe I've done that as I've not found a cleaner way. I've written as many tests to double check my work and validate I've not broken anything else in my wake. I did not add support to WithAttributes as I have no intentions of using it and it's path for supporting Precognition differs since it does not use FormRequests.

I have a PR open in laravel/framework (laravel/framework#54514) for 4c4720a to make it automagical using the normal middleware, but I have added a middleware that makes the needed tweaks for Actions in Precognition for both backwards compatibility in older versions of Laravel and in the event they opt to deny/close the PR.

@lorisleiva
Copy link
Owner

Hey 👋 Thanks for submitting a PR. I'm afraid I've been out of the Laravel game for a while now and I'm not too sure what this PR does. Could you explain the PR in more details and convince me this isn't going to break the current consumers of this library?

@Spice-King
Copy link
Author

Hi. If people do not enable Precognition on their actions and do not attempt to submit Precognitive requests to their actions, nothing should change for them. If they tried to enable Precognition before, they would have met with an error (as noted and worked around in this discussion), but still would not have Precognition return validation failures. The change merely enables Precognition to actually validate an Action's ActionRequest as it would a FormRequest, rather than always seeming to succeed, as validation was being done inside the ControllerDecorator and thus the request aborted by Precognition as a success before being run.

All pre-existing tests still pass, even in Laravel 10.0 (though, I've have loved to cut out all of 10.x for the better Precognitive tooling for tests that have been added since), plus the new set I've added to validate my work. I think the only weird bit I have added is the app()->extend(ActionRequest::class, [...]) one, and that's because extend fires it's hooks before afterResolving. FormRequestServiceProvider adds a callback there to configure the fact it validates after resolving, but would need to know the action before then. and extend is a guarantied to be before code wise, so no race conditions. If you need some clarification on some other bit of code, open a review on it if you want to pick my brain.

I do know that documentation would need a slight touch up no matter if Laravel accepts my PR to the framework or not. Precognitive requests would need the HandlePrecognitiveActionRequests middleware on version of the framework without that PR merged. I'm hoping that they accept it since it's a very minor tweak, but I'll have to see when ever they respond to it.

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