Symfony 2.2 includes a new rendering strategy system to replace the old and rather crufty way of handling subreqests/ESI/hInclude. However, we're still using a clone of the old one. Let's get caught up.

This patch (as soon as I have a nid) drops most of the old HttpKernel class and switches the HtmlPageController over to use the inline rendering strategy. However, the AjaxController has code in it to handle an Ajax sub-response. I didn't want to rip that out yet, but the new rendering strategy logic only returns a string, nothing else. Blargh. I therefore kept our subclass but stripped it down to JUST the short forward() method. The old render() method (which we weren't using anyway) is gone.

That *may* be a problem for SCOTCH, though, since we wanted to return PartialResponse objects, not strings. I'm not entirely sure what Sam has in mind here, but tagging for SCOTCH anyway.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
15.15 KB

And patch.

Status: Needs review » Needs work

The last submitted patch, 1945024-render-strategy.patch, failed testing.

larowlan’s picture

Note also #1944472: Add generic content handler for returning dialogs where we're aiming to use the Ajax controller in conjunction with a generic dialog controller that will in turn forward to the original non Javascript controller/content for that URL. Hopefully this will simplify logic for dialogs that just present content from a non Javascript fallback using the new Ajax commands from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases.

sdboyer’s picture

Issue tags: -WSCCI

i chatted with Crell about this in IRC, but i figured i should recap here.

the system, as written, doesn't earn us a lot. it's hampered by the fact that it's entirely reliant on "string-catching" between output-buffered request levels. the very fact that we're trying to encapsulate additional information - like assets - onto our HTML-ish responses means that we can't rely on that system. also, the job being done by the FragmentHandler here significantly overlaps with what the DisplayController (nee DrunkController) does, and being that that's the only case we really have for this right now, it's maybe not worth putting this in.

to be clear, that doesn't mean this isn't useful - just that we'll reuse the RenderableFragmentInterface while not using their code for it directly.

...at least, in SCOTCH land.

sdboyer’s picture

Issue tags: +WSCCI

whoops, didn't mean to untag.

but more importantly, on further review of the patch...eh, put it in (once it goes green). i can easily do the necessary targeted overrides on the SCOTCH work, and this is an incremental step forward from what we have now. i'm pulling a modified version of this patch into the SCOTCH repo now, anyway.

Crell’s picture

Looks like testbot just hiccuped before. Reviews, plz! :-)

Crell’s picture

Status: Needs work » Needs review
katbailey’s picture

Status: Needs review » Needs work

This needs a reroll to move the service registration into core.services.yml.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll

Needs a reroll for the core.services.yml but besides that, this seems like an awesome cleanup

ParisLiakos’s picture

Status: Needs review » Needs work

lol!

Crell’s picture

Status: Needs work » Needs review
FileSize
15.07 KB

Silly HEAD and it's movingness...

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony, -Needs reroll, -scotch

The last submitted patch, 1945024-render-strategy.patch, failed testing.

podarok’s picture

Status: Needs work » Needs review

#11: 1945024-render-strategy.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +Needs reroll, +scotch

The last submitted patch, 1945024-render-strategy.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.98 KB

Just a rerole.

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony, -Needs reroll, -scotch

The last submitted patch, render_strategy-1945024-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#15: render_strategy-1945024-15.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +Needs reroll, +scotch

The last submitted patch, render_strategy-1945024-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
16.11 KB

Let's inject the fragment handler.

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-19.patch, failed testing.

effulgentsia’s picture

Despite several rerolls since the last time this was tagged "Needs reroll", this does need yet another reroll, so leaving that tag.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.7 KB

He, that's true, here is another rerole, though this fails completely.

Note: I added setRequest to the http kernel to support the new synchronized services in symfony 2.3.

Exception thrown when handling an exception (Symfony\Component\DependencyInjection\Exception\RuntimeException: You tried to create the "finish_response_subscriber" service of an inactive scope.)

Currently I see a lot of this exceptions, so this won't work.

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-22.patch, failed testing.

dawehner’s picture

Oh, symfony 2.3 was not committed yet: #1989230: Update to Symfony 2.3

andyceo’s picture

Status: Needs work » Needs review
Issue tags: -WSCCI, -symfony, -Needs reroll, -scotch

#22: drupal-1945024-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#22: drupal-1945024-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#22: drupal-1945024-22.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +Needs reroll, +scotch

The last submitted patch, drupal-1945024-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.65 KB

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-31.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.15 KB

I don't think these services explicit has to be tagged with a response scope, especially in symfony 2.3.

Status: Needs review » Needs work

The last submitted patch, drupal-1945024-33.patch, failed testing.

sdboyer’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.83 KB
18.31 KB

The problem now is that controllers can't no longer return redirect responses, which then conflicts with all forms and the user/1 controller.

This patch adds the various services needed to make it running.

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony, -Needs reroll, -scotch

The last submitted patch, drupal-1945024-36.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

#36: drupal-1945024-36.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony, +Needs reroll, +scotch

The last submitted patch, drupal-1945024-36.patch, failed testing.

katbailey’s picture

Assigned: Crell » katbailey

I'm trying to see if I can make this patch be just about getting rid of FrameworkBundle's HttpKernel and our uses of subrequests. The patch in #2032535: Resolve 'title' using the route and render array does away with the subrequest in the HtmlPageController, but there's also DialogController and AjaxController to deal with so I've been wrestling with those. It's kind of gnarly :-( But I'd like to take a bit more time on it if that's ok.

larowlan’s picture

@katbailey I'm more than happy to help with the Dialog and Ajax controllers, let me know.

katbailey’s picture

Status: Needs work » Needs review
FileSize
31.13 KB

OK, here's a start at ripping out subrequests. Unfortunately I already know that the ajax_form_callback() is not handled (or the file upload ajax callback) and who knows what else is broken, but it's a start.

Status: Needs review » Needs work

The last submitted patch, httpkernel.1945024.41.patch, failed testing.

dawehner’s picture

So once #1987602: Convert ajax_form_callback() to a new style controller is on, this should also work?

+++ b/core/core.services.ymlundefined
@@ -182,6 +182,7 @@ services:
+    synchronized: true

Do you know the other issue on which people tried to make this change?

+++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.phpundefined
@@ -37,7 +37,7 @@ public function __construct(ContentNegotiation $negotiation) {
+    if (empty($defaults['_controller']) && in_array($this->negotiation->getContentType($request), array('html', 'drupal_ajax'))) {

Mh, I quite liked the previous behavior which did not hardcoded 'drupal_ajax' into the entity route enhancer, because it just magically worked with subrequests.

+++ b/core/core.services.ymlundefined
@@ -182,6 +182,7 @@ services:
+    synchronized: true

Do you know the other issue on which people tried to make this change?

katbailey’s picture

So once #1987602: Convert ajax_form_callback() to a new style controller is on, this should also work?

Ooh, yes indeed - it might actually be worth rolling a combined patch just to see.

Do you know the other issue on which people tried to make this change?

Nope, can you please provide a link?

I quite liked the previous behavior which did not hardcoded 'drupal_ajax' into the entity route enhancer, because it just magically worked with subrequests

Hmm, I prefer this over anything involving "magic" any day.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.39 KB

Ooh, yes indeed - it might actually be worth rolling a combined patch just to see.

Oh I probably mixed up 'synthetic' with 'synchronized'.

Let's see how much else is missing.

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony, -Needs reroll, -scotch

The last submitted patch, combine-1945024-46.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +symfony, +Needs reroll, +scotch

#42: httpkernel.1945024.41.patch queued for re-testing.

katbailey’s picture

Assigned: katbailey » Unassigned
dawehner’s picture

Title: Switch from FrameworkBundle's HttpKernel to rendering strategies » Remove subrequests from central controllers and use the controller resolver directly.
FileSize
31.16 KB

That is just a recent rerole of #42

Status: Needs review » Needs work

The last submitted patch, wscci-1945024-50.patch, failed testing.

dawehner’s picture

This issue is blocked upon #2032535: Resolve 'title' using the route and render array getting in, as it solves the problem in HtmlPageController but does actually work in constrast to this patch.

zaurav’s picture

Assigned: Unassigned » zaurav

Re-rolling

dawehner’s picture

Feel free to do a reroll, but we still should get in the title issue first...

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.27 KB

Rerolled, let's ask the bot.

Status: Needs review » Needs work

The last submitted patch, wscci-1945024-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
33.13 KB

Fixed the title handling in the dialog controller though the tests for form in the dialog breaks.

The reason is quite simple: The html form controller works in between, so in core you currently end up with a process which works like this:
subrequest
DialogController => HtmlFormController => ActualFormController

Hardcoding forms seems harder, so maybe we still need subrequests for dialog and maybe also ajaxController.

Status: Needs review » Needs work

The last submitted patch, wscii-1945024-57.patch, failed testing.

Crell’s picture

Crell’s picture

Status: Needs work » Postponed

Let's circle back after #2068471: Normalize Controller/View-listener behavior with a Page object is in, since it duplicates much of the work.

Crell’s picture

Issue summary: View changes

Added link

kim.pepper’s picture

Issue summary: View changes
Status: Postponed » Needs work
dawehner’s picture

Status: Needs work » Fixed

Afaik these have been all the subrequests in core

Crell’s picture

I just checked, and AjaxController, DialogController, FormController, HtmlControllerBase, HtmlFormController, and HtmlPageController are all subrequest-free. ExceptionController still has a subrequest, but it has other things that need to get fixed and will likely always have a subrequest in it.

So I think we can mark this issue fixed, no?

kim.pepper’s picture

Great!

Status: Fixed » Closed (fixed)

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