In the current kernel, the View subscriber is hard coded to handle HTML and Core's flavor of JSON. This needs to be made extensible somehow to allow the addition of additional supported types, responses, etc.

At present it does not simply attach arbitrary view listeners for each output type because Fabien recommended against doing so, and that we make a single listener that has its own extensibility. That makes it a bit more encapsulated as a self-contained library rather than something that can only work when attached to Symfony's listeners. (Symfony's architecture strongly encourages free-standing fully self-contained libraries and thin bridges to link them to Symfony itself, which I agree is an overall good model.)

Discuss. :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

A single listener that itself calls out seems OK to me, rather than trying to make the actual listener attachment generic.

katbailey’s picture

Should it continue to be based off what gets returned from $this->negotiation->getContentType($request)? Right now the ViewSubscriber uses that to decide which of its own methods to call to create a response, so would a more extensible solution have the ViewSubscriber instantiate a class (that implements the "ViewResponderInterface" or whatever) that is somehow named after the format, e.g. json or html?
I'm having a hard time figuring out how this could work, because I don't see how the format would get mapped to the right class other than using a naming convention, but that doesn't seem like it would get us any further than what's there already, extensibilitywise.

I'm probably thinking about this wrong so am looking for guidance because I'm happy to take a crack at it once I get a better picture of how it could work.

katbailey’s picture

Status: Active » Postponed
Anonymous’s picture

Something related... by default, the call to $this->negotiation->getContentType($request) will only handle mimetypes that are in the list provided by Request::initializeFormats(). However, you can add to that list of mimetype/format mappings using Request::setFormat.

The module declaring the new format (such as JSON-LD) would need to be able to call setFormat on the request object before code execution reaches ViewSubscriber::onView.

moshe weitzman’s picture

catch’s picture

Priority: Normal » Major

Seems at least major to me.

katbailey’s picture

Status: Active » Needs work
FileSize
12.46 KB

Here's one possible approach, though it depends on #1706064: Compile the Injection Container to disk because it uses a compiler pass to add all services tagged with 'kernel.view.responder' as responders to the ViewSubscriber.
The patch probably won't even apply on top of HEAD so not setting to needs review, just sticking it up here in case anyone would like to comment on the approach...

pounard’s picture

I like this approach, but maybe those responders should be defined into a configuration file instead of being hardcoded so that people could disable them on a per-site basis? If you look at SF or ZF they both allow you to set your components per config then compile it into a static container which will be site-specific, I'd like this to happen for Drupal too, especially for things such as those responders.

Anonymous’s picture

It isn't clear to me how a contrib module would add a new responder, but that could be because I haven't been keeping up with the changes that WSCCI has brought.

If there was a JSON-LD module, how would it add its responder?

katbailey’s picture

@lin sorry I missed you earlier. Basically in order for a module to provide a responder, it would come with a bundle class and in its build method it would register a service tagged with ViewResponder. I guess it would also need to add an event listener that would listen for the KernelEvents::REQUEST event and call setFormat on the request, though I haven't given much thought to that so not totally sure that's the right approach.

I also saw your question in irc about achieving this without DIC compilation. I'm sure it's possible - I'm just not sure what the right approach would be. Using a compiler pass seems like a nice way to do it, but there might be an even nicer way...

Crell’s picture

Kris and I met with Fabien, Lukas, and assorted Drupalers yesterday and discussed how WSCCI and SCOTCH will hand-off. What we came up with, in short (more detailed writeup to come) involves moving what is currently a controller to a subcontroller, and introducing a new default controller. The default controller will start as, basically, a port of the html view subscriber. So that will call the sub-controller (page callback), and then do its thing and return a response. That means the view subscriber never gets called. Then SCOTCH can replace that with its own controller, doing whatever it wants.

I'm not 100% sure what the impact on this class is from that, other than it probably means it largely goes away. :-) For any non-HTML stuff we can require a controller to return a real response in the typical case. For HTML pages, it will never get called as the main controller will take care of it and always return a response.

katbailey’s picture

Status: Needs work » Postponed

Per today's wscci meeting, most of this work will get done in the router patch, #1606794: Implement new routing system. We may still need to revisit this specific use case once that's in though.

RobLoach’s picture

Status: Postponed » Active
Anonymous’s picture

Status: Active » Closed (duplicate)

I don't believe that we need ViewSubscriber to be extensible anymore. Any additional serialization that we would extend it with will instead be handled by the combination of REST/serialization that klausi and I are working on, so I'm going to mark this as duplicate of #1814864: Provide way to register serialization classes.

larowlan’s picture

Priority: Major » Minor
Status: Closed (duplicate) » Active

There is still a to-do in /core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.php pointing to this issue.
I guess that should be removed if this is closed.

larowlan’s picture

Status: Active » Needs review
Issue tags: -WSCCI +WSCCI-conversion
FileSize
2.36 KB

Here's a patch to remove that comment and document the suggested approach

larowlan’s picture

Spelled subscriber wrong, sorry testbot

larowlan’s picture

New wording to indicate both approaches

larowlan’s picture

Sorry bot, more refinemnet

kim.pepper’s picture

Status: Needs review » Needs work

Looks good. One minor nitpick.

+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.phpundefined
@@ -19,9 +19,36 @@
+ * @see Symfony\Component\EventDispatcher\EventSubscriberInterface

Needs preceding slash

Crell’s picture

Eh, there's nothing actually wrong with using view listeners. ViewSubscriber the class was only ever intended as a temporary shiv. I wouldn't want to warn people away from the very feature that SCOTCH is going to be leveraging heavily.

effulgentsia’s picture

Given #11, how is SCOTCH going to be leveraging VIEW subscribers heavily? I think we'll end up reducing Drupal core to just one VIEW subscriber that can turn a render array into an HtmlFragment. That's it. #1959574: Remove the deprecated Drupal 7 Ajax API is removing the remnants of using View subscribers for Ajax wrapping.

I like some of the information in #19, but I'm not sure that's where it belongs. I think instead, we need to clarify what sorts of things subcontrollers are for, and what sorts of things VIEW subscribers are for, rather than encouraging people to think of them interchangeably.

Crell’s picture

True, they are different things, although the way we're using them admittedly has some gray area in it, unfortunately. Some of our enhancers, I suppose, COULD be redone as view subscribers. Whether they should or not, I don't know.

effulgentsia’s picture

Whether they should or not, I don't know.

Based on #11, my take on it is this: if you want a route to return rendered content that can be wrapped in different ways, use _content/_form/etc. and let route enhancers set the _controller for doing the wrapping. I think all of our current enhancers plus the ones in #1944472: Add generic content handler for returning dialogs satisfy this, and should not be converted to view subscribers.

On the other hand, if you want your controller (or subcontroller) to return data as a PHP array or non-Response object, and allow that data to be rendered (as opposed to wrapped) in different ways, that's what View subscribers are for. I think ViewSubscriber::onJson() is a great example of this and ViewSubscriber::onHtml() would be if the drupal_render_page() were converted to drupal_render().

Based on http://stackoverflow.com/questions/12034187/symfony2-change-rendered-vie..., I guess there's Symfony projects that use view subscribers for swapping out templates, which fits the "here's where you render data" part of the pipeline, but in Drupal, we have drupal_render() and all of the alter hooks that act on render arrays as they're getting built, plus a theme system, so that's where all the rendering flexibility actually lives, so all we need a view subscriber for (when responding with html) is to call drupal_render().

Per #14, for serializing entities and other complex resources, we have rest.module, which uses Symfony's Serialization pipeline rather than View subscribers, so we don't need View subscriber format extensibility for that use case. That's because serializing complex resources in hypermedia friendly ways requires a lot more per-format / per-specification logic than just calling json_encode() or equivalent.

However, I do think we may want to change our autocomplete controllers and anything else returning simple data to return PHP arrays rather than JsonResponse objects, and let our ViewSubscriber::onJson() handle the conversion to JsonResponse. Then, if someone wants to implement autocompletes with XML as the transport format, they can do so by adding another view subscriber with an onXml() method, and not need to change the autocomplete routes or controllers.

the way we're using them admittedly has some gray area in it

Based on above, I think the remaining blurriness in our current code is that:
- ViewSubscriber still has the Ajax wrappers, but that's being removed in #1959574: Remove the deprecated Drupal 7 Ajax API.
- ViewSubscriber::onHtml() calls drupal_render_page() instead of drupal_render() (and letting HtmlPageController handle the page wrapping).
- AjaxController and HtmlPageController call drupal_render() / drupal_render_page() instead of working with HtmlFragment responses, but that's being worked on in #1812720: Implement the new panels-ish controller [it's a good purple].
- HtmlFormController doesn't use subrequests, so calls drupal_render_page() itself rather than delegating page wrapping to HtmlPageController, but we can fix that once #1812720: Implement the new panels-ish controller [it's a good purple] makes HtmlPageController more interesting.

Crell’s picture

Well, except that _content callbacks in a SCOTCH-y future should return a PageFragment object, which won't be a response. So that would suggest we'd almost always be using a view listener rather than a route enhancer for pages, but that's not what Sam is writing.

effulgentsia’s picture

I think we've disagreed on this before, and that's ok: when SCOTCH is further along, we can hopefully come to a resolution. But just for the record, my stance is that:
- Most _content callbacks should return render arrays, not PageFragment (is that different from HtmlFragment?) objects.
- There should be a View subscriber whose only job is to drupal_render() a render array into a PageFragment object. This might require a little (but only a little) more than just a drupal_render() call, depending on the details of what's needed for out of band info, like 'title' and assets.
- Whether or not PageFragment should be a Response or not is still TBD.
- The HtmlPageController (registered by the route enhancer) is responsible for assembling the PageFragment objects returned by subrequests into a Response object representing the complete page.

effulgentsia’s picture

Title: Make ViewSubscriber extensible » Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply
Priority: Minor » Normal
Issue tags: -WSCCI-conversion +WSCCI

@Crell and I discussed some stuff last week at MWDS that touches on this issue, but I'll let him explain it here (or link to the issues he's working on).

dawehner’s picture

Crell’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)

Closing as a duplicate of the issue from #28. There's been enough new work and thought elsewhere that this thread is no longer relevant. Thanks all.