Problem/Motivation

This is a child issue of #2352155: Remove HtmlFragment/HtmlPage. Hence this is a critical task.

A big part of #2352155: Remove HtmlFragment/HtmlPage is making the render pipeline more understandable. Typically, Drupal renders the main content into HTML (format: html). But as part of WSCCI, Drupal 8 gained the ability to negotiate to a specific (HTTP parlance) Content-Type (in Symfony Request parlance: a request format).
So we must ensure that the part of the pipeline that takes the main content and turns it into a Response is understandable not only for html, but also for drupal_ajax, drupal_dialog and drupal_modal — which are the other supported formats.

Proposed resolution

I believe that as part of that, it'd be great if we would merge AjaxResponseRenderer into AjaxController, because:

  1. AjaxController is the only code that uses @ajax_response_renderer service at all
  2. AjaxResponseRenderer is already tightly coupled to it anyway
  3. together with the other changes in #2352155, this will make AjaxController contain/show the entire "main content -> Response" pipeline for drupal_ajax requests, aiding understandability

Remaining tasks

Review.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#10 rm_ajax_response_renderer-2364127-10.patch1.48 KBlussoluca
#1 rm_ajax_response_renderer-2364127-1.patch16.62 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new rm_ajax_response_renderer-2364127-1.patch16.62 KB
dawehner’s picture

dawehner
Primary language German
commented

It's hard to describe what I want to answer here.

First, as you wrote in the IS, this is not a real requirement to get the critical fixed. Having a class which does the actual logic,
and one class which is just a wrapper is not a bad thing at all. That concept is called separation of concern.
It's not clear why this is hard to understand (you have the exact class in that file), but in case it is, you have huge problems in Drupal 8.

If we move this into the AjaxController, there is no way to reuse the rendering logic, this is a pain. We accidentally removed
a lot of API in Drupal 8 as it got moved to protected methods on controllers, of wait, you have done this here :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I agree with the general things you say. But in this specific case, they don't apply, I think, precisely because AjaxResponseRenderer effectively can only be used by AjaxController anyway.

The actual rendering of AjaxResponses happens in AjaxResponse::prepare(). That's why I think it's fine to merge AjaxResponseRenderer into AjaxController.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Priority: Critical » Major

For me this is RTBC, will track down Daniel to see if he agrees, but this is not critical, but a major to have - given the diagram.

We added around 20 loc of which around 10 loc will be removed due to special cases going away.

But in turn we removed around 30-40 loc of boilerplate, which really is too specific to have a use somewhere else.

If I wanted my own ajax renderer, I would subclass ajax_controller service anyway and even if I need to replace the ::content method, its only 10 loc to copy and likely I might even call the parent()::content() method anyway, then alter it.

Therefore => clear win!

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Well whatever.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
dawehner: Fabianx: don't people write custom ajax callbacks and we could leverage it there?
dawehner: Fabianx: what about reusability benefit?
…
WimLeers: dawehner: custom ajax callbacks construct AjaxResponses, they don't build a render array and then call AjaxResponseRendererer; if they did, they would just have used _content + AjaxController.
dawehner: WimLeers: okay this is fair
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 40db9b9 and pushed to 8.0.x. Thanks!

  • alexpott committed 40db9b9 on 8.0.x 
    Issue #2364127 by Wim Leers: Merge AjaxResponseRenderer into...
lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented
Status: Fixed » Needs review
StatusFileSize
new rm_ajax_response_renderer-2364127-10.patch1.48 KB

I get a fatal error:

Recoverable fatal error: Argument 3 passed to Drupal\Core\EventSubscriber\ViewSubscriber::__construct() must be an instance of Drupal\Core\Ajax\AjaxResponseRenderer, none given

because the patch removes the third argument from the view_subscriber but not from the ViewSubscriber class

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Huh, wow, what!?!? That hunk should definitely have been in my patch also, and I'd swear it was… How the hell did my patch even pass testing then?

What a mess — sorry about this!

Thanks for the patch, lussoluca!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Given #10 we must be missing test coverage. Also I will no longer commit on issue followups. Please open a new issue. Why? Because this is a bug and the original issue was task so we can not manage and maintain the issue metadata correctly. See you in the new issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Would really love to know if there was ever another followup issue. I made this one: #2366645: Drupal\Tests\Core\Controller\AjaxControllerTest has wrong @covers.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Afaik no followup for #10 has been created.

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented

Followup here: #2366757: Fatal error on ViewSubscriber

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.