hotwired-laravel / turbo-laravel

This package gives you a set of conventions to make the most out of Hotwire in Laravel.
https://turbo-laravel.com
MIT License
803 stars 50 forks source link

[RFC] Change default validation exception handling #29

Closed tonysm closed 3 years ago

tonysm commented 3 years ago

Folks, I've been tinkering with Turbo Native for a bit, and looks like our default behavior of redirecting *.store to *.create and *.update to *.edit won't do the trick. It works well enough on the web, but on Turbo Native, the redirect triggers the navigation piece, which causes the app to behave weirdly, closing the native modal to reopen it again with the new form.

https://user-images.githubusercontent.com/1178621/126818162-cb6ceddc-f13d-47b1-9ab9-a7df02315b35.mp4

I've seen the Rails and Symfony folks simply re-rendering the form with a 422 status code, and Turbo does support it, and that seems to be the best way to avoid this blinking behavior.

The Rails folks simply render the form action again with a 422 status code:

    def update
      respond_to do |format|
        if @post.update(post_params)
          format.html { redirect_to @post.discussion, notice: "Post update" }
        else
          format.html { render :edit, status: :unprocessable_entity }
        end
      end
    end

(source)

Also, Rails recently changed their default scaffolding to generate the files rendering the forms with a 422 status code.

And the Symfony folks do something similar:

#[Route('/{id}/edit', name: 'conference_edit', methods: ['GET', 'POST'])]
public function edit(Request $request, Conference $conference): Response
{
    $form = $this->createForm(ConferenceType::class, $conference);

    $form->handleRequest($request);

    if ($form->isSubmitted() && $form->isValid()) {
        // do something with the $conference object
        // (e.g. persist it in the database)

        return $this->redirectToRoute('conference_show', [
            'id' => $conference->getId(),
        ]);
    }

    return $this->renderForm('conference/edit.html.twig', [
        'form' => $form,
    ]);
}

(source)

I'm still not sure how we could possibly do that. My initial thought was to start an internal request and send it back to the app following the same convention (*.store triggers a GET request to the *.create route when validation fails) and return the response.

Here's an example from a simple TODO app I've built:

Route::post('todos', function () {
    try {
        $todo = Todo::create(request()->validate([
            'name' => 'required',
        ]));
    } catch (ValidationException $exception) {
        return response()->view('todos.create', [
            'todo' => Todo::make(),
            'errors' => tap(new ViewErrorBag())->put('default', $exception->validator->getMessageBag()),
        ], 422);
    }

    if (request()->wantsTurboStream() && ! Turbo::isTurboNativeVisit()) {
        return response()->turboStreamView('todos.turbo.created_stream', [
            'todo' => $todo,
            'newTodo' => Todo::make(),
        ]);
    }

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

This code does a couple of things:

  1. When validation passes and it's a Turbo Request, but not from a Turbo Native client, it returns the Turbo Streams to update the web app (prepend the todo to the list and reset the form);
  2. When validation passes and it's a Turbo Request from a Turbo Native client, it redirects to the todo itself, this should trigger the client's native navigation and close the modal, updating the fragment to show the todo (full behavior on a video below);
  3. When validation fails, it re-renders the create todo view itself, but it feeds it with the message bag from the validation. This behavior works well for both web (which renders the form inside an inline turbo frame) and Native (which renders the form in a native modal)

Here's how the app behaves in the web version:

https://user-images.githubusercontent.com/1178621/126818624-169544a3-511f-49f3-98a4-61dd78428b30.mp4

And here's how the native version behaves:

https://user-images.githubusercontent.com/1178621/126818240-e1b68d28-8efa-4c9b-b670-8339e4c41417.mp4

At first, I thought keeping the default behavior in Laravel of redirecting on validation errors was the best option, but now that I've played with Turbo Native a couple more, the best way seems to be to render the form with validation errors in the same request with a 422 status code.

tonysm commented 3 years ago

I have the feeling that rendering the form with 422 is more useful. However, that doesn't play well with Laravel's default behavior. If you type-hint a Form Request, validation errors (on failures) will throw exceptions before it even calls the controller action method. I wonder if we could add a "Lazy Form Request" that could allow us to write these things like this:

class TodosController
{
  public function create()
  {
    return view('todos.create', [
      'todo' => Todo::make(),
    ]);
  }

  public function store(LazyCreateTodoRequest $request)
  {
    // The form request will execute neither authorization nor validation automatically. Instead,
    // we have to manually call some kind of method inside the request handler ourselves.
    // This way, we could handle validation errors without dealing with exceptions.

    if (! $request->passes()) {
      return $this->create()->withStatus(422);
    }

    $todo = Todo::create($request->validated());

    if (request()->wantsTurboStream() && ! Turbo::isTurboNativeVisit()) {
      return response()->turboStreamView('todos.turbo.created_stream', [
        'todo' => $todo,
        'newTodo' => Todo::make(),
      ]);
    }

    return redirect()->route('todos.show', $todo);
  }
}

Uhm, wondering how this is different than simply telling not to use the form requests from Laravel and doing the validation in the controllers like:

<?php

namespace App\Http\Controllers;

use App\Models\Todo;
use Illuminate\Contracts\Support\MessageBag;
use Illuminate\Support\ViewErrorBag;

class TodosController extends Controller
{
    public function create()
    {
        return view('todos.create', [
            'todo' => Todo::make(),
        ]);
    }

    public function store()
    {
        $validator = validator(request()->all(), [
            'name' => 'required',
        ]);

        if ($validator->failed()) {
            return $this->renderFormWithErrors($validator->getMessageBag());
        }

        $todo = Todo::create($validator->validated());

        if (request()->wantsTurboStream() && ! request()->wasFromTurboNative()) {
            return $this->renderTurboStreams($todo);
        }

        return redirect()->route('todos.show', $todo);
    }

    private function renderFormWithErrors(MessageBag $errors)
    {
        return response($this->create()->with([
            'errors' => tap(new ViewErrorBag())->put('default', $errors),
        ]), status: 422);
    }

    private function renderTurboStreams(Todo $todo)
    {
        return response()->turboStreamView('todos.turbo.created_stream', [
            'todo' => $todo,
            'newTodo' => Todo::make(),
        ]);
    }
}
vanthao03596 commented 3 years ago

I have the feeling the rendering the form with 422 is more useful. However, that doesn't play well with Laravel's default behavior. If you type-hint a Form Request, validation errors (on failures) will throw exceptions before it even calls the controller action method. I wonder if we could add a "Lazy Form Request" that could allow us to write these things like this:

class TodosController
{
  public function create()
  {
    return view('todos.create', [
      'todo' => Todo::make(),
    ]);
  }

  public function store(LazyCreateTodoRequest $request)
  {
    // The form request will execute neither authorization nor validation automatically. Instead,
    // we have to manually call some kind of method inside the request handler ourselves.
    // This way, we could handle validation errors without dealing with exceptions.

    if (! $request->passes()) {
      return $this->create()->withStatus(422);
    }

    $todo = Todo::create($request->validated());

    if (request()->wantsTurboStream() && ! Turbo::isTurboNativeVisit()) {
      return response()->turboStreamView('todos.turbo.created_stream', [
        'todo' => $todo,
        'newTodo' => Todo::make(),
      ]);
    }

    return redirect()->route('todos.show', $todo);
  }
}

Uhm, wondering how this is different than simply telling not to use the form requests from Laravel and doing the validation in the controllers like:

<?php

namespace App\Http\Controllers;

use App\Models\Todo;
use Illuminate\Contracts\Support\MessageBag;
use Illuminate\Support\ViewErrorBag;

class TodosController extends Controller
{
    public function create()
    {
        return view('todos.create', [
            'todo' => Todo::make(),
        ]);
    }

    public function store()
    {
        $validator = validator(request()->all(), [
            'name' => 'required',
        ]);

        if ($validator->failed()) {
            return $this->renderFormWithErrors($validator->getMessageBag());
        }

        $todo = Todo::create($validator->validated());

        if (request()->wantsTurboStream() && ! request()->wasFromTurboNative()) {
            return $this->renderTurboStreams($todo);
        }

        return redirect()->route('todos.show', $todo);
    }

    private function renderFormWithErrors(MessageBag $errors)
    {
        return response($this->create()->with([
            'errors' => tap(new ViewErrorBag())->put('default', $errors),
        ]), status: 422);
    }

    private function renderTurboStreams(Todo $todo)
    {
        return response()->turboStreamView('todos.turbo.created_stream', [
            'todo' => $todo,
            'newTodo' => Todo::make(),
        ]);
    }
}

When the first time i try Turbo with laravel, i'm stuck with type hint FormRequest