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.

Files: 
CommentFileSizeAuthor
#57 wscii-1945024-57.patch33.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,848 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#57 interdiff.txt3.92 KBdawehner
#55 wscci-1945024-55.patch30.27 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,322 pass(es), 9 fail(s), and 2 exception(s).
[ View ]
#50 wscci-1945024-50.patch31.16 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,409 pass(es), 18 fail(s), and 14 exception(s).
[ View ]
#46 combine-1945024-46.patch60.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,002 pass(es), 18 fail(s), and 14 exception(s).
[ View ]
#42 httpkernel.1945024.41.patch31.13 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch httpkernel.1945024.41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 interdiff.txt18.31 KBdawehner
#36 drupal-1945024-36.patch19.83 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1945024-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 drupal-1945024-33.patch19.15 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,001 pass(es), 1,001 fail(s), and 148 exception(s).
[ View ]
#31 drupal-1945024-31.patch18.65 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#22 drupal-1945024-22.patch18.7 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 drupal-1945024-19.patch16.11 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,889 pass(es), 76 fail(s), and 20 exception(s).
[ View ]
#19 interdiff.txt2.29 KBdawehner
#15 render_strategy-1945024-15.patch14.98 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,743 pass(es), 631 fail(s), and 459 exception(s).
[ View ]
#11 1945024-render-strategy.patch15.07 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945024-render-strategy_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1945024-render-strategy.patch15.15 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 54,138 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new15.15 KB
PASSED: [[SimpleTest]]: [MySQL] 54,138 pass(es).
[ View ]

And patch.

Status:Needs review» Needs work

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

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.

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.

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.

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

Status:Needs review» Needs work

lol!

Status:Needs work» Needs review
StatusFileSize
new15.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945024-render-strategy_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new14.98 KB
FAILED: [[SimpleTest]]: [MySQL] 55,743 pass(es), 631 fail(s), and 459 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.29 KB
new16.11 KB
FAILED: [[SimpleTest]]: [MySQL] 54,889 pass(es), 76 fail(s), and 20 exception(s).
[ View ]

Let's inject the fragment handler.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new18.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new18.65 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new19.15 KB
FAILED: [[SimpleTest]]: [MySQL] 56,001 pass(es), 1,001 fail(s), and 148 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new19.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1945024-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new18.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.

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new31.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch httpkernel.1945024.41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new60.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,002 pass(es), 18 fail(s), and 14 exception(s).
[ View ]

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.

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

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

Assigned:katbailey» Unassigned

Title:Switch from FrameworkBundle's HttpKernel to rendering strategiesRemove subrequests from central controllers and use the controller resolver directly.
StatusFileSize
new31.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,409 pass(es), 18 fail(s), and 14 exception(s).
[ View ]

That is just a recent rerole of #42

Status:Needs review» Needs work

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

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.

Assigned:Unassigned» sauravshrestha

Re-rolling

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

Status:Needs work» Needs review
StatusFileSize
new30.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,322 pass(es), 9 fail(s), and 2 exception(s).
[ View ]

Rerolled, let's ask the bot.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.92 KB
new33.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,848 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

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.

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.

Issue summary:View changes

Added link

Issue summary:View changes
Status:Postponed» Needs work

Status:Needs work» Fixed

Afaik these have been all the subrequests in core

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?

Great!

Status:Fixed» Closed (fixed)

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