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

[RFC] Add HTTP Responder pattern #29574

Closed
ro0NL opened this issue Dec 11, 2018 · 19 comments
Closed

[RFC] Add HTTP Responder pattern #29574

ro0NL opened this issue Dec 11, 2018 · 19 comments

Comments

@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Dec 11, 2018

I've recently polished my HomeController to, what i think, its most ultimate form:

<?php
namespace App\Controller;

use App\Http\Responder;
use App\Http\RespondTemplate;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Annotation\Route;

/**
 * @Route("/", name="home")
 */
final class HomeController
{
    public function __invoke(Request $request, Responder $responder): Response
    {
        return $responder->respond(new RespondTemplate('home.html.twig'));
    }
}

I've used this technique more or less some time now, and as such implemented it in my demo app in a more standard way:

https://github.com/msgphp/symfony-demo-app/tree/master/src/Http

Now im curious if it could be core candidate concept. I think it's very flexible.

ref #21732 (comment) #21765 (comment)

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 11, 2018

To clarify its capability, by default any Respond type can be like:

(new RespondRouteRedirect('route_name', ['param' => 'x']))
  // default API
  ->withStatus(302)
  ->withHeaders(['X-Bar' => 'boo'])
  ->withFlashes(['success' => 'yay', 'error' => ['oh', 'noes']]);
@theofidry

This comment has been minimized.

Copy link
Contributor

@theofidry theofidry commented Dec 11, 2018

@ro0NL what is the real benefit of it (unless you do return different types, like JSON & XML from the same action)?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 12, 2018

@theofidry on high-level not so much. As the kernel is already our true HTTP responder, sort of speak. The controller is just an implementation detail of it.

However, on low-level, it's creates a way to unify controllers, and have them consise and decoupled. So this is basically an alternative (better IMHO) solution to AbstractController and its util responder methods.

If''s flexible, and can be used in e.g. an event listener as well.

So practically the benefit is above snippet from a standard point of view. Not juggling with url generators, twig envs, etc. to provide the response (which is the controller's true job). Ie. we want a redirect-response, not URL generation per se.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 12, 2018

Currently we add more&more value to AbstractController: #28875

But we dont provide a low level API to do things standalone. I dont like it.

Above PR could be done like $respond->withLinks(...) what works for everyone. At this point AbstractController would just leverage the responder.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

@ostrolucky ostrolucky commented Dec 12, 2018

I also don't see benefit of this

class HomeController
{
    public function __invoke(Request $request, Responder $responder): Response
    {
        return $responder->respond(new RespondTemplate('home.html.twig'));
    }
}

over current

class HomeController
{
    public function __invoke(Request $request, Environment $twig): Response
    {
        return new Response($twig->render('home.html.twig'));
    }
}
@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 12, 2018

I know. :) But it becomes boring to deal with it over&over again using the same services.

Im not into 'render this template' or 'generate this URL'. I just want to respond, quickly :)

Using pattern standardizes the approach, and avoids per-case solutions by inlining logic per controller. Or using AbstractController for that matter, which is not the design i seek.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

@ostrolucky ostrolucky commented Dec 12, 2018

Having to inject Responder service over and over again is less DRY, where in current aproach sometimes you might need Twig service, sometimes not.

This is precisely what are abstract controllers for. If you don't like them, there is also https://github.com/fervo-and-friends/controller-traits or things similar to https://github.com/kbond/ControllerUtil

Im not into 'render this template' or 'generate this URL'. I just want to respond, quickly :)

You are, you have to specify which Responder object to use, in this case RespondTemplate

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 12, 2018

This is precisely what are abstract controllers for

in the most pure way i agree with you. This is just a util service to standardize boring HTTP responding stuff. If it needs twig or routing is an implementation detail of the concept.

But we might favor composition over inheritance no?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 12, 2018

you have to specify which Responder object to use

the difference is subtle, but there's only one "Responder", You provide it a "Respond" definition (no services involved here)

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Dec 13, 2018

I also don't see benefit of this
[...]
over current
[...]

It lets you create a set of standard wrappers, but also pushes out the direct dependency on your templating engine. Even better though, it's a viable replacement of @Template and it replaces the need of a RedirectResponse object to be created in your controller.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

@ostrolucky ostrolucky commented Dec 13, 2018

new RespondTemplate() signifies direct dependency on templating engine. And I don't see how is replacing RedirectResponse with RespondRedirect different, nor where is the need to replace @Template

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Dec 13, 2018

And I don't see how is replacing RedirectResponse with RespondRedirect different,

RedirectResponse requires a non-empty string as constructor argument, which makes it impossible to have any form of lazy route generation for example.

nor where is the need to replace @Template

This is a dependency on the SensioFrameworkExtraBundle, it's not a core feature. On top of that, it's heavily restrictive. If you want to either render or redirect, you have an @Template on your controller, while returning an array in scenario 1, but a RedirectResponse in scenario 2. This annotation also lacks support for things such as headers, cookies, multiple templates and probably some others that I forgot.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 24, 2018

The current design is simple and generic: you attach a callable to a route and it's been called when that route is requested.
What you do then is up to you and Symfony shouldn't provide more opinionated ways to map a route to some code IMHO.

AbstractController is just a collection of helpers. These helpers are shipped all together, providing one main benefit: easy discovery. If you don't want to use it, all is fine, it means you need to know what the framework provides you and cherry-pick the features you want by yourself.

Nothing to do here in core to me.

@ro0NL ro0NL closed this Dec 24, 2018
@GromNaN

This comment has been minimized.

Copy link
Contributor

@GromNaN GromNaN commented Dec 25, 2018

Convert the responder service into a listener for the kernel.view event and your controller will be even simpler.

@HeahDude

This comment has been minimized.

Copy link
Member

@HeahDude HeahDude commented Dec 25, 2018

Did you mean kernel.view?

@GromNaN

This comment has been minimized.

Copy link
Contributor

@GromNaN GromNaN commented Dec 27, 2018

Exact @HeahDude, I updated my comment.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Dec 27, 2018

i was more seeking a unified way to produce responses. Though i can see controllers returning special types (not a Response), to be handled upon kernel.view, being a valid approach; for regular HTTP controllers i aim to preserve controller(Request $r): Response

The responder pattern also helps with e.g. $event->setResponse(..). But i closed per @nicolas-grekas comment this an implementation detail of core-features.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

@ro0NL ro0NL commented Mar 5, 2019

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Mar 5, 2019

Thanks @ro0NL !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.