symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
853 stars 314 forks source link

[Turbo] allow adding an "interceptor controller" to the route which gets invoked when the request is a Turbo Stream compatible request #518

Closed dkarlovi closed 2 years ago

dkarlovi commented 2 years ago

When you want to add streams to an existing app, my idea is to

  1. register a new controller which listens to the same route as the original, but expects the correct Accept header
  2. it forwards the request to the original controller, without the Accept header
  3. it then allows rendering a separate response either based on 2. response or something else entirely

See a functional example here, I'm using a tag (with a route property) to link the controller to a route, but it could be an attribute or similar. It would be a nice addition since the mechanics of this are not totally trivial to get right, because of the stripping of the header, subrequests, etc. https://github.com/symfony/ux/issues/518#issuecomment-1297295599

dkarlovi commented 2 years ago

This concept seems to be somewhat harder to understand from comments in Symfony Slack so I'll try to outline it a bit better here:

  1. you have an existing route, say
    add_to_cart:
    path: /cart/add
    method: POST
    controller: Some3rdPartyCartController
  2. this route works like POST /cart/add, product=123
  3. we add a new route
    streams_add_to_cart:
    path: /cart/add
    method: POST
    controller: CustomStreamsSpecificCartController
    conditions: request.accept == 'text/vnd.turbo-stream.html'
  4. this route works like POST /cart/add, product=123, Accept: text/vnd.turbo-stream.html
  5. the CustomStreamsSpecificCartController does something like

    __invoke()
    {
    // invoke the original request, but without the Accept: text/vnd.turbo-stream.html
    
    return $this->render('add_to_cart.stream.html.twig', $anyCustomDataStreamsTemplateRequires);
    }
  6. we should be able to do this automatically as much as possible, ideally without needing to copy/paste routes manually, for example an annotation like
    #[StreamRoute("add_to_cart")]

    would be enough.

Why not do it in the original controller?

The original controller should work exactly as before, it's untouched.

Why not in a request listener?

We don't know which stream controller to point to, we need a place to put the controller name for each route we want to stream, which is exactly what "a route" is.

Why do you need a separate controller for each route?

Each route's Twig template could require separate, context specific information, the controller is used to differentiate this.

/cc @weaverryan I know you've been doing something similar in Casts.

weaverryan commented 2 years ago

Hi!

To help make sure I understand, can you state the specific problem? Is it that in your original /cart/add controller, you want to avoid adding the TurboBundle::STREAM_FORMAT === $request->getPreferredFormat()? Or is it that the /cart/add controller is something that you can't modify (I noticed you used Some3rdPartyCartController in your example)?

cheers!

dkarlovi commented 2 years ago

Correct, I can't modify it. Imagine this is happening from a bundle.

But also, the approach I'm looking at here seems a bit cleaner since you're opting in to Streams per route.

dkarlovi commented 2 years ago

Creating routes seems impossible so I'm replacing _controller from an event listener. I'm not super happy about this, but it's better than nothing, this is my event subscriber:

<?php

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\AcceptHeader;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\UX\Turbo\TurboBundle;

final class TurboStreamControllerEventSubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [KernelEvents::REQUEST => 'onRequest'];
    }

    public function __construct(private array $routes)
    {
    }

    public function onRequest(RequestEvent $event): void
    {
        if ($event->isMainRequest() === false) {
            return;
        }

        $request = $event->getRequest();
        if (TurboBundle::STREAM_FORMAT !== $request->getPreferredFormat()) {
            return;
        }

        /** @var null|string $route */
        $route = $request->attributes->get('_route');
        if ($route === null || \array_key_exists($route, $this->routes) === false) {
            return;
        }

        $request->attributes->set('_non_turbo_streams_request', $this->createOriginalRequest($request));
        $request->attributes->set('_controller', $this->routes[$route]);
    }

    private function createOriginalRequest(Request $request): Request
    {
        $accept = AcceptHeader::fromString($request->headers->get('Accept'))
            ->filter(sprintf('/^(?!%1$s)/', preg_quote(TurboBundle::STREAM_MEDIA_TYPE, '/')))
        ;
        $subRequest = $request->duplicate();
        $subRequest->headers->set('Accept', $accept->__toString());

        return $subRequest;
    }
}

I pass something like

route => controller

map to it, it gets created in a compiler pass from a tag.

The controller looks like this


use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

final class AddToWishlistTurboStreamController extends AbstractController
{
    use TurboStreamControllerTrait;

    public function __invoke(Request $request): Response
    {
        $this->invokeOriginalRequest($request);

        return new Response('Render a custom Turbo Streams response for add to wishlist here');
    }
}

Closing.

dkarlovi commented 2 years ago

Thinking on this further, this still seems like it could be a nice addition, maybe I need to reword the idea.

dkarlovi commented 2 years ago

@weaverryan updated the description, would love to know your feedback if any.

weaverryan commented 2 years ago

I'm not convinced yet, but it's worth iterating on here to think about it. What if you allowed the original controller to execute like normal. Then, on a KernelEvents::RESPONSE, you have a listener. This listener would be passed some sort of array (or more technically, locator) of "handler" objects, which are somehow linked to specific controllers or routes. The listener would find the correct handler, if any, and then call it, passing it the Response. That handler would then return the stream response. The handlers (or maybe "formatters?") would replace your controller above and look something like:

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

#[StreamFormatter(route: 'the_original_route')]
class AddToWishlistStreamFormatter
{
    public function __invoke(Response $response): Response
    {
        return new Response('Render a custom Turbo Streams response for add to wishlist here');
    }
}

Maybe for convenience we pass your method the Twig Environment, not sure.

Again, I'm not sold on this approach / need, but I'm also not totally "unsold" on it. Let me know what you think of this approach for your use-case/

dkarlovi commented 2 years ago

Yes, that could also work. My approach focused on "intercepting the request", but what it ends up doing is intercepts the response. Nice idea! :+1:

dkarlovi commented 2 years ago

So the response listened would only be registered if there's any "stream formatters". It would only trigger if it's a stream request. If the route matches, the formatter(s) are triggered. I could see that being useful.

dkarlovi commented 2 years ago

The locator could be the existing DI service locator, with each service an iterable of formatters, keyed by route name. It could leverage all the existing Container stuff to make this pretty easy to implement.

dkarlovi commented 2 years ago

OK, I did just that: response subscriber which gets injected with a service locator keyed by route name.

dkarlovi commented 1 year ago

@weaverryan I think this approach is very convenient to work with and would probably be a nice addition to the bundle.

Here's an example which uses it: https://demo-feat-3d-configurator.cloud.sigwin.hr/de/shop/configure/schrank-konfigurator~3899

It's basically decorating the search route: https://demo-feat-3d-configurator.cloud.sigwin.hr/de/shop/configure/schrank-konfigurator~3899/search

for Turbo Streams and it was very nice to be able to add this declaratively like described, the Twig people were happy with what they can do without help. :smile: