Problem/Motivation
We are trying to remove the process layer but in doing so 'getting' all the CSS and JS in the preprocess layer will render the JS/CSS too early and later preprocess functions will need to re render the items again to re-populate the variable for the template.
Proposed resolution
Create a new RenderWrapper class that allows for later-rendering via __toString() method, and implement it to remove template_process_html() and template_process_maintenance_page().
Remaining tasks
n/a
API changes
New class that wraps:
- drupal_get_js()
- drupal_get_css()
- drupal_get_head()
drupal_get_breadcrumb()- drupal_get_title()
Related Issues
#1843650: Remove the process layer (hook_process and hook_process_HOOK)
#2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class
Comment | File | Size | Author |
---|---|---|---|
#98 | drupal-render_wrapper-2004286-98.patch | 11.18 KB | jenlampton |
#98 | interdiff.txt | 529 bytes | jenlampton |
#97 | drupal-render_wrapper-2004286-92.patch | 10.99 KB | jenlampton |
#97 | interdiff.txt | 888 bytes | jenlampton |
#95 | drupal-render_wrapper-2004286-92.patch | 10.99 KB | jenlampton |
Comments
Comment #1
dasjoalso see #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
Comment #2
joelpittetOk so I need something a bit more generic for also drupal_get_head() drupal_get_breadcrumbs, etc... so here is a generic version of the same idea.
This class just allows for late rendering of these functions.
Comment #4
joelpittet#2: late-render-css-js-2.patch queued for re-testing.
Comment #6
joelpittettypo, same as #2
Comment #7
joelpittethmm, well there was more typos in the class consolidation... that name sucks anyway. Please suggest a better class name.
Comment #9
joelpittetok slow down... this is right.
Comment #9.0
joelpittetadded related meta
Comment #10
joelpittetbetter issue title
Comment #11
mlncn CreditAttribution: mlncn commentedWithout and with the patch both have this weirdness in the page classes:
but that's not related to this patch.
And the "Does this make exceptions not work" comment in the code... i think this can be removed, though i'm not sure how to trigger the right kind of exception to test this.
With that cleanup i think this is RTBC.
Comment #12
dasjonote that this issue has duplicated #1843704: Remove template_process_html().
i think we have a problem here because page_bottom is replaced instead of being appended:
Comment #13
joelpittet@dasjo thanks for the catch, didn't see that one. Any suggestions to deal with that? Separate variable for scripts? Embed in a renderable array maybe?
Comment #14
Fabianx CreditAttribution: Fabianx commentedI suggest working more strongly with Render API here.
I think if page_bottom was an array, that:
{{ page_bottom }}
should still work, so:
might directly make this work.
The $args = array('footer') feels a little strange to me ... Was that intended?
Comment #15
joelpittetThanks @Fabianx, that was intended but may not be of Drupal coding standards, just nameing args so it's clear what they are... in this case $args is arguments so that is a bit worst than usual. I can remove that.
I tried setting it as an array and was getting undefined index 0, but probably did it wrong, so will try again because I think that's the way to go too.
Comment #16
joelpittetThis works but I am not sure I am totally happy with this... kinda wish Renderable Arrays were renderable objects, or at least RenderableArrayObjects... so this kind of thing would just work.
Anyways, here's what I have for further vetting. If anybody know's of some trick where you can add a callback to a normal renderable array that could get called at print time, that may help.
Comment #17
joelpittetOk moved to #type, probably did this wrong and still need better name for this... but this code works so worth a look see.
Comment #18
joelpittetPossible better names:
deferred_print
print_me_later
call_back_later
compile_last
wait_do_i_have_all_your_assets_yet_before_i_print
wait_for_it
hold_your_horses
ill_get_around_to_printing_this_later
Comment #20
Fabianx CreditAttribution: Fabianx commented#17: 2004286-17-renderable-wrapper.patch queued for re-testing.
Comment #21
Fabianx CreditAttribution: Fabianx commentedPasses tests, removes the process layer, is compatible with Twig initiative further goals, defers printing to later, best practice of using render arrays.
=> RTBC
PS: And I am pretty happy with a "render_callback" type, which can be internal to Drupal.
Comment #22
tim.plunkettWhat does the word "devert" mean?
Comment #23
joelpittetSomething about dropping your religion I guess. http://www.urbandictionary.com/define.php?term=devert
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedReviewed the most recent patch. render_callback sure looks to me like it complicates our render api (a little) which is not the direction we want to go.
I don't understand the problem/motovation well. Modules and themes can do whatever they want with the list of stylesheets all the way up to the bitter end, when we finally are calling theme('html'). At this point, even theme('page') has completed. Why is that not late enough? Furthermore, drupal_get_css() runs hook_css alter() so how exactly . Wy can't modules alter using the hook instead of doing so in preprocess? Which module needs to "re-populate the variable for the template"
Comment #25
joelpittet@moshe weitzman thanks for reviewing. So the problem is we are trying to get rid of the process_layer #1843650: Remove the process layer (hook_process and hook_process_HOOK), also we are not calling theme() anymore because we want to give drillable variables to the templates. All theme() calls are converted to renderable arrays and possibly renderable objects (D9) later, and right now renderable arrays are being made drillable inside the twig templates. So if I pass $variables['node'] = $node; I can get {{ node.fieldname }} printed in a template for example.
This patch is a stopgap because there are issues to remove drupal_add_js() and all the rest and it will probably get dropped out when that stuff is #scotched up. But in the meantime I am looking for a way to drop that process layer and this solves that. Preprocess functions adds all the assets (js/css etc) and Process renders their array into a unchangable string. We are going for {{ styles }} and the "possibility" to {{ styles.overlayLibraryNameOrWhatever }}
Not sure on the hook_css() bit, maybe that's another way to do what I am trying to accomplish? Could you try this theory out on this? #1843704: Remove template_process_html() which was closed as duplicate from this issue solving it.
Comment #26
joelpittetWould totally prefer this to be an object like I originally did it, but bigger picture all the "types" from hook_element_info() would become renderable objects, which inherit a renderable interface. So this kinda just doesn't jump the gun to early and lines the ducks up in a row;)
Comment #27
Fabianx CreditAttribution: Fabianx commented#24: I can see where you are coming from. The render_callback is very abstract, though also very powerful in deferring any method to render later.
However: It is just a #type and possibly only used internally.
As a compromise would having:
#type => 'css', '#type' => 'js', '#type' => 'html_head' be more acceptable to you?
Thanks for your feedback!
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedWould it be possible to not declare a #type of render_callback and instead do #pre_render => 'drupal_get_css' or some wrapper around drupal_get_css(). In this case, I don't think the layer of indirection is helpful.
Comment #29
Fabianx CreditAttribution: Fabianx commentedI totally agree with that. We are trying it with #pre_render only now :).
Comment #30
joelpittetOk realized my #type conversion and even just #pre_render without the #type are getting called before #attached libraries get processed and the toolbars don't load. So those don't work, I could be doing it wrong but the more I try, the further I get from keeping this simple. My first approach with the class, works. Please have a look at #16
Comment #31
joelpittetComment #32
joelpittetSame as #16 with new name(welcome to gave name suggestions if that helps).
Comment #33
star-szrNice work on this @joelpittet! Documentation nitpickery ahead:
Not sure this comment is needed, I vote we remove it.
This should be 'Contains \Drupal\Core…' per https://drupal.org/node/1354#file, the standard for this changed.
I think the 'Renderable wrapper' comment can be removed.
I think the summary line needs to be more than just 'Constructor'.
'The callback function name' - capitalize 'the'.
Also, extra space between array and $args - ref. https://drupal.org/node/1354#param.
Capitalize 'the' after the datatype.
'Calls' per http://drupal.org/node/1354#functions.
Comment #34
joelpittetThanks @Cottser, attached is the changes from #33
Comment #35
star-szrLooking much better, a few more suggestions:
Remove extra line between namespace and class docblock.
If we're using Twig in the code sample there should be no semicolon here.
I know this is just a code sample, but I think doc standards still apply here, so the 'Produces' comment should be a complete sentence.
No wonder this looked funny to me earlier, the descriptions need to be on separate lines - see https://drupal.org/node/1354#param and https://drupal.org/node/1354#return.
Comment #36
joelpittetThanks again. I removed the extra cruft from the code sample including that comment.
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedI think this is convoluted but I'm not gong to stop it.
Why do we have
$variables['css'] = drupal_add_css();
and $variables['css']. Are they each useful in different ways. If not, we should remove one.Comment #38
joelpittet@moshe weitzman I think it's a leftover from some other change, it's never used in the template as it's an array of css classes, possibly for others to manipulate later but still not nessasary. We can probably remove it here, thanks for spotting that, I squinted at it for a while too.
I wish I was better at the theme layer API and could work in some magic that would make everybody happy but this is the most straight forward solution to the problem I could think of...
Comment #39
Fabianx CreditAttribution: Fabianx commentedI re-reviewed it again.
When the css variable is removed this is RTBC in my opinion.
Comment #40
azinoman CreditAttribution: azinoman commentedI applied the patch but the styles for the tool bar seem like they're missing.
Comment #41
jenlamptonIt looks like toolbar is adding it's assets via
hook_library_info()
and those assets are not added to the page after this patch is applied. Assets added that way are attached to the page indrupal_process_attached
. Alternatively, field module adds it's assets via hook_page_build, and those seem to work just fine. We may be running into a separate issue of "too many ways to solve the same problem" here. But anyway...The problem here was
$variables['page'] = $variables['page']['#attached'];
which needed to be$variables['page'] = drupal_render($variables['page']);
instead.New patch attached.
edit: stupid upload module, will try to cancel tests on bad patches.
Comment #43
jenlamptonThis one failing test should pass as soon after the following issue gets in
#2003814: Remove Bartik process layer functions, refactor color module alterations
Marking on postponed for now.
Comment #43.0
jenlamptonAdded related / duplicate issue.
Comment #43.1
joelpittetUpdated issue summary.
Comment #44
jenlamptonunblocked now that #2003814: Remove Bartik process layer functions, refactor color module alterations is committed :)
Probably needs a reroll.
Comment #45
jenlampton#41: render-wrapper-2004286-41.patch queued for re-testing.
Comment #47
jenlamptontagging for later today
Comment #48
jenlamptonokay, let's try this again.
Comment #49
star-szrYay green! Here's a review of things that stood out to me, otherwise this is looking very good I'd say!
This should just be
new RenderWrapper('drupal_get_css', array($css));
- the$args =
is unnecessary.I believe the standards for @code used to be to indent by two spaces but now there is no indent, ref. https://drupal.org/node/1354#code.
Typo: constructor.
Just some outdated docs here I think. If the callback can be anything (not just drupal_get_* functions) then we shouldn't refer to drupal_get_* functions here.
I think the if line needs to get indented one more space and that should line things up.
Also, there should be a blank line between the last method and the end of the class.
Comment #50
jenlamptonokay, quick reroll here just to keep things moving. I took care of everything except #4, because for now the only place we are using these us around get_* functions, so I don't know what to put here instead... how's this?
Looks like I forgot...
Comment #52
jenlampton#50: twig-render_wrapper-2004286-50.patch queued for re-testing.
Comment #54
jenlamptonbah, okay, trying again.
Comment #55
joelpittet@jenlampton @Cottser thank you for all the changes on that one.
Comment #55.0
joelpittetupdate dupe issue
Comment #56
siccababes CreditAttribution: siccababes commentedI tested this patch and it appears to be just fine. After I applied the patch, I reset drupal and went through the installation process again. I installed drupal without any issues. This patch works fine.
Comment #57
star-szrThis needs a reroll and a small docs tweak, otherwise this looks good to me.
This should be changed to @var string, see the constructor docblock.
Comment #58
LinL CreditAttribution: LinL commentedRe-rolled.
Comment #60
star-szrThanks very much @LinL!
I'm not sure if this is the right way to solve this but this should install at least (passed manual installer testing locally). Removes template_process_install_page() and adds a couple lines to seven_preprocess_install_page(). I'm not sure if we should be trying to turn 'styles' and 'scripts' back into RenderWrapper objects in seven_preprocess_install_page() - I'm guessing eventually this stuff will all be #attached anyway, see #1969916: Remove drupal_add_js/css from seven.theme and the meta at #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff.
Comment #61
star-szrRe-titling, this really needs a review so we can remove the process layer.
Comment #62
jenlamptondibbs!
Comment #63
jenlamptonI think it's fine to use RenderWrapper objects in seven for now so we can get rid of process before July 1. The cleanup will come naturally when things are updated to use #attached :)
Installer works great, everything else hunky dorey! :)
Comment #64
jenlamptonyah
Comment #64.0
jenlamptonremove related
Comment #65
star-szrTalked to @alexpott and this should have test coverage, I'm on it.
Comment #66
star-szrAlso, per @alexpott on IRC:
Comment #66.0
star-szrUpdate summary to reflect latest patch
Comment #67
star-szrChanges:
Comment #68
c4rl CreditAttribution: c4rl commentedI know this is just a stopgap, but at DrupalCamp Colorado today we were talking about the naming and architecture of this thing. RenderWrapper is pretty ambiguous, and the fact that it takes a callback as an argument seems like bad design to me.
What about something like:
Comment #68.0
c4rl CreditAttribution: c4rl commentedCode block -> list
Comment #69
jenlamptonSince we need to get this in *today* I propose the patch as-is, and then a follow-up to clean up our classes, as stated in #68 :)
Comment #70
star-szrdrupal_add_js() and drupal_add_css() are deprecated: #2028415: Mark drupal_add_*() deprecated
If drupal_get_head() and drupal_get_title() are the only things using this class in a month or three, maybe we can just ditch this class and rework those. Things are steadily heading in this direction but they are just not there yet. This class is kind of like a polyfill for late rendering :)
So longer term I do see this going away, but I see it as a necessary piece in removing the process layer in Drupal 8.
Edit: I also worry that adding heavier architecture will make things slow, I'm looking at you Attribute.
Comment #71
jenlamptonokay, well I reviewed the new patch (with tests!) and it still works great :)
I've also created a follow-up issue, so we don't forget to pay back our technical debt in 3 months: #2031869: Rename RenderWrapper, and turn temporary shiv into a more elegant solution (if necessary)
Comment #72
star-szrSimpler - don't need to throw an exception for non-array arguments. Just type-hint and PHP handles it. Thanks @alexpott :)
Comment #73
star-szrPHPUnit awesomeness - I am definitely a fan. Thanks again goes to @alexpott :)
Comment #74
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #75
alexpottCommitted 5aced02 and pushed to 8.x. Thanks!
Perhaps the Twig change notice needs updating to reflect the functionality this gives us... https://drupal.org/node/1831138
Comment #76
jessebeach CreditAttribution: jessebeach commentedWe're seeing the gray messages bar in appear on the page when no messages exist.
I ran a git bisect and narrowed the introduction of the regression to the commit of this issue. Not entirely sure why it's happening yet. I'll open a bug.
Comment #77
nod_note that it's a good side effect, I'd need to have the message area always available to fix this #77245: Provide a common API for displaying JavaScript messages.
Comment #78
jessebeach CreditAttribution: jessebeach commentedThe issue noted in #76: #2033359: Messages area is rendered when no messages are present..
Comment #79
jessebeach CreditAttribution: jessebeach commentednod_, re: #77, maybe we just need the right JS/CSS then to hide the messages area when it's empty?
Comment #80
Wim Leers#76 is an easy fix.
However, #2033275: Contextual links broken because of JS error, which is another side-effect of this issue, is not an easy fix AFAICT.
In my opinion, this patch should be reverted until it finds a solution, and frankly, until it is properly tested. No manual testing happened here, which is essential in this case because subtle JS problems can arise because of it, and Drupal core sadly does not have JS testing coverage yet.
See #2033275-6: Contextual links broken because of JS error for a detailed explanation of the deeper problems.
http://buytaert.net/drupal-8-api-freeze
Comment #81
catchOn #2033275: Contextual links broken because of JS error Wim suggested rolling this back. Since there's two unintended side-effects including one critical I think that's a good idea.
Comment #82
alexpottI agree lets roll this back until we have a solution for #2033275-6: Contextual links broken because of JS error...
Committed 3d521d9 and pushed to 8.x. Sorry...
Comment #83
star-szrSorry folks :(
Comment #84
Wim LeersYes, I'm sorry too :( Let me know if I can be of help with figuring out a way to achieve the goal of this issue without the side-effects.
(I know that Views' bizarre usage of contextual links can be tricky — I got bitten by it too in another patch, but it's the only way it can do what it needs to do. If you find an easier way to do that, then maybe you can completely side-step the side effect at #2033275: Contextual links broken because of JS error.)
Comment #85
joelpittetThis seems to be part of this issue and didn't have that in when I was testing things out.
Any premature rendering of variables will cause trouble. The idea behind this issue is to render what used to be rendered in process layer into the template printing(ie way later), leaving the variables intact until that point.
This may have been in for a reason though... so removing that should work but also solving what it was intending to solve may also need to be done. And writing another test for this would help too.
Comment #86
joelpittetSo yeah don't need drupal_render($variables['page']);
Was hoping to get rid of the ones for 'page_top' and 'page_bottom' but it's a different kind of chicken and egg thing.
Because {{ page }} contains {{ page.page_top }} {{ page.page_bottom }} it will try to render them as it's children. That's why they are moved in the process layer to the root level variable. Moving them early makes the assumption that no changes to $variables['page']['page_top'] will occur and all further changes are on $variables['page_top']. But because it's really a 'special' region that's hidden from the blocks UI, I wonder if that assumption would cause issues?
Anyways here's the patch with the line above removed. This fixes the contextual filters issue.
Comment #87
star-szrWorking on tests to avoid the bug reported in #2033275: Contextual links broken because of JS error from recurring.
Comment #88
joelpittet@jenlampton could you have a look at #86 to make sure it doesn't break what you were fixing in #41?
Comment #89
jenlamptonSure giving it a little test. The missing assets from toolbar are easy to spot since the entire menu becomes a unstyled list... and... it's looking good! :)
Comment #90
star-szrWorking on the test coverage still.
Comment #91
star-szrThis is still on my radar. Will likely happen Thursday or early next week.
Comment #92
star-szrHere's a test for the condition that caused the bug in #2033275: Contextual links broken because of JS error (the page variable being rendered/flattened early).
The first patch is the new test along with the previously committed patch from #73.
Comment #93
jenlamptonNice! :) Patch still looks good, new tests passing. Assets, messages, titles, and breadcrumbs still appearing as normal. Code looks great! back to RTBC :)
Comment #94
alexpottLets wrap these in the render wrapper too... like so :)
Comment #95
jenlamptonyah, agree.
Comment #97
jenlamptontrying again (thanks @cottser!)
Comment #98
jenlamptontrying again (thanks @cottser!)
Comment #99
markhalliwellReviewed with Dreditor, patch passes!
Comment #100
alexpottCommitted 8b4fa68 and pushed to 8.x. Thanks!
Comment #101
star-szrI was going to add details about this class to the process layer change notice (https://drupal.org/node/2038981) but I thought this probably deserves its own change notice, feedback please before I post:
RenderWrapper class added to postpone rendering of variables until printed in the template
A RenderWrapper helper class has been added so that function calls can be deferred until they are printed in a template. This new class made it possible to remove the template process layer from the theme system: https://drupal.org/node/2038981.
Before:
After:
Preprocess function instead of process function and using RenderWrapper to postpone the call to drupal_get_js().
Later preprocess functions must re-wrap the variable in the RenderWrapper if changes are needed, otherwise the variable will be prematurely rendered into a string. Example:
Impacts:
Comment #102
star-szrRan the change notice by @Fabianx and posted:
https://drupal.org/node/2052023
Comment #103
longwaveThis introduced a bug if page_bottom is set by a module; drupal_render() renders the array into a string, then we try to append a RenderWrapper object to it. See #2056837: Modules can no longer alter page_bottom
Comment #104.0
(not verified) CreditAttribution: commentedAdd drupal_get_title() to list of functions that this would be used to late render
Comment #105
sun