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.

#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.

CommentFileSizeAuthor
#283 interdiff.txt1.37 KBdawehner
#283 status-page-quickfix-htmlpage.patch1.78 KBdawehner
#282 status-page-quickfix.patch1.71 KBCrell
#280 status-page-quickfix.patch2.05 KBCrell
#271 status-page-quickfix.patch557 bytesCrell
#266 htmlpage-2068471.patch130.28 KBdawehner
#263 htmlpage-2068471.patch130.28 KBdawehner
#257 interdiff.txt1.92 KBdawehner
#257 htmlpage-2068471.patch130.26 KBdawehner
#241 htmlpage-2068471.patch130.28 KBdawehner
#240 htmlpage-2068471.patch130.28 KBdawehner
#237 htmlpage-2068471.patch130.68 KBdawehner
#235 htmlpage-2068471.patch130.56 KBdawehner
#234 interdiff.txt567 bytesdawehner
#234 htmlpage-2068471.patch130.41 KBdawehner
#234 interdiff.txt567 bytesdawehner
#233 htmlpage-2068471.patch130.41 KBdawehner
#233 interdiff.txt3.07 KBdawehner
#226 interdiff.txt1.16 KBdawehner
#226 htmlpage-2068471.patch130.56 KBdawehner
#224 interdiff.txt2.44 KBCrell
#224 2068471-htmlpage.patch141.16 KBCrell
#221 htmlpage-2068471.patch129.79 KBdawehner
#221 interdiff.txt2.92 KBdawehner
#220 htmlpage-2068471.patch130.61 KBdawehner
#220 interdiff.txt6.59 KBdawehner
#217 interdiff.txt4.33 KBdawehner
#217 interdiff.txt0 bytesdawehner
#217 htmlpage-2068471.patch136.29 KBdawehner
#213 diff2.txt1.52 KBdawehner
#213 diff1.txt3.19 KBdawehner
#213 interdiff.txt454 bytesdawehner
#213 htmlpage-2068471.patch132.99 KBdawehner
#209 interdiff.txt963 bytesdawehner
#209 htmlpage-2068471.patch132.92 KBdawehner
#208 htmlpage-2068471.patch132.92 KBdawehner
#208 interdiff.txt511 bytesdawehner
#200 interdiff.txt12.77 KBCrell
#200 2068471-htmlpage.patch133.24 KBCrell
#194 interdiff.txt537 bytesdawehner
#194 htmlpage-2068471.patch133.34 KBdawehner
#194 interdiff.txt0 bytesdawehner
#190 page-2068471-190.patch133.75 KBtim.plunkett
#190 interdiff.txt2.92 KBtim.plunkett
#189 htmlpage-2068471.patch133.12 KBdawehner
#186 interdiff.txt5.22 KBdawehner
#186 htmlpage-2068471.patch133.8 KBdawehner
#184 htmlpage-2068471.patch131.7 KBdawehner
#184 interdiff.txt2.17 KBdawehner
#179 interdiff.txt2.37 KBtim.plunkett
#179 page-2068471-179.patch130.52 KBtim.plunkett
#176 interdiff.txt34.88 KBtim.plunkett
#176 page-2068471-176.patch132.89 KBtim.plunkett
#174 page-2068471-174.patch134.06 KBtim.plunkett
#171 interdiff.txt2.33 KBtim.plunkett
#171 page-controller-2068471-171.patch133.91 KBtim.plunkett
#166 htmlpage-2068471.patch131.7 KBdawehner
#165 interdiff.txt3.35 KBdawehner
#165 htmlpage-2068471.patch162.74 KBdawehner
#160 htmlpage-2068471.patch161.91 KBdawehner
#156 htmlpage-2068471.patch130.88 KBdawehner
#147 htmlpage-2068471.patch131.31 KBdawehner
#143 htmlpage-2068471.patch129.77 KBdawehner
#141 htmlpage-2068471.patch129.73 KBdawehner
#141 interdiff.txt469 bytesdawehner
#139 htmlpage-2068471.patch129.25 KBdawehner
#136 html-view-subscriber-simplytest.png46.22 KBtstoeckler
#135 htmlpage-2068471.patch130.08 KBdawehner
#135 interdiff.txt1.59 KBdawehner
#133 htmlpage-2068471.patch130.16 KBdawehner
#133 interdiff.txt7.79 KBdawehner
#133 diff.txt35.49 KBdawehner
#133 screenshot.png29.61 KBdawehner
#131 2068471-htmlpage-7.patch126.24 KBCrell
#126 htmlpage-2068471-126.patch126.91 KBdawehner
#126 interdiff.txt1.81 KBdawehner
#124 htmlpage-2068471-124.patch126.84 KBdawehner
#122 htmlpage-2068471-122.patch126.3 KBdawehner
#118 2068471.patch126.21 KBdawehner
#116 drupal8.routing-system.2068471-116.patch129.61 KBjibran
#113 htmlpage-2068471-113.patch125.18 KBdawehner
#106 htmlpage-2068471-106.patch117.06 KBdawehner
#106 interdiff.txt1.74 KBdawehner
#104 htmlpage-2068471-104.patch115.64 KBdawehner
#104 interdiff.txt6.01 KBdawehner
#101 htmlpage-2068471-2068471-101.patch114.02 KBdawehner
#99 2068471-htmlpage.patch116.19 KBCrell
#97 2068471-htmlpage.patch116.19 KBCrell
#97 interdiff.txt25.1 KBCrell
#94 htmlpage-2068471-94.patch111.68 KBdawehner
#92 drupal8.routing-system.2068471-92.patch111.8 KBfubhy
#90 htmlpage-2068471-90.patch111.64 KBdawehner
#90 interdiff.txt1.76 KBdawehner
#88 interdiff.txt3.77 KBdawehner
#88 htmlpage-2068471-88.patch111.28 KBdawehner
#86 htmlpage-2068471-86.patch110.84 KBdawehner
#83 interdiff.txt4.26 KBdawehner
#83 htmlpage-2068471-83.patch110.18 KBdawehner
#80 htmlpage-2068471-80.patch110.83 KBdawehner
#80 interdiff.txt3.47 KBdawehner
#78 htmlpage-2068471-78.patch107.37 KBdawehner
#78 interdiff.txt1.52 KBdawehner
#75 htmlpage-2068471-75.patch106.43 KBdawehner
#75 interdiff.txt4.12 KBdawehner
#72 reroll.patch105.93 KBlarowlan
#70 0001-adapt-the-translations.patch6.42 KBdawehner
#70 htmlpage-2068471-70.patch105.92 KBdawehner
#69 htmlpage-2068471-68.patch104.69 KBtim.plunkett
#61 interdiff.txt1.86 KBdawehner
#61 htmlpage-2068471-61.patch105.79 KBdawehner
#59 htmlpage-2068471-59.patch103.93 KBdawehner
#59 interdiff.txt4.08 KBdawehner
#57 htmlpage-2068471-57.patch101.08 KBdawehner
#57 interdiff.txt5.61 KBdawehner
#53 htmlpage-2068471.patch100.55 KBdawehner
#51 interdiff.txt1.69 KBdawehner
#51 htmlpage-2068471.patch101.04 KBdawehner
#49 interdiff.txt8.18 KBdawehner
#49 htmlpage-2068471-49.patch99.36 KBdawehner
#45 2068471-htmlpage.patch91.54 KBCrell
#45 interdiff.txt12.12 KBCrell
#42 2068471-htmlpage.patch90.62 KBCrell
#40 htmlpage-2068471-40.patch90.42 KBdawehner
#40 interdiff.txt5.72 KBdawehner
#38 htmlpage-2068471-38.patch87.31 KBdawehner
#38 htmlpage-2068471-38-serialize.patch97.71 KBdawehner
#36 htmlpage-2068471-36.patch87.3 KBdawehner
#36 interdiff.txt15.27 KBdawehner
#32 html_page-2068471-32.patch82.21 KBdawehner
#32 html_page-2068471-32-serialization.patch93.96 KBdawehner
#29 html_page-2068471-29.patch82.28 KBdawehner
#29 interdiff.txt1.17 KBdawehner
#29 html_page-2068471-29-serialization.patch94.61 KBdawehner
#27 html_page-2068471-27-serialization.patch94.57 KBdawehner
#27 interdiff.txt7.13 KBdawehner
#27 html_page-2068471-27.patch82.24 KBdawehner
#25 2068471-htmlpage.patch77.38 KBCrell
#23 2068471-htmlpage.patch78.58 KBCrell
#23 interdiff.txt13.52 KBCrell
#21 html_page-2068471-21.patch64.62 KBdawehner
#21 interdiff.txt784 bytesdawehner
#21 ajax_controller.patch4.61 KBdawehner
#19 html_page-2068471-19.patch63.85 KBdawehner
#17 html_page-2068471-17.patch65.29 KBdawehner
#17 interdiff.txt5.98 KBdawehner
#13 2068471-htmlpage.patch61.81 KBCrell
#13 interdiff.txt1.6 KBCrell
#12 interdiff.txt11.52 KBdawehner
#8 htmlpage-2068471-8.patch61.1 KBdawehner
#4 2068471-htmlpage.patch54.09 KBCrell
#4 interdiff.txt21.32 KBCrell
#1 2068471-htmlpage.patch37.77 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

FileSize
37.77 KB

And patch. I'm fairly certain this won't pass yet, but I want to see how badly it doesn't pass. :-)

Crell’s picture

Status: Active » Needs review

Oh yeah, that...

Anonymous’s picture

Issue summary: View changes

Add link to sandbox.

Crell’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI
FileSize
21.32 KB
54.09 KB

After 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!

Crell’s picture

Issue summary: View changes

Add more related issues.

tstoeckler’s picture

Status: Needs work » Needs review

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. :-)

Per #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.

Crell’s picture

At 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*

dawehner’s picture

FileSize
61.1 KB

Note: 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.

Damien Tournoud’s picture

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.

I 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 in KernelEvents::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:

-class ExceptionController extends ContainerAware {
+class ExceptionController extends HtmlControllerBase implements ContainerAwareInterface {

You should be able to render an exception to HTML, AJAX, Dialog and JSON.

Crell’s picture

As 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.

dawehner’s picture

FileSize
11.52 KB

Posting an interdiff from #4 to #8

Crell’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
61.81 KB

OK, 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.

sdboyer’s picture

+++ b/core/lib/Drupal/Core/Page/HtmlPage.php
@@ -0,0 +1,212 @@
+class HtmlPage extends HtmlFragment {

i 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() and page_top and page_bottom, but those can come...later?

dawehner’s picture

The overlay fails are because it alters both the full page, by adding an additional theme wrapper, but also changing the page_top region manually.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.98 KB
65.29 KB

It seemed to be that #13 runned into a broken testrun, with broken configuration directories etc.

dawehner’s picture

Status: Needs work » Needs review
FileSize
63.85 KB

HEre is a rebased patch (@crell The rebased branch is not pushed yet).

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
784 bytes
64.62 KB

Some 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.

Crell’s picture

Status: Needs work » Needs review
FileSize
13.52 KB
78.58 KB

More cleanup by both dawehner and myself.

Crell’s picture

Status: Needs work » Needs review
FileSize
77.38 KB

OK, let's try rebasing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
82.24 KB
7.13 KB
94.57 KB

This 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
94.61 KB
1.17 KB
82.28 KB

So, realized that this was a PHP 5.4 syntax.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ExceptionControllerTest.php
@@ -31,7 +33,8 @@ public static function getInfo() {
-    $exception_controller = new ExceptionController(new ContentNegotiation());
+    $key_value_factory = new KeyValueMemoryFactory();
+    $exception_controller = new ExceptionController(new ContentNegotiation(), new LanguageManager($key_value_factory->get('state')));

Why bother with the factory and not just do new MemoryStorage('state')?

dawehner’s picture

Status: Needs work » Needs review
FileSize
93.96 KB
82.21 KB

Good point berdir! Sometimes you just think to complex.

Rebased against 8.x

dawehner’s picture

While 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.

Crell’s picture

*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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.27 KB
87.3 KB

Let's see how much is broken now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
97.71 KB
87.31 KB

Just another rerole, one with and one without the serialization patch.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
90.42 KB

dawehner--

Crell’s picture

Status: Needs work » Needs review
FileSize
90.62 KB

Rebased 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).

Crell’s picture

Note 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.

Crell’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
91.54 KB

I 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.

dawehner’s picture

Status: Needs work » Needs review

#45: 2068471-htmlpage.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
99.36 KB
8.18 KB

A couple of more fix and a specific attribute to be able to set the statuscode on the exception subrequest.

dawehner’s picture

Status: Needs work » Needs review
FileSize
101.04 KB
1.69 KB

This patch is not committed, just try to figure out whether this fixes the issue. (might be a php 5.3 thing)

dawehner’s picture

Status: Needs work » Needs review
FileSize
100.55 KB

Damnit.

pwolanin’s picture

on404Html() is setting _exception_statuscode to 403

Crell’s picture

dawehner: 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.61 KB
101.08 KB

Ha, who ever guessed that drupal would be bugfree, see #2083941: \Drupal\Core\Theme\Attribute->value() is named wrong and does not work

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
103.93 KB

A couple of fixes later.

dawehner’s picture

Issue summary: View changes

Add description of form interstitial et al.

dawehner’s picture

Status: Needs work » Needs review
FileSize
105.79 KB
1.86 KB

The remaining ones will really be fun. No real idea yet.

Crell’s picture

Status: Needs work » Needs review

Setting 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!)

disasm’s picture

I have not reviewed the patch, but in reviewing the issue summary, I think this makes a lot of sense. +1 here.

catch’s picture

+++ b/core/lib/Drupal/Core/Controller/AjaxController.php
@@ -30,50 +49,64 @@ class AjaxController extends ContainerAware {
+    $html = drupal_render($content);

I 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.

Crell’s picture

Yes, 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.

sdboyer’s picture

yeah, 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.

dawehner’s picture

Title: Normalize Controller/View-listener behavior with a Page object » Normalize Controller/View-listener behavior with a Page objectgg

My rerole skills are not enough anymore to rerole that branch.

tim.plunkett’s picture

Title: Normalize Controller/View-listener behavior with a Page objectgg » Normalize Controller/View-listener behavior with a Page object
FileSize
104.69 KB

I did my very best to reroll this. The branch was completely un-rebase-able.

Not yet pushed to any new branches.

dawehner’s picture

Fixed some of the translations and pushed to 2068471-htmlpage-2 branch.

Anonymous’s picture

FileSize
105.93 KB

rerolled but can't run batch so can't test locally, too late in the day to start looking at that

larowlan’s picture

Status: Needs work » Needs review
dawehner’s picture

FileSize
4.12 KB
106.43 KB

@Larowlan ... we are using the 2068417-htmlpage branch of the wscci sandbox, ... anyway this rerole was easy.

Some fixes included.

dawehner’s picture

Status: Needs work » Needs review

.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
107.37 KB

A pair of fixes.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
110.83 KB

Back to the 4 failures of before.

andypost’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/common.inc
    @@ -3651,10 +3662,18 @@ function drupal_pre_render_dropbutton($element) {
    -function drupal_render_page($page) {
    +function drupal_prepare_page($page, $legacy = FALSE) {
    
    @@ -3667,7 +3686,7 @@ function drupal_render_page($page) {
    -    $page = element_info('page');
    +    $page = $legacy ? element_info('page') : element_info('page');
    

    any reason for?

  2. +++ b/core/includes/theme.inc
    @@ -2566,6 +2551,12 @@ function template_preprocess_html(&$variables) {
    +  // @todo Add the metatags via drupal_add_html_head for now, so the alter hook
    +  //   in drupal_get_html_head() still works.
    ...
    +//  $page->addMetatag(new Metatag('MobileOptimized', 'width'));
    +//  $page->addMetatag(new Metatag('HandheldFriendly', 'true'));
    +//  $page->addMetatag(new Metatag('viewport', 'width=device-width'));
    

    removed?

dawehner’s picture

Good first hunk!

The reason why I decided to keep the second hunk was that these lines should actually be used in the future.

dawehner’s picture

Status: Needs work » Needs review
FileSize
110.18 KB
4.26 KB

Down to one.

larowlan’s picture

please don't give me any commit credit for that re-roll above

dawehner’s picture

Status: Needs work » Needs review
FileSize
110.84 KB

Let there be another reroll.

dawehner’s picture

Status: Needs work » Needs review
FileSize
111.28 KB
3.77 KB

Another reroll failure.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
111.64 KB

bla.

fubhy’s picture

Status: Needs work » Needs review
FileSize
111.8 KB

Re-roll

dawehner’s picture

Status: Needs work » Needs review
FileSize
111.68 KB

Another saturday another rerole and another new branch: 2068471-htmlpage-4 ...

Dave Reid’s picture

The 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...

<meta name="[name]" content="[value]" />
<meta http-equiv="[name]" content="[value]" />
<meta property="[name]" content="[value]" />
<link rel="[name]" href="[value]" />
<link rev="[name]" href="[value]" />
Crell’s picture

Status: Needs work » Needs review
FileSize
25.1 KB
116.19 KB

OK, 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... :-)

Crell’s picture

Status: Needs work » Needs review
FileSize
116.19 KB

Would you people please stop improving Drupal while I'm trying to roll patches?

dawehner’s picture

Status: Needs work » Needs review
FileSize
114.02 KB

Another day another rerole. Hopefully this will finally show test failures.

sdboyer’s picture

Would you people please stop improving Drupal while I'm trying to roll patches?

optimism! :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 ...

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -0,0 +1,75 @@
+  static function getSubscribedEvents() {
+    $events[KernelEvents::VIEW][] = array('onHtmlFragment', 100);
+    $events[KernelEvents::VIEW][] = array('onHtmlPage', 50);
+
+    return $events;
+  }

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
115.64 KB

Let's see.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
117.06 KB

And more.

dawehner’s picture

The rest of the failures is pretty interesting:

  • Drupal\comment\Tests\CommentPreviewTest:
    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.
  • Drupal\field_ui\Tests\FieldUIRouteTest:
    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.
  • Drupal\locale\Tests\LocaleImportFunctionalTest:
    Problem caused by a broken batch process
  • Drupal\system\Tests\Menu\TrailTest
  • Drupal\user\Tests\UserCancelTest:
    Problem caused by a broken batch process
tim.plunkett’s picture

The FieldUIRouteTest test is actually just wrong, see #210212: form date type: bug when two date elements in one form

tstoeckler’s picture

Re #109 I think you linked the wrong issue?!

dawehner’s picture

Nice! Here is the link tim was actually linking to: #2102125: Big Local Task Conversion

Crell’s picture

Talked 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
125.18 KB
dawehner’s picture

Issue summary: View changes

Update for recent code evolution.

cosmicdreams’s picture

Hey 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.

dawehner’s picture

Issue tags: +Needs reroll

****

jibran’s picture

While @dawehner is on the train. A patch from @dawehner.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.21 KB

Let's try this one.

cosmicdreams’s picture

Status: Needs work » Needs review

#118: 2068471.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.3 KB

Let's see.

dawehner’s picture

Status: Needs work » Needs review
FileSize
126.84 KB

Many of them cannot be reproduced.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
126.91 KB

The 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.

dawehner’s picture

Issue summary: View changes

Add related issue.

sdboyer’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

yes!

i reviewed before the D7 upgrade, and all i really had were nits.

  1. +++ b/core/includes/theme.inc
    @@ -2505,13 +2509,22 @@ function _template_preprocess_default_variables() {
    +  $variables['attributes'] = $page->getAttributes();
    +  $variables['page'] = $page->getContent();
    +
       // Compile a list of classes that are going to be applied to the body element.
       // This allows advanced theming based on context (home page, node of certain type, etc.).
    -  $variables['attributes']['class'][] = 'html';
    +  $body_classes = $variables['attributes']['class'];
    +  $body_classes[] = 'html';
       // Add a class that tells us whether we're on the front page or not.
    -  $variables['attributes']['class'][] = $variables['is_front'] ? 'front' : 'not-front';
    +  $body_classes[] = $variables['is_front'] ? 'front' : 'not-front';
       // Add a class that tells us whether the page is viewed by an authenticated user or not.
    -  $variables['attributes']['class'][] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $body_classes[] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $variables['attributes']['class'] = $body_classes;
    

    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.

  2. +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,100 @@
    +    // At the form and form_state to trick the getArguments method of the
    

    nit: s/At/Add/

i may have missed other small things, but i'd say this is good enough to go.

katbailey’s picture

Just noticed one thing: the HtmlPageController no longer uses the HttpKernel so it can be removed as a constructor param.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

I decided to move this issue back to needs review given that the batch still don't work as before.

webchick’s picture

Issue tags: +alpha target

I 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.

Crell’s picture

FileSize
126.24 KB

Attempted 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.61 KB
35.49 KB
7.79 KB
130.16 KB

Fixed 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:
image

I tried to compare the previous html with the current one, a couple of points:

  • The order of css/js has changed. This is probably caused by the other order of the drupal_render() calls. Previously you
    had the big page render array of dooom, while now you render the page specific asserts earlier, which probably causes the different order.
  • Some metatags looks like . I fixed the Attribute objects and added a test to ensure that you can have nullable values
  • The views library 'views.contextual-links' is not added anymore to the page, which could also cause the odd placement here.
    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:
      if (!empty($variables['page']['#views_contextual_links'])) {
        $key = array_search('contextual-region', $variables['attributes']['class'] instanceof AttributeArray ? $variables['attributes']['class']->value() : $variables['attributes']['class']);
        if ($key !== FALSE) {
          unset($variables['attributes']['class'][$key]);
          $variables['attributes']['data-views-page-contextual-id'] = $variables['title_suffix']['contextual_links']['#id'];
          drupal_add_library('views', 'views.contextual-links');
        }
      }
    

    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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
130.08 KB

Ha, this was indeed my fault.

tstoeckler’s picture

Hmm... I can't reproduce the screenshot at least not on simplytest.me
Attached is a screenshot with FF25 on Ubuntu.

tim.plunkett’s picture

135: htmlpage-2068471.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
129.25 KB

Just another ordinary reroll.

dawehner’s picture

Status: Needs work » Needs review
FileSize
469 bytes
129.73 KB

Ha, this fix was easy.

This is just a normal reroll.

dawehner’s picture

Status: Needs work » Needs review
FileSize
129.77 KB

There we go.

dawehner’s picture

Things 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:

  if (!empty($variables['page']['sidebar_first']) && !empty($variables['page']['sidebar_second'])) {
    $variables['attributes']['class'][] = 'two-sidebars';
  }
  elseif (!empty($variables['page']['sidebar_first'])) {
    $variables['attributes']['class'][] = 'one-sidebar';
    $variables['attributes']['class'][] = 'sidebar-first';
  }
  elseif (!empty($variables['page']['sidebar_second'])) {
    $variables['attributes']['class'][] = 'one-sidebar';
    $variables['attributes']['class'][] = 'sidebar-second';
  }
  else {
    $variables['attributes']['class'][] = 'no-sidebars';
  }

  if (!empty($variables['page']['featured'])) {
    $variables['attributes']['class'][] = 'featured';
  }

  if (!empty($variables['page']['triptych_first'])
    || !empty($variables['page']['triptych_middle'])
    || !empty($variables['page']['triptych_last'])) {
    $variables['attributes']['class'][] = 'triptych';
  }

  if (!empty($variables['page']['footer_firstcolumn'])
    || !empty($variables['page']['footer_secondcolumn'])
    || !empty($variables['page']['footer_thirdcolumn'])
    || !empty($variables['page']['footer_fourthcolumn'])) {
    $variables['attributes']['class'][] = 'footer-columns';
  }

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.

tim.plunkett’s picture

The 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.

Jeff Burnz’s picture

@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.

dawehner’s picture

FileSize
131.31 KB

@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.

Oh great, thank you!

This is just another rerole after theme callback, synthetic request, current user and more.

dawehner’s picture

Status: Needs work » Needs review

147: htmlpage-2068471.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review

147: htmlpage-2068471.patch queued for re-testing.

andypost’s picture

Status: Needs work » Needs review

147: htmlpage-2068471.patch queued for re-testing.

andypost’s picture

@dawehner please re-roll the patch and does it still in https://drupal.org/node/1260830/git-instructions/2068471-htmlpage

dawehner’s picture

Status: Needs work » Needs review
FileSize
130.88 KB

Just another normal rerole.

Crell’s picture

Issue tags: +beta blocker
dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
161.91 KB

The 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.

dawehner’s picture

Status: Needs work » Needs review

160: htmlpage-2068471.patch queued for re-testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
162.74 KB
3.35 KB

Here 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).

dawehner’s picture

FileSize
131.7 KB

The 30 additional KB have been caused by some merge conflict in overlay.

yched’s picture

Patch is green, so I assume the batch fails are JS only ?
Are there specific steps to reproduce ?

Crell’s picture

Do 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.

tim.plunkett’s picture

I'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?

  1. +++ b/core/includes/theme.inc
    @@ -2195,13 +2201,22 @@ function _template_preprocess_default_variables() {
       $language_interface = language(Language::TYPE_INTERFACE);
    
    @@ -2218,6 +2233,7 @@ function template_preprocess_html(&$variables) {
    +  $language_interface = \Drupal::service('language_manager')->getLanguage();
    

    Why is this declared twice?

  2. +++ b/core/includes/theme.inc
    @@ -2195,13 +2201,22 @@ function _template_preprocess_default_variables() {
    -  $variables['attributes']['class'][] = 'html';
    +  $body_classes = $variables['attributes']['class'];
    +  $body_classes[] = 'html';
       // Add a class that tells us whether we're on the front page or not.
    -  $variables['attributes']['class'][] = $variables['is_front'] ? 'front' : 'not-front';
    +  $body_classes[] = $variables['is_front'] ? 'front' : 'not-front';
       // Add a class that tells us whether the page is viewed by an authenticated user or not.
    -  $variables['attributes']['class'][] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $body_classes[] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $variables['attributes']['class'] = $body_classes;
    

    I keep staring at this, but can't tell what the actual change is here.

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -354,11 +354,12 @@ public function submit(array $form, array &$form_state) {
         $form_state['comment_preview']['#title'] = $this->t('Preview comment');
    ...
    +    $form_state['comment_preview']['#title'] = $this->t('Preview comment');
    

    Dupe

tim.plunkett’s picture

Here 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

tim.plunkett’s picture

This fixes it for me.

tim.plunkett’s picture

Not for installing though, clearly.

Needs

diff --git a/core/includes/batch.inc b/core/includes/batch.inc
index c4aab84..afa87bf 100644
--- a/core/includes/batch.inc
+++ b/core/includes/batch.inc
@@ -149,7 +149,7 @@ function _batch_progress_page() {
 
   $url = url($batch['url'], $batch['url_options']);
 
-  return array(
+  $build = array(
     '#theme' => 'progress_bar',
     '#percent' => $percentage,
     '#message' => $message,
@@ -187,6 +187,7 @@ function _batch_progress_page() {
       ),
     ),
   );
+  return drupal_render($build);
 }
 
 /**

to install, didn't have a chance to test that with simpletest yet, gotta run.

tim.plunkett’s picture

FileSize
134.06 KB

Guess I should have just posted that, it seems to work locally.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
132.89 KB
34.88 KB

Here 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.

Crell’s picture

The interdiff from #176 looks fine to me. Thanks, Tim! Now to figure out why testbot changed its mind about the patch... *sigh*

tim.plunkett’s picture

FileSize
130.52 KB
2.37 KB

I 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.

The last submitted patch, 179: page-2068471-179.patch, failed testing.

tim.plunkett’s picture

179: page-2068471-179.patch queued for re-testing.

dawehner’s picture

Just pushed a new rebase + some cleanups to the branch, though that obviously contains still the batch "fix".

yched’s picture

Agreed 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)

dawehner’s picture

FileSize
2.17 KB
131.7 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, 184: htmlpage-2068471.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
133.8 KB
5.22 KB

And this time with the proper patch and interdiff.

The last submitted patch, 186: htmlpage-2068471.patch, failed testing.

The last submitted patch, 186: htmlpage-2068471.patch, failed testing.

dawehner’s picture

FileSize
133.12 KB

Give us our daily rerole.

tim.plunkett’s picture

FileSize
2.92 KB
133.75 KB

The 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.

The last submitted patch, 189: htmlpage-2068471.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 190: page-2068471-190.patch, failed testing.

The last submitted patch, 190: page-2068471-190.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
0 bytes
133.34 KB
537 bytes

Fixing these failures...

sdboyer’s picture

Status: Needs review » Reviewed & tested by the community

go go gadget awesomehat.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK found several nitpicks and a few bits I didn't really get. The overall structure looks OK though.

  1. +++ b/core/includes/theme.inc
    @@ -2259,6 +2269,12 @@ function template_preprocess_html(&$variables) {
    +  //  $page->addMetatag(new Metatag('viewport', 'width=device-width'));
    

    These should either be uncommented or removed.

  2. +++ b/core/includes/theme.inc
    @@ -2286,17 +2302,32 @@ function template_preprocess_html(&$variables) {
    +  // @todo Decide whether we want to get drupal_get_html_head() support until
    

    'get drupal_get_html_head() support' doesn't parse for me.

  3. +++ b/core/lib/Drupal/Core/Controller/AjaxController.php
    @@ -30,50 +49,64 @@ class AjaxController extends ContainerAware {
    +  /**
    ...
    +   *
    

    This could have a better summary, it's more or less the content of the return statement.

  4. +++ b/core/lib/Drupal/Core/Controller/AjaxController.php
    @@ -30,50 +49,64 @@ class AjaxController extends ContainerAware {
    +   * @return array
    +   *   The render array that results from invoking the controller.
    

    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.

  5. +++ b/core/lib/Drupal/Core/Controller/DialogController.php
    @@ -35,112 +36,123 @@ class DialogController {
    +  /**
    +   * Returns the result of invoking the sub-controller.
    +   *
    +   * @param \Symfony\Component\HttpFoundation\Request $request
    +   *   The request object.
    +   * @param mixed $controller_definition
    +   *   A controller definition string, or a callable object/closure.
    +   *
    +   * @return array
    +   *   The render array that results from invoking the controller.
    +   */
    

    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.

  6. +++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
    @@ -186,14 +225,15 @@ public function on404Html(FlattenException $exception, Request $request) {
    -      drupal_set_title(t('Page not found'));
    

    Nice to see more of these going :)

  7. +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,100 @@
    +   * Returns the result of invoking the form.
    

    Invokes the form and returns the result?

  8. +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,100 @@
    +    // Using reflection, find all of the parameters needed by the form in the
    +    // request attributes, skipping $form and $form_state.
    

    This comment says 'using reflection', but I can't see reflection used here.

  9. +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,100 @@
    +    $request->attributes->remove('form_state');
    

    This looks bizarre, but I guess #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API is already open.

  10. +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,100 @@
    +    //$this->formBuilder->setRequest($request);
    

    Should either be uncommented or removed.

  11. +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
    @@ -76,38 +50,32 @@ public function __construct(HttpKernelInterface $kernel, ControllerResolverInter
    +   *   The render array that results from invoking the controller.
    

    ibid.

  12. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -18,38 +21,57 @@
    +  protected $formBuilder;
    

    Why does the entity route enhancer need the form builder?

  13. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -18,38 +21,57 @@
    -        $defaults['_controller'] = '\Drupal\Core\Entity\HtmlEntityFormController::content';
    +        $wrapper = new HtmlEntityFormController($this->resolver, $this->manager, $this->formBuilder, $defaults['_entity_form']);
    +        $defaults['_content'] = array($wrapper, 'getContentResult');
    

    Oh this is why :)

  14. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
    @@ -0,0 +1,81 @@
    +      $response = new Response((string) $this->renderer->renderPage($page), $page->getStatusCode());
    

    What can $this->renderer->renderPage() return apart from string?

  15. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php
    @@ -0,0 +1,97 @@
    +    return theme('html', array('page_object' => $page));
    

    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?

  16. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -0,0 +1,219 @@
    +  protected $links = array();
    

    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.

  17. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -0,0 +1,219 @@
    +  public function addLink(Link $link) {
    

    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.

  18. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -0,0 +1,219 @@
    +  public function getAttributes() {
    

    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.

  19. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -0,0 +1,219 @@
    +  public function getContentTop() {
    

    This sounds like the content top region, but surely it's not that?

  20. +++ b/core/lib/Drupal/Core/Page/HtmlPageRendererInterface.php
    @@ -0,0 +1,49 @@
    +   * some attached information (assets, metatags, etc.) An HtmlPage represents
    

    Does it really contain assets? That was removed I thought.

  21. +++ b/core/lib/Drupal/Core/Utility/Title.php
    @@ -17,4 +17,9 @@ class Title {
    +   * For for controller titles, for sanitizing via Xss::filterAdmin.
    

    For for.

  22. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -128,6 +128,8 @@ protected function setUp() {
    +    drupal_get_schema(NULL, TRUE);
    

    How come this is necessary for test passes, seems unrelated to the rest of the patch? Could use a comment.

  23. +++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.php
    @@ -41,8 +87,34 @@ public function batchPage(Request $request) {
    +    $page->setContentTop(drupal_render($page_array['page_top']));
    +    $page->setContentBottom(drupal_render($page_array['page_bottom']));
    +    $page->setContent(drupal_render($page_array));
    +
    

    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?

sdboyer’s picture

re: #196.16

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.

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.

Crell’s picture

Some 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.

webchick’s picture

I 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.

Crell’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
133.24 KB
12.77 KB

Point 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...

dawehner’s picture

Regarding 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.

catch’s picture

4) 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.

That at least needs a long comment in the code.

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.

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).

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.

Hmm I remember another issue about this, but also not being convinced that there wasn't a point at which ga could interact already.

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.

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.

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 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.

catch’s picture

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.

OK 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.

Crell’s picture

#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.

sun’s picture

  1. +++ b/core/includes/theme.inc
    @@ -2684,12 +2711,13 @@ function drupal_common_theme() {
         'page' => array(
           'render element' => 'page',
           'template' => 'page',
    +      'theme wrapper' => 'html',
         ),
    

    Is 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?

  2. +++ b/core/modules/system/system.routing.yml
    @@ -375,12 +375,20 @@ system.admin_config:
    -system.batch_page:
    +system.batch_page.normal:
    ...
    +system.batch_page.json:
    

    It wasn't obvious what "normal" refers to.

    Could we rename "normal" to "html" (or "htmlpage")?

Crell’s picture

RE #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.

Crell’s picture

I'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.

dawehner’s picture

FileSize
511 bytes
132.92 KB

Fixed the installer

dawehner’s picture

FileSize
132.92 KB
963 bytes

Fixed #2 of sun's comment.

The last submitted patch, 208: htmlpage-2068471.patch, failed testing.

The last submitted patch, 209: htmlpage-2068471.patch, failed testing.

jibran’s picture

48 exception(s) not bad. We are real close.

dawehner’s picture

FileSize
132.99 KB
454 bytes
3.19 KB
1.52 KB

@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.

pwolanin’s picture

diff2 looks like magic or a bad hack that will not be easy to understand in the future.

The last submitted patch, 213: htmlpage-2068471.patch, failed testing.

sun’s picture

Sure, 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:

  1. +++ b/core/core.services.yml
    @@ -387,10 +387,19 @@ services:
    +  controller.form:
    +    class: Drupal\Core\Controller\FormController
    +    arguments: ['@controller_resolver', '@service_container']
    +  controller.entityform:
    +    class: Drupal\Core\Entity\HtmlEntityFormController
    +    arguments: ['@controller_resolver', '@service_container', '@entity.manager']
    
    @@ -506,7 +523,7 @@ services:
       exception_controller:
         class: Drupal\Core\Controller\ExceptionController
    -    arguments: ['@content_negotiation']
    +    arguments: ['@content_negotiation', '@string_translation', '@title_resolver', '@html_page_renderer']
         calls:
           - [setContainer, ['@service_container']]
    

    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).

  2. +++ b/core/includes/theme.inc
    @@ -2054,15 +2056,22 @@ function _template_preprocess_default_variables() {
    -  $variables['attributes']['class'][] = 'html';
    +  $body_classes = $variables['attributes']['class'];
    +  $body_classes[] = 'html';
    ...
    +  $variables['attributes']['class'] = $body_classes;
    

    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?

  3. +++ b/core/includes/theme.inc
    @@ -2147,17 +2159,30 @@ function template_preprocess_html(&$variables) {
    +  $html_heads = drupal_get_html_head(FALSE);
    ...
    +  foreach ($html_heads as $name => $tag) {
    +    if ($tag['#tag'] == 'link') {
    +      $link = new Link($name, isset($tag['#attributes']['content']) ? $tag['#attributes']['content'] : NULL, $tag['#attributes']);
    ...
    +      $page->addLink($link);
    +    }
    +    elseif ($tag['#tag'] == 'meta') {
    +      $metatag = new Metatag(NULL, $tag['#attributes']);
    ...
    +      $page->addMetatag($metatag);
    

    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?

  4. +/**
    + * Base class for HTML page-generating controllers.
    + */
    +class HtmlControllerBase {
    
     /**
      * Wrapping controller for forms that serve as the main page body.
      */
    ...
    +class HtmlFormController extends FormController {
    
     /**
      * Default controller for most HTML pages.
      */
    ...
    +class HtmlPageController extends HtmlControllerBase {
    
    +/**
    + * Default page rendering engine.
    + */
    +class DefaultHtmlPageRenderer implements HtmlPageRendererInterface {
    
    +/**
    + * Response object that contains variables for injection into the html template.
    ...
    + */
    +class HtmlFragment {
    
    +
    +/**
    + * Data object for an HTML page.
    + */
    +class HtmlPage extends HtmlFragment {
    
    +/**
    + * Interface for HTML Page Renderers.
    + *
    + * An HTML Page Renderer is responsible for translating an HtmlFragment object
    + * into an HtmlPage object, and from a page object into a string.
    + */
    +interface HtmlPageRendererInterface {
    

    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? :)

  5. +++ b/core/lib/Drupal/Core/Page/HtmlPage.php
    @@ -0,0 +1,219 @@
    +/**
    + * Data object for an HTML page.
    + */
    +class HtmlPage extends HtmlFragment {
    

    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]).

dawehner’s picture

FileSize
136.29 KB
0 bytes
4.33 KB

I cannot remember why the change was needed (something with notices maybe). Sadly the git history is lost.

Crell’s picture

Issue summary: View changes

#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? :-)

andypost’s picture

summary now looks better, some other nitpics

  1. 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.

    +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -0,0 +1,96 @@
    +abstract class FormController {
    

    FormController as abstract now, any reason to register it in container?

  2. +++ b/core/includes/theme.inc
    @@ -2060,15 +2062,22 @@ function _template_preprocess_default_variables() {
    -  $variables['attributes']['class'][] = 'html';
    +  $body_classes = $variables['attributes']['class'];
    +  $body_classes[] = 'html';
       // Add a class that tells us whether we're on the front page or not.
    -  $variables['attributes']['class'][] = $variables['is_front'] ? 'front' : 'not-front';
    +  $body_classes[] = $variables['is_front'] ? 'front' : 'not-front';
       // Add a class that tells us whether the page is viewed by an authenticated user or not.
    -  $variables['attributes']['class'][] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $body_classes[] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
    +  $variables['attributes']['class'] = $body_classes;
    

    really unnecessary change

  3. +++ b/core/lib/Drupal/Core/Controller/AjaxController.php
    @@ -30,50 +49,65 @@ class AjaxController extends ContainerAware {
    +    if ($content instanceof HtmlPage) {
    +      $content = $content->getContent();
    +    }
    +    if ($content instanceof Response) {
    +      $content = $content->getContent();
    +    }
    ...
    +    // A page callback could return a render array or a string.
    +    if (!is_array($content)) {
    ...
    +    $html = drupal_render($content);
    ...
    +      $content = array(
    +        '#markup' => $content,
    

    seems need comment why HtmlPage could return aray that not Response

  4. +++ b/core/lib/Drupal/Core/Controller/DialogController.php
    @@ -35,112 +36,124 @@ class DialogController {
    +    $page_content = $this->getContentResult($request, $_content);
    ...
    +    if ($page_content instanceof HtmlPage) {
    ...
    +    if ($page_content instanceof Response) {
    ...
    +    if (!is_array($page_content)) {
    ...
    +    $content = drupal_render($page_content);
    

    same here

dawehner’s picture

FileSize
6.59 KB
130.61 KB

Pushed a patch without metatag/link support to 2068471-htmlpage-8-metatag

dawehner’s picture

FileSize
2.92 KB
129.79 KB

Thank you for the reviews. Fixed the points of #219.

Status: Needs review » Needs work

The last submitted patch, 221: htmlpage-2068471.patch, failed testing.

webchick’s picture

That is a truly exceptional number of exceptions, sir!

Crell’s picture

FileSize
141.16 KB
2.44 KB

The 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.

Crell’s picture

Status: Needs work » Needs review

Damnit, d.o!

dawehner’s picture

FileSize
130.56 KB
1.16 KB

Okay, this proves that the html classes change is needed at least for 5.3

dawehner’s picture

@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.

The last submitted patch, 226: htmlpage-2068471.patch, failed testing.

dawehner’s picture

226: htmlpage-2068471.patch queued for re-testing.

pwolanin’s picture

drupal_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

+  public function dialog(Request $request, $_content, $modal = FALSE) {
+    $page_content = $this->getContentResult($request, $_content);
+
+    if ($page_content instanceof HtmlPage) {
+      $page_content = $page_content->getContent();
+    }
+    if ($page_content instanceof Response) {
+      $page_content = $page_content->getContent();
+    }

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.

The last submitted patch, 226: htmlpage-2068471.patch, failed testing.

pwolanin’s picture

minor in AttributeValueBase::render()

if ($this->value !== NULL) {

Why not use isset()?

dawehner’s picture

FileSize
3.07 KB
130.41 KB

Thank you for the review.

but inside the code a callable object is not handled, only a closure.

The controller resolver takes care of the other usecases.

dawehner’s picture

FileSize
567 bytes
130.41 KB
567 bytes

Missed #232

dawehner’s picture

FileSize
130.56 KB

I predict that this won't be green again. There have been too many changes in similar regions of the code.

Status: Needs review » Needs work

The last submitted patch, 235: htmlpage-2068471.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
130.68 KB

Screwed the rerole.

Dries’s picture

I 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.

dawehner’s picture

There 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.

dawehner’s picture

FileSize
130.28 KB

A weekend would not be a weekend without a rebase.

dawehner’s picture

FileSize
130.28 KB

Keeping the patch up to date.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This is ready.
I manually tested it again, and the metatag stuff has been removed, and all other feedback has been addressed.

xjm’s picture

241: htmlpage-2068471.patch queued for re-testing.

webchick’s picture

Assigned: Crell » catch

Passing back to catch.

catch’s picture

Assigned: catch » Dries

Fine 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).

dawehner’s picture

241: htmlpage-2068471.patch queued for re-testing.

dawehner’s picture

241: htmlpage-2068471.patch queued for re-testing.

dawehner’s picture

Can we please get this in rather sooner than later given that reroles on this issue are kind of a pain.

webchick’s picture

Assigned: Dries » Unassigned

Dries 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.

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Let'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

Crell’s picture

Issue summary: View changes

Expanded that comment a bit more.

pounard’s picture

if ($controller_definition instanceof \Closure) {

is_callable() wouldn't be better there?

Crell’s picture

Without context I can't tell what you're referring to; we use that approach in a few places, including in HEAD now, I think...

pounard’s picture

It's within the last patch, code ends up with call_user_func_array() on the tested variable.

pounard’s picture

Anyway from http://php.net/manual/de/functions.anonymous.php we can read:

Anonymous functions are currently implemented using the Closure class. This is an implementation detail and should not be relied upon.

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 only time you should be typing arguments is when there is something about that particular class which you need. Even then, you are better off with use of an interface. That is just good OOP design.

The thread is actually about Closures; Considering PHP flexibility about what's callable or not, this sounds especially true in this use case.

dawehner’s picture

FileSize
130.26 KB
1.92 KB

Rerolled and fixed the three instanceof instances.

The last submitted patch, 257: htmlpage-2068471.patch, failed testing.

dsnopek’s picture

257: htmlpage-2068471.patch queued for re-testing.

The last submitted patch, 257: htmlpage-2068471.patch, failed testing.

The last submitted patch, 257: htmlpage-2068471.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -76,38 +50,32 @@ public function __construct(HttpKernelInterface $kernel, ControllerResolverInter
+    if (is_callable($controller_definition)) {
+      $callable = $controller_definition;
+    }
+    else {
+      $callable = $this->controllerResolver->getControllerFromDefinition($controller_definition);
+    }

See, 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?

dawehner’s picture

FileSize
130.28 KB

GRRR

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Issue summary: View changes
dawehner’s picture

FileSize
130.28 KB

Yet another one.

catch’s picture

Title: Normalize Controller/View-listener behavior with a Page object » Change notice: Normalize Controller/View-listener behavior with a Page object
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

So 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.

larowlan’s picture

Great job dawehner

jbrown’s picture

Title: Change notice: Normalize Controller/View-listener behavior with a Page object » Normalize Controller/View-listener behavior with a Page object
Priority: Major » Critical

This 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

Crell’s picture

Investigating...

Crell’s picture

Status: Active » Needs review
FileSize
557 bytes

Trivial 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.)

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

We also need a follow up to add test coverage for that path

jbrown’s picture

Patch in #271 fixes it for me.

Crell’s picture

larwolan: 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.

jbrown’s picture

Surely the functionality of admin/reports/status needs test coverage. It would have prevented this problem occurring.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Agreed. 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.

Crell’s picture

Not 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...

effulgentsia’s picture

Do you really want a test that just ensures a 200 response from admin/reports/status?

Yes.

Fully testing the output of that report is totally out of scope

Agreed. Normal priority follow up for that.

Not every bug is a regression. This is only a regression in the most abstract academic definition. It's just a bug.

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.

The last submitted patch, 271: status-page-quickfix.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
2.05 KB

I 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.

tstoeckler’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/System/StatusTest.php
    @@ -0,0 +1,47 @@
    +    $this->drupalLogin($this->admin_user);
    ...
    +    ));
    ...
    +      'access administration pages',
    ...
    +    $this->web_user = $this->drupalCreateUser(array(
    ...
    +    // administrative tasks, but not all of them.
    ...
    +    // who can only access administration pages and perform some Locale module
    ...
    +    // Create an administrator with all permissions, as well as a regular user
    ...
    +    $this->admin_user = $this->drupalCreateUser(array_keys(module_invoke_all('permission')));
    
    +++ b/core/modules/system/system.routing.yml
    @@ -272,7 +272,7 @@ system.theme_enable:
         _permission: 'administer site configuration'
    

    I don't see those users being used at all, the 'administer site configuration' permission should be sufficient.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/StatusTest.php
    @@ -0,0 +1,47 @@
    +    // testAdminPages() requires Locale module.
    

    Bad copy-paste?

Crell’s picture

FileSize
1.71 KB

Yeah, 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.)

dawehner’s picture

Just nitpicking.

yched’s picture

From the patch that got committed :

+++ b/core/lib/Drupal/Core/Routing/Enhancer/ContentControllerEnhancer.php
@@ -50,19 +51,17 @@ public function __construct(ContentNegotiation $negotiation) {
-    if ($request->attributes->get('dialog') && !empty($defaults['_content'])) {
-      $defaults['_controller'] = $defaults['_content'];
-    }
+    //if ($request->attributes->get('dialog') && !empty($defaults['_content'])) {
+    //  $defaults['_controller'] = $defaults['_content'];
+    //}

What's the status of this ? Is there an associated @todo ?

pounard’s picture

@#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.

dawehner’s picture

@pounard
Closures won't be used anyway, to be honest.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let'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.

webchick’s picture

Title: Normalize Controller/View-listener behavior with a Page object » Change notice: Normalize Controller/View-listener behavior with a Page object
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

I 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.

yched’s picture

#284 / #284

I think that was probably just dead code we forgot to remove. Let's remove it.

#2166059: Leftover commented-out code after "Page object" patch

ianthomas_uk’s picture

The committed patch calls drupal_container() which was recently removed. I've opened a followup: #2167517: system_custom_theme calls removed function drupal_container

jessebeach’s picture

Issue tags: +Needs change record

Added the Needs change notification tag.

jessebeach’s picture

Assigned: Unassigned » jessebeach

I'll write this change notice. I want to understand better what happened here.

jessebeach’s picture

Issue tags: -Needs change record

Change record posted: https://drupal.org/node/2170551

dawehner’s picture

Nice! 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.

jessebeach’s picture

Maybe just mention the HtmlFragment/page/block idea in a comment? Since it's possible but not implemented?

jessebeach’s picture

Title: Change notice: Normalize Controller/View-listener behavior with a Page object » Normalize Controller/View-listener behavior with a Page object
Crell’s picture

Status: Active » Fixed

Then I think we're done here. Follow-ups are happening elsewhere.

tstoeckler’s picture

jibran’s picture

Title: Normalize Controller/View-listener behavior with a Page object » Change notice: Normalize Controller/View-listener behavior with a Page object
Status: Fixed » Needs work
Issue tags: +Needs change record

Thank 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.

webchick’s picture

That 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.

jibran’s picture

Title: Change notice: Normalize Controller/View-listener behavior with a Page object » Normalize Controller/View-listener behavior with a Page object
Priority: Major » Critical
Status: Needs work » Fixed
Issue tags: -Needs change record

Thanks @webchick for the suggestion.

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'll give it a try.
Meanwhile I think there is no point in keeping this open. So back to fixed. :)

Status: Fixed » Closed (fixed)

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

ianthomas_uk’s picture

Can 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.

Wim Leers’s picture