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.
Comment | File | Size | Author |
---|---|---|---|
#57 | wscii-1945024-57.patch | 33.13 KB | dawehner |
#57 | interdiff.txt | 3.92 KB | dawehner |
#55 | wscci-1945024-55.patch | 30.27 KB | dawehner |
#50 | wscci-1945024-50.patch | 31.16 KB | dawehner |
#46 | combine-1945024-46.patch | 60.39 KB | dawehner |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch.
Comment #3
larowlanNote 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.
Comment #4
sdboyer CreditAttribution: sdboyer commentedi 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 theDisplayController
(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.
Comment #5
sdboyer CreditAttribution: sdboyer commentedwhoops, 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.
Comment #6
Crell CreditAttribution: Crell commentedLooks like testbot just hiccuped before. Reviews, plz! :-)
Comment #7
Crell CreditAttribution: Crell commentedComment #8
katbailey CreditAttribution: katbailey commentedThis needs a reroll to move the service registration into core.services.yml.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedNeeds a reroll for the core.services.yml but besides that, this seems like an awesome cleanup
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedlol!
Comment #11
Crell CreditAttribution: Crell commentedSilly HEAD and it's movingness...
Comment #13
podarok#11: 1945024-render-strategy.patch queued for re-testing.
Comment #15
dawehnerJust a rerole.
Comment #17
dawehner#15: render_strategy-1945024-15.patch queued for re-testing.
Comment #19
dawehnerLet's inject the fragment handler.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedDespite several rerolls since the last time this was tagged "Needs reroll", this does need yet another reroll, so leaving that tag.
Comment #22
dawehnerHe, 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.
Currently I see a lot of this exceptions, so this won't work.
Comment #24
dawehnerOh, symfony 2.3 was not committed yet: #1989230: Update to Symfony 2.3
Comment #25
andyceo CreditAttribution: andyceo commented#22: drupal-1945024-22.patch queued for re-testing.
Comment #27
dawehner#22: drupal-1945024-22.patch queued for re-testing.
Comment #29
dawehner#22: drupal-1945024-22.patch queued for re-testing.
Comment #31
dawehnerComment #33
dawehnerI don't think these services explicit has to be tagged with a response scope, especially in symfony 2.3.
Comment #35
sdboyer CreditAttribution: sdboyer commentedrelevant: #2028749: Explore addressable block rendering
Comment #36
dawehnerThe 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.
Comment #38
jibran#36: drupal-1945024-36.patch queued for re-testing.
Comment #40
katbailey CreditAttribution: katbailey commentedI'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.
Comment #41
larowlan@katbailey I'm more than happy to help with the Dialog and Ajax controllers, let me know.
Comment #42
katbailey CreditAttribution: katbailey commentedOK, 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.
Comment #44
dawehnerSo once #1987602: Convert ajax_form_callback() to a new style controller is on, this should also work?
Do you know the other issue on which people tried to make this change?
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.
Do you know the other issue on which people tried to make this change?
Comment #45
katbailey CreditAttribution: katbailey commentedOoh, yes indeed - it might actually be worth rolling a combined patch just to see.
Nope, can you please provide a link?
Hmm, I prefer this over anything involving "magic" any day.
Comment #46
dawehnerOh I probably mixed up 'synthetic' with 'synchronized'.
Let's see how much else is missing.
Comment #48
dawehner#42: httpkernel.1945024.41.patch queued for re-testing.
Comment #49
katbailey CreditAttribution: katbailey commentedComment #50
dawehnerThat is just a recent rerole of #42
Comment #52
dawehnerThis 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.
Comment #53
zauravRe-rolling
Comment #54
dawehnerFeel free to do a reroll, but we still should get in the title issue first...
Comment #55
dawehnerRerolled, let's ask the bot.
Comment #57
dawehnerFixed 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.
Comment #59
Crell CreditAttribution: Crell commentedRelated, possibly duplicating: #2068471: Normalize Controller/View-listener behavior with a Page object
Comment #60
Crell CreditAttribution: Crell commentedLet's circle back after #2068471: Normalize Controller/View-listener behavior with a Page object is in, since it duplicates much of the work.
Comment #60.0
Crell CreditAttribution: Crell commentedAdded link
Comment #61
kim.pepperRe-opening since #2068471: Normalize Controller/View-listener behavior with a Page object went in.
Comment #62
dawehnerAfaik these have been all the subrequests in core
Comment #63
Crell CreditAttribution: Crell commentedI 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?
Comment #64
kim.pepperGreat!