Allow HtmlResponse to use a flexible emitter

Moving to the right component, and assigning to @Crell.

This is essentially what Fabian and I discussed at DrupalCon BCN. However, we'd need an EmittableResponseInterface, which is what the trait is for. Don't confuse the emitter with the emitted.

Wim, are you assigning to me for validation or asking me to write it? :-)

Validation :)

Per #2469431-114: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts and #2469431-115: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:

Is #2577631: Allow HtmlResponse to use a flexible emitter a nice-to-have or a must-have?

+

#2577631: Allow HtmlResponse to use a flexible emitter is a must-have as neither cloning the response nor creating our own Response is a clean solution in the end - as discussed with Crell.

Also it sends very much the right signal - to be able to flexible stream responses, which is kinda a revived hype at the moment.

Also it is just PSR-7 back-porting overall and could well live in the future in Symfony upstream and be allowed for all Responses.

Well, here's some code anyway. :-) It doesn't wire up any emitters yet, as I'm not certain where Fabian intended to do so, but this provides the basic code we discussed.

That said, while working on it, it occurred to me that perhaps we could just leverage Diactoros' emitters directly. We can convert a Symfony Response to PSR-7, can we not? And there's already a model in place for PSR-7 response emitters, in Diactoros. It's already in vendor. Could we just use that directly?

I still have to learn how to resist a naming nitpick, it seems, but :

+++ b/core/lib/Drupal/Component/HttpFoundation/EmittableResponseInterface.php
@@ -0,0 +1,17 @@
+interface EmittableResponseInterface {

It feels confusing to formulate this in such a way that "some Responses are emittable, and some are not" - and then what, we keep those and never send them to the client ? ;-). The distinction between "send" and "emit" is fairly subtle, especially since we are still in an HttpFoundation stack , and *not* in PSR-7/Diactoros land (AFAICT, Emitters seem to be a Diactoros construct rather than a PSR-7 one, right ?)

Sticking with layman's terms, this is not really about being "a response that can be emitted", but rather "a response to which you can assign a specific emitter" ?

--> Proposal : ResponseWithEmitterInterface, EmitterAwareResponse ?
That remains understandable even for people that don't have the background of the "we'd like to do some Diactoros stuff but have to stay in HttpFoundation at the moment so we port a concept over" discussion that happened in this issue ?

FabianX: I'm not sure I follow on DefaultResponseEmitter. What's wrong with send()? The net result is the same: The response object gets sent to the client. In practice I don't know that we'll use the Default emitter, but it's there to have something to implement that interface. :-) The interesting ones would be the ones you have to write yet. (nudge nudge... :-) )

yched: Yeah, the terminology is a bit mixed as we are, in essence, working around a design limitation of HttpFoundation and using Diactoros as a model. (Emitters are Diactoros specific, but the existence of something like them is implied by PSR-7 being data-only and having no send() of its own. So any PSR-7 server implementation would need something along those lines one way or another.)

I would be OK with EmitterAwareResponseInterface instead of EmittableResponseInterface, although I generally prefer adjective interface names to awareness. (There's something deeply philosophical in that sentence, I'm sure...)

@Crell :

- Emitters / Diactoros / PSR-7 : thanks for the clarification, totally makes sense.

- "I generally prefer adjective interface names to awareness" - Sure, in the cases where a good adjective exists, ++ on the adjective. "Being able to delegate sending to an external independant emitter implementation" seems hard though (in english at least - I'm sure it's easy peasy in german :-p)
+ lol at the philosophical implications. I guess in Europe we like "aware" because of Jean-Claude Van Damme.

- DefaultResponseEmitter : I think @Fabianx means that the current results in an infinte "you do it, no you do it, no you..." loop between Response::send() -> DefaultResponseEmitter::emit() -> Response::send() -> ... ?

lol at the philosophical implications. I guess in Europe we like "aware" because of Jean-Claude Van Damme.

… You just pulled in Jean-Claude Van Damme into a sentence with "philosophical"? HERESY! He's a Belgian actor doing silly American movies, and he's being brought up by a French actor, home to Descarte and Sartre. I really don't know what the hell is happening here :D

I feel like a Hercule Poirot reference would be appropriate...

I see what you mean on the infinite loop. (Maybe that's how Jean-Claude Van Damme got in here?) I was thinking that the Emitter could work with any Response, and for most responses (ie, not the ones we're using it for) send() would still work. So we probably need a guard to not send() for an EmitterAwareResponse, or something.

Whatever, at this point I turn it over to FabianX to figure out how to leverage. :-)

#13: <offtopic>Hercule Poirot is most welcome at https://twitter.com/wimleers/status/653510681820557313</offtopic>

Sorry for the distractions on this issue so far.

It seems that "emittable" kinda means "streamable" here. Which got me thinking… why do we even need all this? Can't we just change HtmlResponse to extend StreamedResponse instead of Response?

Then we could have a RESPONSE event listener that does setCallback('BigPipeStreamer::send'), which then results in index.php-> $response->send() -> StreamedResponse::send() -> BigPipeStreamer::send(). The only thing you can't do with that AFAICT is control how headers are sent.

Just playing the devil's advocate here, just trying to minimize change.

I think the answer to #16 belongs in the IS, because I strongly suspect a committer will ask the same question.

Oh, wow, what a useless piece of ****** is StreamedResponse!? Indeed, that doesn't work at all.

Ok, so let's continue with the current approach. I'm happy to review.

So, from #5:

#2577631: Allow HtmlResponse to use a flexible emitter is a must-have as neither cloning the response nor creating our own Response is a clean solution in the end - as discussed with Crell.

So actually this is NOT clear why this makes it a must-have. If it's just for the sake of making it nicer, that doesn't make it a must-have.

The question is simple: is BigPipe possible without this? If no, then this is critical. If yes, then this isn't critical.

I'm still confused why this could be a must-have/critical if we've had a working BigPipe implementation without emitters.

Wim: The issue is the Response. Symfony Response objects are by default self-sending. That is, they have a single hard-coded emitter, send(). That can only be changed by extension, not by composition. That is the problem.

All responses get $response->send() called at the end. Period. That's where the actual printing happens.

In concept, suppose we did write a BigPipe routine, and stuck it into send(). How do you turn on bigpipe, then? You would have to change the response class you're using, at runtime, based on configuration. That's much more involved and complicated, and means you have 2 classes to maintain. If you have an HtmlResponse returned from somewhere (which does *not* have to come from the Renderer service, necessarily, so we can't rely on that), how do you "swap" from "normal page" to "BigPipe page"? You would have to add a Response listener that detects HtmlResponse (or similar), dissects it, and builds a BigPipeHtmlResponse out of it. That's extra cost, and very hard-coded. If you want another output mechanism you have to do that all over again.

If, instead, you allow a Response to opt-in to externalizing its send() (ie, composition), then a listener need only toss in an externalized send(), ie, an emitter. Very lightweight, very flexible.

So I guess it depends on your definition of "possible". Technically we could *probably* implement bigpipe without this approach, but the result would be so brittle that it would barely be usable, or worth using. Fabian?

(Side note: I think I will now use this example as my go-to for the danger of complecting a value object, Response, with behavior, emitting.)

I gather that the reason this is proposed as an RC target is that it's a soft/niceness blocker for BigPipe? I updated the summary to that effect. Would be good to answer #20 there as well.

Per Fabian, then.

And then present BigPipe as a complete and clean solution / product, rather than rush parts of this in - even though the disruption of this change here is zero.

Emphasis mine. This is my biggest concern also.

Until 8.1.0, the current solution, which indeed involves a response subscriber, will work just fine. #21 uses the adjective "brittle", which I think is a vast exaggeration. 99% of text/html responses in Drupal 8 will be HtmlResponse objects. We can't magically apply BigPipe to plain Response objects anyway.

This is new territory. I'm glad we're not rushing it in. Especially considering there's no API change anyway.

Looks nice. But still needs tests. And I'm not sure about the EmitterAwareResponseInterface name. I think ResponseStreamerInterface and StreamedResponseInterface would make more sense. Especially because #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits will likely add CacheableStreamedResponseInterface — see comments 44 and 45 there.

1. +++ b/core/lib/Drupal/Component/HttpFoundation/DefaultResponseEmitter.php
   @@ -0,0 +1,34 @@
   +/**
   + * @file Contains Drupal\Component\HttpFoundation\DefaultResponseEmitter.
   + */
   

   @file docblocks are not necessary anymore :)

2. +++ b/core/lib/Drupal/Component/HttpFoundation/DefaultResponseEmitter.php
   @@ -0,0 +1,34 @@
   + * Default Symfony response emitter.
   

   This is not Symfony, but Drupal. I guess the intent is to get this into upstream Symfony, and that's why it is named like this?

3. +++ b/core/lib/Drupal/Component/HttpFoundation/ResponseEmitterInterface.php
   @@ -0,0 +1,40 @@
   +  public function emit(Response $response);
   

   This must to be typehinted to EmitterAwareResponseInterface, not Response.

4. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
   @@ -61,7 +60,7 @@ public function onRespondEarly(FilterResponseEvent $event) {
   +   * Transforms a HtmlResponse to a response with BigPipe emitter.
   

   s/to a response with BigPipe emitter/to use the BigPipe emitter/

5. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
   @@ -91,16 +90,8 @@ public function onRespond(FilterResponseEvent $event) {
   +    $big_pipe_response = $response;
   +    $big_pipe_response->setResponseEmitter($this->bigPipe);
    
   

   Well this can then just become $response->setResponseEmitter($this->bigPipe)

Digging into a some performance issues on a site. Noticed this issue. Still needed or? The IS point to https://github.com/zendframework/zend-diactoros/blob/master/src/Response... that is archived and moved to https://github.com/laminas/laminas-diactoros/tree/3.4.x/src/Response that longer have this interface, could be renamed/moved though.

iirc this was mainly for https://www.drupal.org/project/big_pipe_sessionless but didn't end up either blocking that or the core bigpipe module.