Updated: Comment #4
Problem/Motivation
We want panels (everywhere) to be implemented without that many hacks/effort in D7. This patch makes the page-level rendering neatly pluggable, which means Panels Everywhere or other new page rendering strategies are equally viable.
Much of the code in our controller pipeline was written with the assumption that it would be replaced "later", after the system had matured a bit and SCOTCH figured out what it wanted to do to it. Unfortunately, that hasn't happened yet and for-reals SCOTCH is not happening in Drupal 8 core. That means we have a number of pieces of code that are not in a releaseable state. Most notably, the same render logic appears in HtmlPageController and in ViewSubscriber, the latter I think twice at least, and some of it is probably never even called. I've lost track of which of those code paths are even called.
Additionally, with the lack of global state (yay!) some modules that did wonky conditional logic in strange places no longer can do so. Some of that wonky logic is completely valid, though. For example: #1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all. (I'm marking this critical due to such regressions.)
We still need to eliminate the subrequests in the wrapping controllers, as they're causing more trouble than they're worth. See #1945024: Remove subrequests from central controllers and use the controller resolver directly..
Putting arbitrary pages into a dialog is clunky.
And of course there's the problem that using an anonymous undocumentable array as a universal data structure is just a bad idea in general, even if that's what we're doing now.
So, let's clean up this pipeline to be more consistent and structured in ways that can address the above issues.
Proposed resolution
The conerstone of this solution is to introduce a new HtmlPage object, which is strictly a data object representing a page. By that point, the "body" of the page is already string-ified, ie, the render array is already rendered. The rest of the page is really just the body and html classes, plus the contents of the HEAD element, but using structured objects rather than arrays.
Additionally, there was a lot of overlap and duplication between HtmlPageController and HtmlFormController, and some unpleasantness to make the latter work through a dialog. After some discussion with dawehner, we came up with a solution that greatly simplifies the concepts around controller handling.
Rather than _form in a route triggering a different wrapping controller, the main wrapping _controller is determined exclusively by the Accept header. The FormEnhancer now fills in only the _content property. Since forms need extra processing after the form object is called, that is handled by setting _content to an interstitial object that calls out to a dedicated interstitial FormController, which replaces HtmlFormController entirely. This is actually closer to what EntityRouteEnhancer was already doing, but it needed a little updating just the same. (This was originally done using a closure, which was far far simpler, but Drupal kept trying to serialize it.)
What that means is that any wrapping _controller can rely on _content being "the thing that gives you a render array that is your body". That thing may be a service, or a controller string, or a closure, doesn't matter. It returns the body, and the wrapping _controller does with it whatever is appropriate.
"Appropriate" in this case is to either return a Response (for the Ajax API-using controllers) or to return an HtmlPage object. It is never, ever to return a render array further down the pipeline. Just an HtmlPage object. (The legacy controller's closure has a lot of duplicated code at the moment, which I don't care about since that's slated for execution anyway. I decided it wasn't worth the effort to convert that to a dedicated interstitial class like Form, although it's nominally possible.)
It is then the responsibility of a view listener to, only, render HtmlPage into an HTML string. At present that's still done using the same tools (render arrays and html.html.twig), but we've therefore separated the body from the wrapper. The Twig folks, according to Jen Lampton, have bee considering not using the full render pipeline for the HTML tag, and this splits the problem space in two so that they could do that if desired.
This pattern of "return structured object from controller / mutate to Response in the view listener" is one I've seen and used elsewhere in the Symfony ecosystem. I actually built a project on it recently at work in Silex and it worked out extremely well.
The net result is that there is a list of data structures of increasing levels of abstraction: String -> render array -> HtmlFragment -> HtmlPage -> Response. A _content callable can return any of those it wants. A _controller can return any of the last 3, which are at the level of granularity that makes sense to be coming back from the controller layer. View listeners then convert any of those objects to the next higher object, until we get to a Response. Contrib listeners may then intercept that process at any level they want to modify the object or take over the entire process. That's a very low-complexity but high-power pipeline that gives contrib developers an enormous amount of flexibility.
Remaining tasks
There's some odd margin issues on pages rendered through the new system, where the page doesn't make it all the way to the edge of the screen. I suspect a class on the html element is missing along the way.- FIXED!The drupal_render_page() calls in overlay_render_region() and the Views Page plugin still need to be converted to use HtmlPage, or possibly HtmlFragment in the former case. That lets us remove drupal_render_page() entirely and remove the split-render for html/html_page and page/page_body- FIXED!- ExceptionController is still horrid. We should probably not try to un-horrify it here, but do that in a follow-up that we'd need anyway. (That is, this is not new technical debt.) ViewSubscriber could also still use work.
The handling of HtmlFormController right now has probably broken forms-in-dialogs. I'm still talking through how to fix that with dawehner. Suggestions welcome.- FIXED!- We probably need to do some trickery to allow an Exception controller to still call a View listener, so that we can expose non-200 HtmlPage objects to view subscribers. That's needed for #1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all. That may or may not be in this issue. If we don't do it here, though, it's still not a new bug as that issue shows contrib can't do what it needs now anyway.
- This does not do anything interesting with assets (CSS/JS). There's some stubs in there from SCOTCH for doing so, but that is a follow-up. (Also not new technical debt.)
- HtmlEntityFormController is now misnamed, since there's nothing page-centric about it. EntityFormController is already taken as a class name, however. Suggestions for an alternate name are welcome. :-)
Note: To be clear, this is *not* SCOTCH. We don't get any new layout capabilities with this. Some of the code was borrowed from SCOTCH, but not much. Really all this does is move around the code that SCOTCH would be replacing in contrib anyway. (Mainly HtmlPageController, as it would no longer be concerned with view listeners. How SCOTCH/PanelsTNG goes about creating an HtmlPage object is its business, as long as it returns one.)
Jen Lampton also mentioned that RenderWrapper may go away later. This issue does not make that any easier or harder.
User interface changes
N/A
API changes
For most module developers, minimal if any. The main change is an addition: The HtmlPage object is available in view listeners, which should let them modify HTML head information in a more structured way.
For internals folks, the page is no longer rendered as one massive array from HTML tag to HTML tag. Instead, it is rendered as a slightly less-massive array for the contents of the body tag, and then separately for the HTML page itself, treating the body as an opaque string. The number of people affected by that can probably be counted on one hand and are all core developers working on Twig. Jen Lampton confirmed at MWDS that this is consistent with the direction they're going anyway.
Related Issues
#1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
#1969270: 403/404 pages: drupal_get_http_header('Status') returns no status code at all
#1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]
#1945024: Remove subrequests from central controllers and use the controller resolver directly.
#2028749: Explore addressable block rendering (blocked by this issue)
Notes
Development is happening in the WSCCI sandbox, in the htmlpage branch here: https://drupal.org/node/1260830/git-instructions/2068471-htmlpage
Please don't just post patches; at least include interdiffs, or better yet let's collaborate in the sandbox.
Comment | File | Size | Author |
---|---|---|---|
#283 | interdiff.txt | 1.37 KB | dawehner |
#283 | status-page-quickfix-htmlpage.patch | 1.78 KB | dawehner |
#282 | status-page-quickfix.patch | 1.71 KB | Crell |
#257 | htmlpage-2068471.patch | 130.26 KB | dawehner |
#241 | htmlpage-2068471.patch | 130.28 KB | dawehner |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch. I'm fairly certain this won't pass yet, but I want to see how badly it doesn't pass. :-)
Comment #2
Crell CreditAttribution: Crell commentedOh yeah, that...
Comment #3.0
(not verified) CreditAttribution: commentedAdd link to sandbox.
Comment #4
Crell CreditAttribution: Crell commentedAfter some discussion with dawhener in IRC, we came up with a solution for handling forms that I really quite like. Rather than explain it here I've updated the issue summary.
This should hopefully have fewer fails. :-) It also converts DialogController to not use a subrequest, which we need to do anyway. I've manually tested and confirmed that forms through a dialog seem to work. Yay!
Comment #4.0
Crell CreditAttribution: Crell commentedAdd more related issues.
Comment #6
tstoecklerPer #1976158: Rename entity storage/list/form/render "controllers" to handlers EntityFormController will be renamed to simply EntityForm, so I think claiming EntityFormController here should not be problem. They should be in a different namespace anyway (?!) so the only problem is the confusion between the two and that, again, should be temporary.
Comment #7
Crell CreditAttribution: Crell commentedAt the moment they're both in the Drupal\Core\Entity namespace. I'm fine with moving [Html]EntityFormController to the Controller namespace rather than Entity namespace if everyone else is. It doesn't matter to me.
Looks like there's a lot of places in core that are still passing render arrays back themselves. Hm. Lots of misbehaving code. *sigh*
Comment #8
dawehnerNote: Batch is fixed, though this is now a real good test to see how many places are still relying on non httpfoundation code, like printing content.
#1987598: Convert aggregator_test_feed() and aggregator_test_redirect() to a new style controller would fix an aggregator test fail.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedI must be missing something. Why do we need those wrapping controllers to begin with?
If I understand correctly, they all do post-processing, so couldn't we just move them to the
KernelEvents::VIEW
event, possibly implemented as their own subscribers?In essence, in this approach we move what Symfony usually calls a controller (
_controller
) to_content
, and move the rendering that Symfony full stack does inKernelEvents::VIEW
inside_controller
. Or a least part of the rendering, because for some reason, we are splitting the rendering of the page structure and the rendering of the wrapper in two completely different spots.The architecture issue shows, because this doesn't make sense whatsoever:
You should be able to render an exception to HTML, AJAX, Dialog and JSON.
Comment #11
Crell CreditAttribution: Crell commentedAs noted in the summary, ExceptionController is currently a mess. We're not, yet, cleaning that up.
The reason for the double-layer controller is that if we let a render array go all the way to the view subscriber we have no good way to what the envelope is. (HTML page, Dialog, ESI of a block, etc.) Essentially, we can either do content negotiation pre-controller (via the enhancers) or post-controller (in a view subscriber that tries to figure it out), but right now we're kinda-sorta doing it in both places, which is just wrong. Switching to a structured object rather than unstructured array makes it much easier for view listeners to cherry-pick what they want to deal with, and do so using a cleaner API. (instanceof is fast and self-documenting.)
Symfony fullstack doesn't have the double-layer because it doesn't have double-layer theming the way we do. Each controller returns the Twig directives for the entire page, which is rendered as a single template (albeit with Twig template inheritance). As I understand it, Symfony CMF does something vaguely similar to what we're doing, though.
The double-layer approach has been with us since Munich, and was developed in a discussion with Fabien and Lukas Smith (Symfony CMF), so I think that's a good sign that it's a solid model. It's also the direction we've been moving closer to for the past many-months. This just cleans up the existing logic.
The "for some reason" that we're splitting the page and body rendering is simple: They're different things. SCOTCH, for instance, only wants to replace the contents of body. It is not, and should not be, interested in messing with the envelope rendering. This gives SCOTCH (or any other alternate render/layout mechanism) a single over-ride point to take over the entire body area and do what it wants, while not disrupting the rest of core.
Comment #12
dawehnerPosting an interdiff from #4 to #8
Comment #13
Crell CreditAttribution: Crell commentedOK, so it looks like there's a fair number of tests that are failing because they are misusing _controller where they should be using _content. That's resulting in them returning a 500 error due to lack of rendering in View Subscriber, which is causing a cascade of fails.
This is why we need to not have 2 intertwined rendering pipelines. :-)
To help find those, I've restored the code that renders a render array in the view subscriber but instead now raises an E_USER_DEPRECATED message. That should give us a more accurate failure list, and highlight the specific routes that are misbehaving.
In other news, batch API is still a mess.
Comment #15
sdboyer CreditAttribution: sdboyer commentedi think i like this idea. blocks return HtmlFragment (in SCOTCH), and primary page controllers return HtmlPage, which is a specialization of HtmlFragment.
the more i worked on this problem in SCOTCH, though, the more it seemed to me that we needed to have both of these objects return something that subclasses Response. i can't see a way to do subrequests if we don't. Crell pointed out in IRC that we could have a crappy object that just subclasses Response and holds the HtmlFragment as an envelope to traverse the barrier - true. and we could also just skip subrequests entirely - also true. the only drawback to that is it would be harder for beejeebus and msonnabaum to end arguments with "because http."
the exception stuff does clearly need dealing with; we talked about that in IRC as well, and it needs more resolution. but this is a good incremental step forward - we still need to get rid of horse shit like body classes derived from
menu_get_object()
andpage_top
andpage_bottom
, but those can come...later?Comment #16
dawehnerThe overlay fails are because it alters both the full page, by adding an additional theme wrapper, but also changing the page_top region manually.
Comment #17
dawehnerIt seemed to be that #13 runned into a broken testrun, with broken configuration directories etc.
Comment #19
dawehnerHEre is a rebased patch (@crell The rebased branch is not pushed yet).
Comment #21
dawehnerSome of the test failures could be related to #2071969: Serialize interface not reliable as the error message is
Drupal\field\Entity\FieldInstance::serialize() must return a string or NULL
Applying the patch from that issue though did not helped.
This fixes most of the aggregator tests. Additional there is a patch which converts the ajax controller to not use a subrequest.
Comment #23
Crell CreditAttribution: Crell commentedMore cleanup by both dawehner and myself.
Comment #25
Crell CreditAttribution: Crell commentedOK, let's try rebasing.
Comment #27
dawehnerThis adds some fixes here and there.
The "-serialization.patch" has #2004282: Injected dependencies and serialization hell applied, as it might be interesting whether this fixes many problems.
Comment #29
dawehnerSo, realized that this was a PHP 5.4 syntax.
Comment #31
BerdirWhy bother with the factory and not just do new MemoryStorage('state')?
Comment #32
dawehnerGood point berdir! Sometimes you just think to complex.
Rebased against 8.x
Comment #34
dawehnerWhile trying to fix the views UI preview I figured something out: Our request object now contains _content and _controller,
with _content potentially being a closure. At the same time, the request object is injected into things like a form and boom
you end up with a non serializable stuff.
Comment #35
Crell CreditAttribution: Crell commented*sigh* Forms are why we can't have nice things.
I can probably refactor the closures out and use objects instead. But really, the problem is we're serializing far too much, and using the request in too many places.
Comment #36
dawehnerLet's see how much is broken now.
Comment #38
dawehnerJust another rerole, one with and one without the serialization patch.
Comment #40
dawehnerdawehner--
Comment #42
Crell CreditAttribution: Crell commentedRebased to chase lots of recent changes in HEAD. Still working on some bug fixes but I wanted to get this into the queue ASAP (and so future interdiffs make sense).
Comment #43
Crell CreditAttribution: Crell commentedNote for dawehner: Looks like the Views http status code issue is due to it assuming Views is responsible for the Response object, which it isn't anymore (I think). It needs to set the status code on the HtmlPage object.
Comment #45
Crell CreditAttribution: Crell commentedI think some of the failures were caused by the temporary theming fork I added. Since we've cleaned up the routes that were misbehaving I've pulled that back out, which has fixed a couple of things for me locally. Let's see if the bot agrees.
Comment #47
dawehner#45: 2068471-htmlpage.patch queued for re-testing.
Comment #49
dawehnerA couple of more fix and a specific attribute to be able to set the statuscode on the exception subrequest.
Comment #51
dawehnerThis patch is not committed, just try to figure out whether this fixes the issue. (might be a php 5.3 thing)
Comment #53
dawehnerDamnit.
Comment #54
pwolanin CreditAttribution: pwolanin commentedon404Html() is setting _exception_statuscode to 403
Comment #56
Crell CreditAttribution: Crell commenteddawehner: the extra listener seems terrible. ;-) That should be possible to fold directly into the existing listener, no? I don't see how making it a separate listener would work, or for that matter even be a good idea.
Comment #57
dawehnerHa, who ever guessed that drupal would be bugfree, see #2083941: \Drupal\Core\Theme\Attribute->value() is named wrong and does not work
Comment #59
dawehnerA couple of fixes later.
Comment #59.0
dawehnerAdd description of form interstitial et al.
Comment #61
dawehnerThe remaining ones will really be fun. No real idea yet.
Comment #63
Crell CreditAttribution: Crell commentedSetting to CNR to get more eyeballs on this besides dawehner and I. I don't want to have big architectural debates only AFTER we suss out the edgy bugs that change what edgy bugs we have to fix.
So far the main feedback was from sdboyer, which was an overall +1. (I disagree about making HtmlPage subclass Response; my work with Silex has made me realize that's probably a bad idea.) Does anyone else want to +1 the concept, or -0.5 it with comments, or...?
(I'm on vacation right now so not really coding, but I can architect anywhere!)
Comment #64
disasm CreditAttribution: disasm commentedI have not reviewed the patch, but in reviewing the issue summary, I think this makes a lot of sense. +1 here.
Comment #65
catchI haven't reviewed the whole patch yet, but just quickly:
This is relying on drupal_render() putting css/js assets into a static. If we're not going to do drupal_render_page() on a big render array, and instead go back to passing strings around, then this needs to handle the collection of #attached properly - i.e. keeping the assets with the HTML string until they're put into HTML HEAD.
drupal_render_collect_attached() does this. I see an asset bag in here but not any mechanism to put things into it or take them out. sdboyer had some of this going with the partial page response work iirc.
Comment #66
Crell CreditAttribution: Crell commentedYes, I'm not dealing with assets in this patch. I kept some of those methods around on the assumption that sdboyer et al will be using them, but since I don't fully know what the status is there I thought it best to keep the scope here small. AFAIK nothing here increases the work to deal with #attached as planned; it just moves it around a bit. (If it does make it appreciably harder, that's something we should fix before this is committed.) For now I'm trying to avoid scope creep. I'd be OK with removing the unused asset methods if sdboyer is OK with it, assuming he'll just add them back later.
Comment #67
sdboyer CreditAttribution: sdboyer commentedyeah, i'd say go ahead and kill the asset methods. i can reintroduce.
pursuant to that, the assets patch is alive again: #1762204-71: Introduce Assetic compatibility layer for core's internal handling of assets.
Comment #68
dawehnerMy rerole skills are not enough anymore to rerole that branch.
Comment #69
tim.plunkettI did my very best to reroll this. The branch was completely un-rebase-able.
Not yet pushed to any new branches.
Comment #70
dawehnerFixed some of the translations and pushed to 2068471-htmlpage-2 branch.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedrerolled but can't run batch so can't test locally, too late in the day to start looking at that
Comment #73
larowlanComment #75
dawehner@Larowlan ... we are using the 2068417-htmlpage branch of the wscci sandbox, ... anyway this rerole was easy.
Some fixes included.
Comment #76
dawehner.
Comment #78
dawehnerA pair of fixes.
Comment #80
dawehnerBack to the 4 failures of before.
Comment #81
andypostany reason for?
removed?
Comment #82
dawehnerGood first hunk!
The reason why I decided to keep the second hunk was that these lines should actually be used in the future.
Comment #83
dawehnerDown to one.
Comment #85
larowlanplease don't give me any commit credit for that re-roll above
Comment #86
dawehnerLet there be another reroll.
Comment #88
dawehnerAnother reroll failure.
Comment #90
dawehnerbla.
Comment #92
fubhy CreditAttribution: fubhy commentedRe-roll
Comment #94
dawehnerAnother saturday another rerole and another new branch: 2068471-htmlpage-4 ...
Comment #96
Dave ReidThe Link and MetaTag classes are too hard-coded. Out of the box it won't support OpenGraph meta tags or anything that needs the format of
<meta property="og:title" content="The Rock" />
(tags that don't use 'name').Here are the various formats we need:
http://drupalcode.org/project/metatag.git/blob/refs/heads/7.x-1.x:/metat...
Comment #97
Crell CreditAttribution: Crell commentedOK, back with some updates from Prague.
Per discussion with sdboyer and Dave's comment above, this patch changes the following:
- Removes the asset parts of HtmlFragment for now.
- Generalizes the MetaTag class a bit to support the use case Dave notes. (Note that link rev is obsolete in HTML5 and we're not supposed to use it, but the class here does support it: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link)
- Splits HtmlFragment from HtmlPage rendering. HtmlPageController now returns an HtmlFragment, not an HtmlPage. Instead, converting HtmlFragment to HtmlPage happens at the kernel.view layer. That creates effectively 3 "layers" at which a controller can respond: HtmlFragment, HtmlPage, or Response. The view step will pick up at whatever layer is returned and take it the rest of the way.
- Crucially, this also means that the body content area is string-ified in order to be put into the HtmlFragment... which we want for memory reasons.
- The actual process of turning a fragment into a page is outsourced to a new HtmlPageRenderer service. Right now it's just a cut and paste of what we're already doing, but that service is specifically "Insert Panels Everywhere Here" (or whatever else folks want to experiment with). That makes the HtmlViewSubscriber class super-thin, which is a good sign.
- The Html pipeline part of the view listeners has been split off to its own subscriber class, mostly just for organizational tidiness. It now contains pretty much no logic as it's all handled by HtmlPageRenderer.
The normalization here to HtmlFragment helps with the eventual goal of the content area being "just a block from a different place", which we very much want for caching and the eventual Scotch-in-Contrib.
Let's see just how many tests I broke... :-)
Comment #99
Crell CreditAttribution: Crell commentedWould you people please stop improving Drupal while I'm trying to roll patches?
Comment #101
dawehnerAnother day another rerole. Hopefully this will finally show test failures.
Comment #102
sdboyer CreditAttribution: sdboyer commentedoptimism! :P
re: interdiff in #97 - short version, YAY.
slightly longer: this is almost exactly what i'd done locally, but hadn't pushed anywhere (sorry). the difference is that i'd lumped ...
these two methods into a single one. but this separation is better - as Larry points out, it does subdivide the layers nicely, and leaves ample priority space for other subscribers to play around between them.
in a follow-up, i intend to argue that the title handling should move down into the view subscriber layer, as well. but this is still a commendable step forward for now.
if it isn't clear to folks, this separation is roughly analogous to what we have in D7 with the page callback, the output of which is then sent through a delivery callback via
drupal_deliver_page()
. turns out, that's a good conceptual model. i'll make the argument on exactly why in the separate issue for rolling over titles to view subscribers, or possibly another one...but yeah - good idea. cray cray.Comment #104
dawehnerLet's see.
Comment #106
dawehnerAnd more.
Comment #108
dawehnerThe rest of the failures is pretty interesting:
The comment preview title has to change based upon the preview submission, though due to some reasons (i think form cache) the new generated form][#title is not used.
The title of the manage fields page sort of relies on menu inheritance so it is not really clear how to fix it as there is both a title callback and a normal title involved.
Problem caused by a broken batch process
Problem caused by a broken batch process
Comment #109
tim.plunkettThe FieldUIRouteTest test is actually just wrong, see #210212: form date type: bug when two date elements in one form
Comment #110
tstoecklerRe #109 I think you linked the wrong issue?!
Comment #111
dawehnerNice! Here is the link tim was actually linking to: #2102125: Big Local Task Conversion
Comment #112
Crell CreditAttribution: Crell commentedTalked about this on the call today. dawehner is going to port over the fixes from #2102125: Big Local Task Conversion for the field UI tests. I'm going to investigate the batch issue, since it looks like batch is broken again. (Surprise surprise.) Not sure what to do about the other two.
Comment #113
dawehnerComment #113.0
dawehnerUpdate for recent code evolution.
Comment #114
cosmicdreams CreditAttribution: cosmicdreams commentedHey everyone, I've fixed up some aspects of ExceptionController here #2119237: use \Symfony\Component\Debug\Exception\FlattenException instead of \Symfony\Component\HttpKernel\Exception\FlattenException
Also, Whoohoo #2102125: Big Local Task Conversion was committed so this patch will need to be rerolled.
Comment #115
dawehner****
Comment #116
jibranWhile @dawehner is on the train. A patch from @dawehner.
Comment #118
dawehnerLet's try this one.
Comment #120
cosmicdreams CreditAttribution: cosmicdreams commented#118: 2068471.patch queued for re-testing.
Comment #122
dawehnerLet's see.
Comment #124
dawehnerMany of them cannot be reproduced.
Comment #126
dawehnerThe functionality is to redirect to the same page as the 403 URL after the login.
This works by using $request->query->destination. HEAD is doing a subrequest in the HtmlPageController, in which the form builder is initialized.
This patch adds a FormEnhancer which allows us to get rid of the subrequest for the form result. On the other hand though the FormEnhancer is initialized
already in the main request (the 403 page, not the form) as it is an event subscriber.
Given that the workaround is to set the currenty request manually.
In a nearly perfect world, our request service would have the request scope, which would kill the form builder if needed.
In an ideal world, we would have a synchronized request service.
Comment #126.0
dawehnerAdd related issue.
Comment #127
sdboyer CreditAttribution: sdboyer commentedyes!
i reviewed before the D7 upgrade, and all i really had were nits.
while i strongly dislike the adding of these properties in this place (as do all the frontenders i've talked to), i actually like the notion of a $page->getAttributes() place into which we've separated the responsibility. hopefully we'll be able to get rid of these hardcoded classes later on.
nit: s/At/Add/
i may have missed other small things, but i'd say this is good enough to go.
Comment #128
katbailey CreditAttribution: katbailey commentedJust noticed one thing: the HtmlPageController no longer uses the HttpKernel so it can be removed as a constructor param.
Comment #129
dawehnerI decided to move this issue back to needs review given that the batch still don't work as before.
Comment #130
webchickI think it'd be great to try and get this in before alpha5 (Nov 18), as it brings us that much closer to the routing system being done. Tagging as such.
Comment #131
Crell CreditAttribution: Crell commentedAttempted reroll... Also fixes the nits above, but not batch API. This will probably fail. :-/ Daniel, I pushed to an htmlpage-7 branch in the wscci sandbox.
Comment #133
dawehnerFixed the ajax dialog controller failure, I hope all the other ones were random.
One other problem which 100% blocks this issue from being committed is the following screenshot:
I tried to compare the previous html with the current one, a couple of points:
had the big page render array of dooom, while now you render the page specific asserts earlier, which probably causes the different order.
The library was added in views_preprocess_html, which does not fire anymore, as we don't have access to the page render array anymore, see the code:
In order to fix this I probably have to jump between the two HtmlViewSubscriber and do something similar. This sounds like a quite big API change, given that these are things mostly themers would like to do.
Comment #135
dawehnerHa, this was indeed my fault.
Comment #136
tstoecklerHmm... I can't reproduce the screenshot at least not on simplytest.me
Attached is a screenshot with FF25 on Ubuntu.
Comment #137
tim.plunkett135: htmlpage-2068471.patch queued for re-testing.
Comment #139
dawehnerJust another ordinary reroll.
Comment #141
dawehnerHa, this fix was easy.
This is just a normal reroll.
Comment #143
dawehnerThere we go.
Comment #144
dawehnerThings are not simple. After a diff of the html before and after it became clear that the following classes attached to the body
element cause the issues:
no-sidebars, sidebar-first ...
When crell started with this issue the logic of adding these classes was still in template_preprocess_html(). After #1982256: Clean up html.html.twig markup went
in this was moved more in the direction of theme specific preprocess functions. Let's have a look at one:
You need access to pretty much the full page array at this point in order to function. The solution is
to move this logic into the preproprocess_page functions, which then alters the page object.
Next problem: The batch.
In the current branch it breaks as for some reasons the drupal_get_schema() static variable contains information
from the DrupalUnitTest. This can be fixed by reset the drupal_get_schema() cache in the tearDown of the DUTB.
Once this is done the only problem is that the batch for some short of time shows the wrong theme, good luck, I give up at 4am.
Comment #145
tim.plunkettThe template_preprocess_html() attributes are added to the body element. template_preprocess_page() does not have access to that...
And those classes were in that function in D7 too, the html.html.twig issue just shuffled it a bit.
Comment #146
Jeff Burnz CreditAttribution: Jeff Burnz commented@145, but we could print attributes in page.html.twig for classes etc that need the full page array, all core themes have a wrapper div in page.html.twig and for some odd reason we do not currently print attributes there. That would seem reasonable to me, i.e. you need full page array then use preprocess_page.
Comment #147
dawehnerOh great, thank you!
This is just another rerole after theme callback, synthetic request, current user and more.
Comment #149
dawehner147: htmlpage-2068471.patch queued for re-testing.
Comment #151
dawehner147: htmlpage-2068471.patch queued for re-testing.
Comment #153
andypost147: htmlpage-2068471.patch queued for re-testing.
Comment #155
andypost@dawehner please re-roll the patch and does it still in https://drupal.org/node/1260830/git-instructions/2068471-htmlpage
Comment #156
dawehnerJust another normal rerole.
Comment #158
Crell CreditAttribution: Crell commentedComment #159
dawehnerThe rerole is currently blocked on #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
Comment #160
dawehnerThe theme callback issue seems to take awhile.
One small little detail while running the tests is the following: All tests seemed to be run twice, (the amount of time needed is basically x2 if you use the UI).
This could be caused by some kind of error which is also causing the active theme to switch temporarily AND OR cause the redirect to the do_nojs page.
In the meantime here is a re-roll, as for sure other places all around core conflicted, you really better not fall too much behind.
Comment #162
dawehner160: htmlpage-2068471.patch queued for re-testing.
Comment #165
dawehnerHere again is a try to use render arrays instead, though this still don't fix the batch process. (interdiff.txt)
The failure in DUTB is fixed with that patch (the interdiff is not included in there).
Comment #166
dawehnerThe 30 additional KB have been caused by some merge conflict in overlay.
Comment #167
yched CreditAttribution: yched commentedPatch is green, so I assume the batch fails are JS only ?
Are there specific steps to reproduce ?
Comment #168
Crell CreditAttribution: Crell commentedDo a fresh install; it should be successful.
Go run a simpletest or two. See if the UI behaves as it should.
If it does, RTBC and let's commit this, stat!
If it doesn't, help figure out why.
Comment #169
tim.plunkettI'll look at it as well.
Is it a good time for a super nitpicky review? I noticed a lot of little things (not included below).
Or should I just fix as many as I can?
Why is this declared twice?
I keep staring at this, but can't tell what the actual change is here.
Dupe
Comment #170
tim.plunkettHere is a video of the weirdness: https://www.monosnap.com/file/oNSo1cutlkVekcOpYVZ57CxgteCllv
The URL starts at http://d8/admin/config/development/testing
Running the test shows http://d8/batch?op=start&id=5
The weird flash is http://d8/batch?id=5&op=do_nojs
The final URL is http://d8/admin/config/development/testing/results/5
Comment #171
tim.plunkettThis fixes it for me.
Comment #173
tim.plunkettNot for installing though, clearly.
Needs
to install, didn't have a chance to test that with simpletest yet, gotta run.
Comment #174
tim.plunkettGuess I should have just posted that, it seems to work locally.
Comment #176
tim.plunkettHere are a bunch of changes to things I saw while reviewing. Many look to be just casualties of rerolling. I also took the liberty of cleaning up ExceptionControllerTest.
I tried to figure out the installation failure but I can't reproduce locally, either with run-tests.sh or manual installs.
Comment #178
Crell CreditAttribution: Crell commentedThe interdiff from #176 looks fine to me. Thanks, Tim! Now to figure out why testbot changed its mind about the patch... *sigh*
Comment #179
tim.plunkettI have no idea how to unbreak what I broke, but I know it fixes the batch UI and I'm fairly confident it's close to a proper fix.
But in the interest of keeping a patch green, here is #166 plus my fixes in #176, without the bits about the installer.
I've included them as a single interdiff for a way forward.
This should at least prove that my last set of changes were okay, and it's just the installer/batch part left.
Comment #181
tim.plunkett179: page-2068471-179.patch queued for re-testing.
Comment #182
dawehnerJust pushed a new rebase + some cleanups to the branch, though that obviously contains still the batch "fix".
Comment #183
yched CreditAttribution: yched commentedAgreed with the _batch_progress_page() fixes from @tim.plunkett.
(and checked with batch_test.module that JS batch progress pages are busted without)
Not sure why they would break the installer though. Install works fine here (UI and drush si)
Comment #184
dawehnerLet's be honest, i have a rapid heartbeat at the moment.
Note: the arguments to drupal_add_html_head has been too many levels deep and in the wrong order.
Comment #186
dawehnerAnd this time with the proper patch and interdiff.
Comment #189
dawehnerGive us our daily rerole.
Comment #190
tim.plunkettThe last patch still had the batch UI problem. My failed one earlier only worked for the UI because it didn't add the meta element at all, which is why it blew up for the bot.
I finally got to the bottom of this. I don't think it was the render array at all, but that's a step in the right direction anyway.
The real problem was that when not in maintenance mode, the #prefix/#suffix (noscript) was being ignored. This approach was hashed out between me, Crell, and msonnabaum.
Comment #194
dawehnerFixing these failures...
Comment #195
sdboyer CreditAttribution: sdboyer commentedgo go gadget awesomehat.
Comment #196
catchOK found several nitpicks and a few bits I didn't really get. The overall structure looks OK though.
These should either be uncommented or removed.
'get drupal_get_html_head() support' doesn't parse for me.
This could have a better summary, it's more or less the content of the return statement.
Is this actually always a render array, or can it be a string too? I know we wrap strings in #type => markup (in fact that happens above in this patch), but that happens after calling getContentResult(), not before.
This is a copy/paste from a method above (same question on the return value). Could it be on a base class? Or I guess it's a trait once we actually start using them.
Nice to see more of these going :)
Invokes the form and returns the result?
This comment says 'using reflection', but I can't see reflection used here.
This looks bizarre, but I guess #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API is already open.
Should either be uncommented or removed.
ibid.
Why does the entity route enhancer need the form builder?
Oh this is why :)
What can $this->renderer->renderPage() return apart from string?
The Twig folks have been removing direct calls to theme, and changing to render arrays with #theme instead. Any reason to not follow roughly the same pattern here?
I don't get why this object has links and metatags properties, but does not contain anything about assets. Is this explained somewhere? I know that's somewhat been punted based on Sam's work, but this looks very incomplete without those.
Again it's very strange to add this, when there's no support for JavaScript or CSS.
Given links can be added to any renderable item via #attached, I'd probably leave this out, then handle all of those together. It's different to title where there can only be one.
There's html attributes, page attributes, and here attributes is returning bodyAttributes. Reading through the patch I'm having trouble keeping track of which is which.
This sounds like the content top region, but surely it's not that?
Does it really contain assets? That was removed I thought.
For for.
How come this is necessary for test passes, seems unrelated to the rest of the patch? Could use a comment.
OK this partially explains why I was confused earlier. This isn't the conten_top and content_bottom regions, but instead page_top and page_bottom. Why aren't the methods named pageTop and pageBottom though?
Comment #197
sdboyer CreditAttribution: sdboyer commentedre: #196.16
yes, it's kinda been punted based on #1762204: Introduce Assetic compatibility layer for core's internal handling of assets, but we're gonna have some tricky stuff to deal with there, regardless. namely, all the stuff we discussed about letting assets proceed out-of-band through render arrays in Prague. that's the real work that needs to be done in order for a getAssets() method to make sense here, i think, and it doesn't really make sense to do that until we have the non-weight sorting framework that the assets patch introduces.
the other things - links, meta tags - are easy enough to deal with right here b/c they don't have the sensitive dependency and ordering concerns that we have with assets. hopefully it is enough to say, though, that we do intend to later add similar methods - probably either getAssets() or getCss(), getJs() - to this class.
Comment #198
Crell CreditAttribution: Crell commentedSome quick replies:
4) Technically, a controller can return a string, render array, HtmlFragment, HtmlPage, or Response (including subclasses thereof). All are supported, although the assumption is that render array is the typical case. In theory contrib could enable even more object types to be returned as long as there is an appropriate view listener. I'll adjust the documentation to be more generic.
5) Hm, possibly? I can investigate how easily we can make a base class here, or just punt and refactor these things to use traits instead once testbot allows traits. My preference is for the latter. (Rule of 3: Copy and pasting once is OK, more than that means you refactor.)
8, 9) That's I believe carried over from the existing FormController class. I'll adjust the doc (it's using reflection, but it's now inside a supporting object), but the actual technique used is not changed by this patch.
14) renderPage() is supposed to return a string. In testing, though, I ran into a few places where due to a bug NULL was returned, which caused the Response object to blow up in confusing ways. Casting to a string allowed it to fail more gracefully. (As a side-effect, it means another implementation of that service could return an object implementing __toString(). I can't think of a reason to do that off hand, but it makes it possible which I don't object to.)
15) As above, because we're just moving existing code around, really. If it's a simple function swap (s/theme/drupal_render/ or close) I will go ahead and make that change. But it's in code that is by design very isolated, and either one is still equally Dong It Wrong(tm) until that code is injectable.
16, 17) What Sam said. We're doing the Easy Bits here, and letting Sam deal with the Hard Bits. Also, this gives us a structured way in the view layer for modules like Metatags, Google Analytics, etc. to add header elements at the page level with context about the HTTP response code, something that render arrays do not have. That is, this functionality is a blocker for #1948588: Google Analytics 8 upgrade, among others.
18) The many attributes are in part inherited from the previous Princess code. HtmlFragment has an attributes element, but HtmlPage naturally has two (body and html elements). I wasn't entirely sure which one to make "the normal one". We can add htmlAttributes and bodyAttributes methods, but getAttributes will still be there and be available; one of the others will just be an alias for it. I can live with that if you can.
19, 23) Similar to 18. HtmlFragment has a property/methods getContent. HtmlPage extends HtmlFragment, so also has a getContent method. However, Drupal has 3 "page body content" regions right now for reasons that escape me. When I asked Jen Lampton about it back at MWDS she said (roughly) "yeah it's weird, we want to remove that, but leave it for now". So I did. However, since the method was already called getContent I named the others getContentTop/getContentBottom for consistency within the object. Having getPageTop, getContent, and getPageBottom didn't make much sense to me. Although now that I type that, maybe it does make sense since pageTop and pageBottom are things that only make sense in the context of an HtmlPage/body element? Input welcome here.
I'll try to make some of the above updates tonight. Now that we know what was breaking batch API (timplunkett++ !!!) it should, I hope, be a bit less fragile but I'd still prefer to get it committed sooner rather than later.
Comment #199
webchickI was asked to chime in about catch's naming concerns.
- In general I agree, the methods + the template variables matching makes the most sense to me.
- However, HtmlPage->pageTop() implies (to me, at least)
<html>...</head>
which is definitely not what this is.- We batted around a few ideas. bodyTop(), bodyContent(), bodyBottom() was the best we could come up with. I'm not sure that's overly stellar compared to what's already in the patch, though.
- However, we probably shouldn't mess with the template variable names (and definitely not in this patch) since in #469242: Move <head> outside page.tpl.php we deliberately named them that way because they represented the top/middle/bottom of page.tpl.php.
Comment #200
Crell CreditAttribution: Crell commentedPoint 3: The dreditor chunk doesn't say what you're actually referring to. I guessed that you meant the same block as point 4.
Point 11: In FormController, only a render array can be returned because it's returning a form array. So the @return line is accurate here.
Point 22: I think that was added by either Daniel or Tim in various rerolling. I'll let them comment.
Point 23: webchick, msonnabaum, timplunkett, and I argued colors for a while in IRC on the naming here. Mostly "what Angie said", but with one correction: The main method is still getContent(), because semantically it is the "meaningful payload" of the object/response; it's the same as getContent() on HtmlFragment (from which it inherits), and similar methods some REST format libraries I've seen. The bodyTop/bodyBottom stuff is incidental Drupal display stuff, so the fact that the methods are not identically named is not semantically incorrect.
Unfortunately, since the last patch something else broke in the installer. Just applying the latest patch above and trying to install results in Ajax errors in the installer. Also, the latest branch in the sandbox seemed to have conflicts for me that I could not explain so I started a new branch from the most recent patch here.
Code is in the 2068471-htmlpage-8 branch in the WSCCI sandbox. Interdiff is against the latest patch.
I do wish the installer wouldn't die on us at inconvenient times...
Comment #201
dawehnerRegarding 22
drupal_get_schema() has a static cache, which can only be cleared with $rebuild = TRUE passed in as second parameter. For some reasons somethings calls drupal_get_schema() in the test, so the non existing schemas from the test leaks into the simpletest batch request, and you get some error.
This just appeared on DUTB using the UI, so this might be not longer required after the batch fixes.
Comment #202
catchThat at least needs a long comment in the code.
Yeah you can set up the render array then run it through drupal_render() (if that function itself is not allowed to return a render array, which it likely isn't).
Hmm I remember another issue about this, but also not being convinced that there wasn't a point at which ga could interact already.
Not sure I understand this solution, if you mean there's three methods but one is the alias for another, then that doesn't sound ideal either. This might just need more docs.
I think that's a bit easier. The main thing for me is not conflicting with the default theme regions - because when I saw this the first couple of seconds I was trying to figure out if it was left over debug or something.
Comment #203
catchOK let's remove that and see what if anything breaks. If it's needed, then it either needs a comment and/or a follow-up issue.
Comment #204
Crell CreditAttribution: Crell commented#200 needs manual testing to verify/reject my installer experience. Then we can test the schema clear necessity.
catch: In the patch in #200:
* Long comment added.
* theme switched to drupal_render(). (Thanks to larowlan for showing me how.)
* Re point 18, I was confused with another bit of HtmlFragment. The attributes method was renamed appropriately.
* Plus the comments in #200 itself.
Comment #205
sunIs it really desirable to automatically wrap the page output in an HTML page?
I'm aware that we've overhauled the page rendering process for e.g. dialogs in D8 using a dedicated pipeline now, but I wonder whether this semi-hard binding between (inner) page and (outer/wrapping) html page output won't make other/custom use-cases harder?
Perhaps a stupid example, but "Render the complete page output of route /foo/bar into a block" might be an example where you'd want just the page output without the wrapping HTML page? (I guess there are better examples)
Wrapping the page output into an HTML page is certainly the primary use-case. However, by declaring "html" as theme wrapper in the theme hook definition, isn't it going to be hard(er) to remove the html wrapper for custom use-cases, since you'll have to find a way to conditionally override the page theming process?
Could we move the automated HTML page theme wrapper rendering into another spot?
It wasn't obvious what "normal" refers to.
Could we rename "normal" to "html" (or "htmlpage")?
Comment #206
Crell CreditAttribution: Crell commentedRE #205.1: Um. I don't even know why that line is there. That line is there in HEAD now, and I took it *out* in this patch. I'll have to check and see why it's showing up there again.
Comment #207
Crell CreditAttribution: Crell commentedI've pushed a few more fixes to the branch. Also, the installer issue is the same one that was affecting batch API before. The noscript tag around the meta-refresh tag is missing, which causes the page to refresh on its own out of sync with the ajax code. The problem is that the fix for that for batch API assumes the rendered code for the page runs through template_preprocess_page, but the maintenance page doesn't.
I think we just need to update template_preprocess_maintenance_page to match, but it's after midnight and I'm a pumpkin. Someone else can have a crack at it, or I'll see if I can tomorrow.
Comment #208
dawehnerFixed the installer
Comment #209
dawehnerFixed #2 of sun's comment.
Comment #212
jibran48 exception(s) not bad. We are real close.
Comment #213
dawehner@sun
Sadly renaming it to system.batch_page.html does not work at the moment as somewhere in the routing system the ordering does not work properly. I added a todo instead hoping you are okay with that
as that patch should really get in rather sooner than later.
The 48 notices can be fixed by both patches. It is hard to explain why this patch breaks the local tasks of views though the actual reason behind is that route rebuilding and rebuilding the local tasks info happened on the same request. In these cases the map of views between view ID/displayID and the used route name is not stored yet into state. It uses descruct() at the moment, which don't work on the same request.
The second patch introduces a new event, which comes at the end of the route building process.
The alternative is just isset() not existing entries, though I consider that more as a hack.
Comment #214
pwolanin CreditAttribution: pwolanin commenteddiff2 looks like magic or a bad hack that will not be easy to understand in the future.
Comment #216
sunSure, happy to ignore the system.batch_page.normal thingie for now — let's create a follow-up issue for that :)
That said, I just had the chance to review this patch in more depth, which triggered the following questions:
One of the most confusing parts for me in this patch is that the ExceptionController uses setter injection for the service container, whereas the new FormController and HtmlEntityFormController are using constructor injection.
I guess I do not understand the technical reasoning behind the different implementation (if there is any). Could we make that consistent?
2. Another inconsistency that I do not understand:
controller.form maps to FormController
controller.entityform maps to HtmlEntityFormController
Why does controller.entityform assume/enforce HTML?
3. Could we rename the existing exception_controller to controller.exception for consistency?
I'm aware that these issues are very minor compared to the overall goal and changes of this patch, but all of these inconsistencies make it really hard to understand the overall proposed changes (unless you're neck-deep into the code like you guys, I guess ;)).
If necessary, happy to defer that name/arch polishing to a follow-up issue (though I'd recommend to make that at least major then).
Minor: I had to study the code for some time to double-check why the body classes array was copied into a new variable to overwrite the original value later, but wasn't able to see one?
Was there a particular reason for doing so?
I'm not really able to see a functional difference between the existing and new code aside from the separate variable?
This part of the proposed changes; i.e., the objectified render elements for HTML link and meta tags, feels a bit like scope-creep... :-|
I perfectly understand where you're trying to get to, but to me, these changes leave the impression of trying to "sneak" a first prototype incarnation of objectified render element system into D8 core, deeply hidden as part of this patch. ;)
Obviously (and history shows sufficient evidence for that), I'm a big fan of sneaking new features and architectural changes into core via "unrelated" patches, too :)
However, this fancy stuff here clearly looks like a first big proposal for a new, object-oriented RenderNG API — certainly just a wrapper around existing render elements for now, but I don't really have the impression that it was properly discussed, and aside from the necessary changes to return raw data (instead of rendered strings), I also do not have the impression that the introduction of these classes is strictly required for the scope of this issue and patch?
I may be mistaken, but it appears the identical functionality could be achieved by making addLink() and addMetatag() accept existing/old-style render elements?
Again, please don't get me wrong; like you, I'd also love to proceed on that topic - and if it was "just me" who's to be considered and concerned, I'd be perfectly fine with sneaking that first incarnation into core here! :)
But I believe it is (also) a matter of respect for the larger Drupal community and other contributors to have (at least) a dedicated proposal and discussion for these suggested changes, as they're quite major.
Thoughts?
To be honest, with all the services, controllers, resolvers, wrappers, event subscribers, and classes being introduced here, I do not really understand anymore what the exact HTML page rendering flow is, and how and in which order things are being invoked...
I'm absolutely positive that you guys know what you're doing, and also, the contents of the individual classes do make some level of sense to me (that is, without being able to imagine a simplified call trace in my head), and the theoretical/abstract issue summary helps a bit, too.
I'm a bit concerned about the level of abstraction and complexity being introduced here and wonder (1) whether that is a temporary measure and will be simplified/polished later, or (2) whether it is actually as complex as it looks, and/or (3) whether there are any ways to simplify it?
From my perspective, it would be very helpful to see a quick run-down of how the intended HTML page rendering flow will look like, as I have a hard time to connect all the pieces into a mental model. Anything would work and help; an image, ASCII-art, or even a structured list only.
Possible? :)
Taking a big step back, I wonder whether the "HtmlPage" class being proposed here isn't actually a HTML Page Model ?
I've to admit that I'm not very familiar with theoretical concepts and programming pattern aspects, but based on what I've seen in other frameworks and languages, this class and concept appears to have a strong correlation to what is being called a Model elsewhere?
If that is the case (?), do you think it might help new developers having a MVC background if these classes would actually be named
Drupal\Core\Model\HtmlPageModel
?Again, please feel free to shoot me down and call this BS — I do not really have experience in that field (since that kind of architecture in other applications always threw me off as too complicated/over-engineered and, to some extent, is what made me use and love oldskool Drupal instead ;) [but I do understand the point of the code being proposed here, so no pun intended]).
Comment #217
dawehnerI cannot remember why the change was needed (something with notices maybe). Sadly the git history is lost.
Comment #218
Crell CreditAttribution: Crell commented#216.1: Filed a follow-up here: #2155909: Standardize controller service names and hierarchy
.2: That's because the value that's returned now isn't an array, but an ArrayAccess-implementing object. Because of quirks in PHP's handling therein we had to use a temp variable for it. It's annoying, but when written the intent was that the theming team already had it in for that block of code and wanted to clean it up anyway. Doing so would be a normal non-release-blocking cleanup.
.3: Ha! I'm not quite that underhanded, TYVM. :-P I am actually quite opposed to an OOP Render API-NG. The idea of "one highly abstracted render structure to rule them all" is fundamentally flawed, IMO; it doesn't matter if it's array-based or object-based.
The intent of HtmlPage and its dependent objects is to, yes, define model objects that have the appropriate level of abstraction for the level at which they operate; that is, HtmlPage is, well, an html page. The body is one big blob of string (with the top/bottom stuff being a legacy Drupalism). The important and relevant part is the structured header data, which can be manipulated with a purpose-built set of methods and objects rather than a generic, hard to document structure. How that body string gets built is, officially, not HtmlPage's problem. As written, that means that a controller could build its own HtmlPage and return it, and it would then have control of the *entire* body area. (Say, wrapping a legacy system.) However, the header would still be available for modules like metatags, Google Analytics, etc. to manipulate and Druplify.
So no, it's not a first step toward Render API-NG, which is why there is no issue to discuss such a shift. It's for doing exactly what it is doing. While [#] is likely to flesh it out a bit more, it would still not go anywhere further down than the block level. Further down than that is, in fact, something I would oppose at this point.
.4: I've added a paragraph to the summary to help clarify the net impact. At some point in the future I'll make some pretty pictures of it all but it's not that time yet. :-)
.5: See above regarding HtmlPage's intent. Yes it's a model, but putting "Model" on the end of the class is entirely redundant.
#217: I'm not a huge fan of the extra event, and I have no idea why it's suddenly necessary now when it hasn't been for the past few months. However, I agree with Peter that it's better than the alternative diff as it has fewer weird holes for bugs to hide in. Plus, it gets the patch green. :-)
I am probably still not eligible to RTBC this patch, but I would beg someone else to do so soon so we can get this put to bed. Pretty please? :-)
Comment #219
andypostsummary now looks better, some other nitpics
FormController as abstract now, any reason to register it in container?
really unnecessary change
seems need comment why HtmlPage could return aray that not Response
same here
Comment #220
dawehnerPushed a patch without metatag/link support to 2068471-htmlpage-8-metatag
Comment #221
dawehnerThank you for the reviews. Fixed the points of #219.
Comment #223
webchickThat is a truly exceptional number of exceptions, sir!
Comment #224
Crell CreditAttribution: Crell commentedThe extra $body_classes variable is required, because $variables['attributes'] is not an array. It's an Attribute object. Attached patch fixes that and adds documentation for why we do that. Interdiff is against #220.
Comment #225
Crell CreditAttribution: Crell commentedDamnit, d.o!
Comment #226
dawehnerOkay, this proves that the html classes change is needed at least for 5.3
Comment #227
dawehner@crell, it seems to be that your rerole got way bigger. This did not happened for me on the 2068471-htmlpage-8-metatag branch (sorry) for me.
Comment #229
dawehner226: htmlpage-2068471.patch queued for re-testing.
Comment #230
pwolanin CreditAttribution: pwolanin commenteddrupal_get_html_head() looks kind of weird wieth the render option - can't the caller render if needed?
Could use some more code comments explaining the logic in DialogController in
The doxygen for doesn't really match the code for method DialogController::getContentResult()
+ * @param mixed $controller_definition
+ * A controller definition string, or a callable object/closure.
but inside the code a callable object is not handled, only a closure.
Comment #232
pwolanin CreditAttribution: pwolanin commentedminor in AttributeValueBase::render()
Why not use isset()?
Comment #233
dawehnerThank you for the review.
The controller resolver takes care of the other usecases.
Comment #234
dawehnerMissed #232
Comment #235
dawehnerI predict that this won't be green again. There have been too many changes in similar regions of the code.
Comment #237
dawehnerScrewed the rerole.
Comment #238
Dries CreditAttribution: Dries commentedI read the issue summary and to me, the problem statement comes down to "the current situation is that the rendering pipeline is messy and clunky", but without fully describing why fixing this is a release-blocker/beta-blocker. Please add a section to the issue summary describing the implications of not doing this before Drupal 8 beta ships. In general, a section like that in issue summaries helps with release management.
Comment #239
dawehnerThere are many reasons why this should be in before beta:
First we change a bit the api, but in a positive way. Previously you had two ways of returning a html-page, like /node/1. Either
by specifying _content or _controller (in the .routing.yml file) and return a render array. This leads to several workarounds in the code
which have been know for problems (there has been for example several issues to add _title support in all the different code paths).
This patch now requiress to use _content if you deal with something on a page.
The second reason is that with the introduction of a htmlpage object and the abstractions around it, we allow for a potential much easier
panels/panels everywhere/panelizer in contrib, or even enable it.
And finally most important: several people have spent entire weekends since months (current branch name: 2068471-htmlpage-8-metadata :)) working on this piece
and are convinced that this both reduces the amount of code paths as well as allows improvements like a better exception handling.
Comment #240
dawehnerA weekend would not be a weekend without a rebase.
Comment #241
dawehnerKeeping the patch up to date.
Comment #242
tim.plunkettThis is ready.
I manually tested it again, and the metatag stuff has been removed, and all other feedback has been addressed.
Comment #243
xjm241: htmlpage-2068471.patch queued for re-testing.
Comment #244
webchickPassing back to catch.
Comment #245
catchFine with this now it no longer has the metatag stuff in it, but since Dries didn't seem sure about this in #28 giving him a chance to come back to it. Fine with anyone committing it if Dries gets back positively (or doesn't get back negatively).
Comment #246
dawehner241: htmlpage-2068471.patch queued for re-testing.
Comment #247
dawehner241: htmlpage-2068471.patch queued for re-testing.
Comment #248
dawehnerCan we please get this in rather sooner than later given that reroles on this issue are kind of a pain.
Comment #249
webchickDries isn't around until next Wednesday (Jan. 8) so I'd recommend not holding this up on him. I was on the phone with him when he posted #238, though, and I believe I can speak for him and say that this was more a general statement that issues like this that are a) marked critical, b) are 6 months past the API freeze deadline, c) change a bunch of stuff, should have an explanation as to why they should be release blocking in the issue summary. He wasn't really pushing back on this patch specifically.
So unassigning from him, but this is way above my pay grade so I'm not touching it myself.
Comment #250
dawehnerComment #251
dawehnerLet's hope https://drupal.org/node/2068471/revisions/view/6804401/6805377 can be read without actual reading the issue summary.
In contrast to other issue though, we have started near the API freeze
Comment #252
Crell CreditAttribution: Crell commentedExpanded that comment a bit more.
Comment #253
pounardis_callable() wouldn't be better there?
Comment #254
Crell CreditAttribution: Crell commentedWithout context I can't tell what you're referring to; we use that approach in a few places, including in HEAD now, I think...
Comment #255
pounardIt's within the last patch, code ends up with call_user_func_array() on the tested variable.
Comment #256
pounardAnyway from http://php.net/manual/de/functions.anonymous.php we can read:
Even thought they tend to add methods to the Closure objects and allow using $this in them doesn't mean external code should rely upon. I'd follow Mr. PHP Documentation about this one.
I'd also follow this comment http://stackoverflow.com/questions/6699573/php-force-instance-of-closure... which states:
The thread is actually about Closures; Considering PHP flexibility about what's callable or not, this sounds especially true in this use case.
Comment #257
dawehnerRerolled and fixed the three instanceof instances.
Comment #259
dsnopek257: htmlpage-2068471.patch queued for re-testing.
Comment #262
Crell CreditAttribution: Crell commentedSee, this is why I wanted context. The code currently assumes it's either a closure or a controller definition. This change makes it "callable" or controller definition. It changes the logic. That means a function name or some 2-part arrays will now fall into the first condition whereas they used to fall into the second. That's no good. It's specifically just anonymous functions we want to exclude.
Daniel, can you reupload the previous patch (give or take rerolling) to restore the original logic?
Comment #263
dawehnerGRRR
Comment #264
dawehnerComment #265
dawehnerComment #266
dawehnerYet another one.
Comment #267
catchSo I also question whether this is really critical, but while a large patch it's mostly a refactoring of incomplete wscci stuff and changes very little in the way of APIs, so gone ahead and committed/pushed to 8.x.
Will need a change notice.
Comment #268
larowlanGreat job dawehner
Comment #269
jbrown CreditAttribution: jbrown commentedThis commit broke admin/reports/status
Notice: Undefined index: #page in seven_preprocess_page() (line 38 of core/themes/seven/seven.theme).
Fatal error: Call to a member function getBodyAttributes() on a non-object in /home/jbrown/sites/drupal8/core/themes/seven/seven.theme on line 39
Comment #270
Crell CreditAttribution: Crell commentedInvestigating...
Comment #271
Crell CreditAttribution: Crell commentedTrivial fix.
(For those following, callables specified by _controller may no longer return render arrays. _content can, but not _controller. The confusion between what _content did and what _controller did is part of what prompted this issue in the first place. We just missed one spot that was doing it wrong all along.)
Comment #272
larowlanWe also need a follow up to add test coverage for that path
Comment #273
jbrown CreditAttribution: jbrown commentedPatch in #271 fixes it for me.
Comment #274
Crell CreditAttribution: Crell commentedlarwolan: Test coverage for "does this path work?" is a lot of work for fairly little gain. We have a lot of paths, and doing that piecemeal is foolish. The only way for that to make sense is a test that tries to fetch all routes defined in all routing.yml files (and possibly some dynamic ones); basically spidering the site. That would be a very slow test, and testbot is slow enough as is.
Comment #275
jbrown CreditAttribution: jbrown commentedSurely the functionality of admin/reports/status needs test coverage. It would have prevented this problem occurring.
Comment #276
effulgentsia CreditAttribution: effulgentsia commentedAgreed. We always need test coverage when fixing a regression. It might be impractical to ever achieve 100% test coverage, but we can at least ensure that we cover what has actually been known to break.
Comment #277
Crell CreditAttribution: Crell commentedNot every bug is a regression. This is only a regression in the most abstract academic definition. It's just a bug.
Do you really want a test that just ensures a 200 response from admin/reports/status? Fully testing the output of that report is totally out of scope...
Comment #278
effulgentsia CreditAttribution: effulgentsia commentedYes.
Agreed. Normal priority follow up for that.
A patch was committed to HEAD that resulted in a fatal error. Whether we call it a regression or not, it's a critical bug. Ideally, all bug fixes should include test coverage, but certainly all critical ones should.
Comment #280
Crell CreditAttribution: Crell commentedI still think this is unnecessary, but whatever.
I have absolutely no idea why Views UI cares about that path at all, so I'm going to guess that's transient weirdness in testbot. We'll see if it still fails.
Comment #281
tstoecklerI don't see those users being used at all, the 'administer site configuration' permission should be sufficient.
Bad copy-paste?
Comment #282
Crell CreditAttribution: Crell commentedYeah, both of those are lazy copy-paste-ing.
(I rarely write WebTests, favoring unit tests for obvious reasons, so I'm a bit out of practice with 'em.)
Comment #283
dawehnerJust nitpicking.
Comment #284
yched CreditAttribution: yched commentedFrom the patch that got committed :
What's the status of this ? Is there an associated @todo ?
Comment #285
pounard@#262 Relying on PHP internals to seclude \Closure from a controller definition seems a bug in the design, whereas anything that can be called should be called directly IMO. It sounds being a realy architecture error to document and rely on the \Closure class since its definition supposedly is only internal and should not be directly used. If a controller definition falls into being a callable then you have a real problem with how you define your controllers.
Comment #286
dawehner@pounard
Closures won't be used anyway, to be honest.
Comment #287
Crell CreditAttribution: Crell commentedLet's fix the immediate need.
yched: I think that was probably just dead code we forgot to remove. Let's remove it.
pounard: Please make a note of that in #2155909: Standardize controller service names and hierarchy, which is already a follow-up for minor cleanup of the classes there.
Comment #288
webchickI thought it was odd to add a whole new test just for this thing, but then in grepping I can't seem to find evidence we have any test coverage at all for the paths under admin/reports. So filed #2165941: Write tests for system reports as a major task (rather than normal) because that seems like a pretty big deal.
Committed and pushed to 8.x. Thanks!
Back to active for the change notice.
Comment #289
yched CreditAttribution: yched commented#284 / #284
#2166059: Leftover commented-out code after "Page object" patch
Comment #290
ianthomas_ukThe committed patch calls drupal_container() which was recently removed. I've opened a followup: #2167517: system_custom_theme calls removed function drupal_container
Comment #291
jessebeach CreditAttribution: jessebeach commentedAdded the
Needs change notification
tag.Comment #292
jessebeach CreditAttribution: jessebeach commentedI'll write this change notice. I want to understand better what happened here.
Comment #293
jessebeach CreditAttribution: jessebeach commentedChange record posted: https://drupal.org/node/2170551
Comment #294
dawehnerNice! I added a small text about the actual API change.
In general we should maybe also describe the idea behind the HtmlFragment which would allow us to unify the page and block rendering pipeline.
Comment #295
jessebeach CreditAttribution: jessebeach commentedMaybe just mention the HtmlFragment/page/block idea in a comment? Since it's possible but not implemented?
Comment #296
jessebeach CreditAttribution: jessebeach commentedComment #297
Crell CreditAttribution: Crell commentedThen I think we're done here. Follow-ups are happening elsewhere.
Comment #298
tstoeckler#2167039: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page() revealed a regression that was introduced here, so referencing that here as well.
Comment #299
jibranThank you @dawehner, @Crell and @tim.plunkett for fixing this. @jessebeach thank you for the change notice.
I have read the change notice and made some formatting changes but sorry I am still not able to understand the essay, I have been following this issue since comment #8.
I am humbly requesting @dawehner and @Crell please update the doc in such a way so that everyone can understand the problem. We have added class diagrams, request flows, screenshots, code snippets, bullets, before after code, backlinks, doc links and etc. to previous change notices to make these more understandable. You have already done a fantastic work please can we have better docs. Thank you.
Sorry for changing the status we can mark it fixed once change notice is in better shape.
Comment #300
webchickThat sounds like a better fit for https://drupal.org/developing/api/8 ("Drupal 8 bootstrap walkthrough?") than the change notice, since change notices will only be read by people who are upgrading modules, vs. everyone using Drupal 8. Ideally, change notices are pretty short: what changed and why, before/after examples, and link off to more extended docs if needed.
What might help is for you to stub out the headings, etc. that you'd like to see, and then work with Larry/Daniel on creating the content that you need in order to understand better.
I think we can mark this fixed since the change notice exists, but leaving open for a touch yet to give jibran a chance to respond.
Comment #301
jibranThanks @webchick for the suggestion.
I'll give it a try.
Meanwhile I think there is no point in keeping this open. So back to fixed. :)
Comment #303
ianthomas_ukCan we now remove the code referencing this issue from template_preprocess_html()?
If we can, please try and avoid breaking the patch for #2209595: Remove drupal_set_title(), drupal_get_title() and associated tests in the process, as that is a beta blocker and touches the same code.
Comment #304
Wim LeersRelated: #2327277: [Meta] Page rendering meta discussion.