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

Change the default invalid form handling to follow redirects internally #36

Merged
merged 14 commits into from
Nov 11, 2021

Conversation

tonysm
Copy link
Collaborator

@tonysm tonysm commented Oct 8, 2021

Changed

  • Instead of redirecting to the form route when a ValidationException is thrown, we're following the redirects internally returning the form response body with a 422 status code, which is better handled by Turbo Native.

Issue #29

This borrows from the Inertia dialog implementation.

@tonysm tonysm changed the title Invalid forms with 422 Respond invalid form responses with a 422 status code instead of redirecting to the form route Oct 8, 2021
@tonysm tonysm changed the title Respond invalid form responses with a 422 status code instead of redirecting to the form route Change the default invalid form handling to follow redirects internally Oct 8, 2021
@tonysm
Copy link
Collaborator Author

tonysm commented Oct 12, 2021

I'm having an issue with the database session driver. For some reason, when I receive a 422 response while using the database session driver, the next request will get a CSRF Token Mismatch exception (which returns a 419). This is not an issue with other session drivers, such as redis or file.

Still investigating, but possibly not related to this code (because of the fact that it works with the other drivers)

@tobyzerner
Copy link
Contributor

tobyzerner commented Nov 7, 2021

@tonysm Rather than trying to guess the form redirect URL from the route name (which simply doesn't work if you're not using those conventions), is it possible to use url()->previous() like the default handling in Illuminate\Foundation\Exceptions\Handler@invalid? Seems to work okay in my initial testing.

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 7, 2021

@tobyzerner nope, we can't do that. Forms can be added to any page using Turbo Frames and when they are submitted we get the referrer header sat as that page, which may not have the form. I cover that in the first video of my introduction here at 6:05

@tobyzerner
Copy link
Contributor

tobyzerner commented Nov 7, 2021

Ah, of course, I was only testing with full page visits.

I'm having a problem with this PR as it stands. The routes in part of my application are inside a group with a name prefix, but not all of them have names. So in effect, I have routes named like:

  • GET /login (name = login): prefix.login
  • POST /login (no name set): prefix.
  • GET /some-other-route (no name set): prefix.

When my POST login route throws a validation error, the redirect guesser uses prefix. as the route name – since it's not empty. The middleware redirects me to the last route with that name, ie. some-other-route.

I wonder if this could even be classified as a Laravel bug? Ideally routes with no name set shouldn't have their name set to their group prefix...

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 8, 2021

It's been a while since I've touched on this PR, but the guesser here should only work if the guessed route exists. It's meant for resource routes (*.store guesses *.create and so on). For anything it cannot "guess", it should not modify the response, so you would get the regular "redirect back" behavior. I think. This PR is currently on a break. But I should be working on this again somewhere soon.

@tobyzerner
Copy link
Contributor

@tonysm Sorry, let me try and clarify my issue. In Laravel, routes end up being given a name if they are inside a group with a name prefix, even if the route itself hasn't explicitly had a name set. So for this route:

Route::name('prefix.')->group(function () {
    Route::get('some-route', MyController::class);
});

Even though I didn't give that route a name, it's still inherited the group prefix, so its name is prefix.. This could possibly be considered to be a Laravel bug...

In any case, this PR breaks given the above behavior. If I register a few routes within a group with a name prefix:

Route::name('app.')->group(function () {
    Route::get('login', [LoginController::class, 'showLoginForm'])->name('login'); // name = "app.login"
    Route::post('login', [LoginController::class, 'login']); // name = "app."
    Route::get('other-route', MyController::class); // name = "app."
});

Now if I POST to /login, and my controller throws a validation error, TurboMiddleware will call guessFormRedirectUrl:

        $route = $request->route(); // the POST /login route
        $name = optional($route)->getName(); // "app."

        if (! $route || ! $name) { // both are present, so we continue
            return null;
        }

        $formRouteName = $this->redirectGuesser->guess($name); // redirect guesser doesn't change the name, so it's still "app."

        if (! Route::has($formRouteName)) { // there is a route called "app.", so continue
            return null;
        }

        return route($formRouteName, $route->parameters() + request()->query()); // since other-route was registered last, the resulting URL is /other-route.

A workaround is to give all of my routes names. Another workaround is to manually catch ValidationException in my controller and set its redirectTo value so TurboMiddleware won't try and guess.

Just flagging that this is unexpected behavior introduced by this PR. Maybe better fixed upstream in Laravel though.

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 8, 2021

Oh, interesting. Thanks for pointing it out. That's definitely not intended behavior. I'll make sure to address this before this PR gets merged.

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 9, 2021

Confirmed that the desired behavior is to respond to invalid form submissions with a 422 status code instead of redirects (here)

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 10, 2021

What is interesting about this error is that if I catch the validation exception and manually return a 422 from the route handler, it works. There is no issue with sessions like that.

Route::post('posts', function () {
    try {
        $post = Post::create(request()->validate([
            'title' => ['required'],
            'content' => ['required'],
        ]));
    } catch (ValidationException $e) {
        view()->share('errors', $errors = new ViewErrorBag);
        $errors->put('default', $e->validator->getMessageBag());

        return response(
            view('posts.create')->render(),
            422
        );
    }

    return redirect()->route('posts.show', $post);
})->name('posts.store');

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 10, 2021

Maybe if I add a validation exception handler?

@tonysm
Copy link
Collaborator Author

tonysm commented Nov 10, 2021

Ok, so re-sending a request through the HTTP Kernel is what triggers the error. If instead I manually create a 422 response it works fine.

This should handle any encrypted request as well.

Co-authored-by: Taylor Otwell <[email protected]>
@tonysm tonysm force-pushed the invalid-forms-with-422 branch from a7bb925 to 23127fc Compare November 11, 2021 02:04
@tonysm tonysm merged commit 6ff03eb into main Nov 11, 2021
@tonysm tonysm deleted the invalid-forms-with-422 branch November 11, 2021 02:37
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