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. :-)
Comment | File | Size | Author |
---|---|---|---|
#19 | view-subscriber-1594870.19.patch | 3.01 KB | larowlan |
#18 | view-subscriber-1594870.18.patch | 2.99 KB | larowlan |
#17 | view-subscriber-1594870.17.patch | 2.36 KB | larowlan |
#16 | view-subscriber-1594870.16.patch | 2.36 KB | larowlan |
#7 | 1594870.viewsubscriber.patch | 12.46 KB | katbailey |
Comments
Comment #1
catchA single listener that itself calls out seems OK to me, rather than trying to make the actual listener attachment generic.
Comment #2
katbailey CreditAttribution: katbailey commentedShould 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.
Comment #3
katbailey CreditAttribution: katbailey commentedMarking this as postponed until #1599108: Allow modules to register services and subscriber services (events) is in place.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedSomething 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.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commented#1599108: Allow modules to register services and subscriber services (events) is in. Game on.
Comment #6
catchSeems at least major to me.
Comment #7
katbailey CreditAttribution: katbailey commentedHere'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...
Comment #8
pounardI 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.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedIt 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?
Comment #10
katbailey CreditAttribution: katbailey commented@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...
Comment #11
Crell CreditAttribution: Crell commentedKris 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.
Comment #12
katbailey CreditAttribution: katbailey commentedPer 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.
Comment #13
RobLoach#1606794: Implement new routing system is in.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #15
larowlanThere 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.
Comment #16
larowlanHere's a patch to remove that comment and document the suggested approach
Comment #17
larowlanSpelled subscriber wrong, sorry testbot
Comment #18
larowlanNew wording to indicate both approaches
Comment #19
larowlanSorry bot, more refinemnet
Comment #20
kim.pepperLooks good. One minor nitpick.
Needs preceding slash
Comment #21
Crell CreditAttribution: Crell commentedEh, 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.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedGiven #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.
Comment #23
Crell CreditAttribution: Crell commentedTrue, 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.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedBased 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.
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.
Comment #25
Crell CreditAttribution: Crell commentedWell, 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.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #27
effulgentsia CreditAttribution: effulgentsia commented@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).
Comment #28
dawehnerHere is the issue: #2068471: Normalize Controller/View-listener behavior with a Page object
Comment #29
Crell CreditAttribution: Crell commentedClosing 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.