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

#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

CommentFileSizeAuthor
#98 drupal-render_wrapper-2004286-98.patch11.18 KBjenlampton
#98 interdiff.txt529 bytesjenlampton
#97 drupal-render_wrapper-2004286-92.patch10.99 KBjenlampton
#97 interdiff.txt888 bytesjenlampton
#95 drupal-render_wrapper-2004286-92.patch10.99 KBjenlampton
#95 interdiff.txt888 bytesjenlampton
#92 2004286-92-failing-test-with-73.patch11.03 KBstar-szr
#92 2004286-92.patch10.95 KBstar-szr
#92 interdiff.txt1.92 KBstar-szr
#86 2004286-86-render-wrapper.patch9.03 KBjoelpittet
#86 interdiff.txt1.64 KBjoelpittet
#76 Drupal_D8_Dev_and_Inbox_-_splendidnoise_gmail.com_-_Gmail_and_Inbox__9__-_jesse.beach_acquia.com_-_Acquia__Inc_Mail_and_d8_—_bash_—_bash_—_Ocean_—_140×33_—_⌘1.png107.59 KBjessebeach
#73 2004286-73.patch9.11 KBstar-szr
#73 interdiff.txt892 bytesstar-szr
#72 2004286-72.patch9.31 KBstar-szr
#72 interdiff.txt1.74 KBstar-szr
#67 2004286-67.patch9.87 KBstar-szr
#67 interdiff.txt3.75 KBstar-szr
#60 2004286-60.patch7.06 KBstar-szr
#60 interdiff.txt1.18 KBstar-szr
#58 twig-render_wrapper-2004286-58.patch6.03 KBLinL
#54 twig-render_wrapper-2004286-54.patch6.06 KBjenlampton
#54 interdiff.txt2.26 KBjenlampton
#50 twig-render_wrapper-2004286-50.patch6.12 KBjenlampton
#50 interdiff.txt2.18 KBjenlampton
#48 twig-render_wrapper-2004286-48.patch5.97 KBjenlampton
#48 interdiff.txt2.14 KBjenlampton
#41 render-wrapper-2004286-41.patch93.3 KBjenlampton
#41 render-wrapper-2004286-41.patch93.24 KBjenlampton
#41 render-wrapper-2004286-41.patch93.23 KBjenlampton
#41 interdiff.txt1.99 KBjenlampton
#36 interdiff.txt1.75 KBjoelpittet
#36 2004286-36-render-wrapper.patch5.93 KBjoelpittet
#34 interdiff.txt2.54 KBjoelpittet
#34 2004286-34-render-wrapper.patch6.01 KBjoelpittet
#32 interdiff.txt6.33 KBjoelpittet
#32 2004286-32-render-wrapper.patch6.09 KBjoelpittet
#17 2004286-17-renderable-wrapper.patch5.28 KBjoelpittet
#17 interdiff.txt5.01 KBjoelpittet
#16 2004286-16-renderable-wrapper.patch6.15 KBjoelpittet
#16 interdiff.txt3.55 KBjoelpittet
#9 late-render-css-js-8.patch5.57 KBjoelpittet
#7 late-render-css-js-7.patch5.56 KBjoelpittet
#6 late-render-css-js-5.patch0 bytesjoelpittet
#2 late-render-css-js-2.patch5.53 KBjoelpittet
late-render-css-js.patch5.97 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dasjo’s picture

joelpittet’s picture

FileSize
5.53 KB

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

Status: Needs review » Needs work
Issue tags: -Coding standards, -Needs manual testing, -theme system cleanup

The last submitted patch, late-render-css-js-2.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

#2: late-render-css-js-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Coding standards, +Needs manual testing, +theme system cleanup

The last submitted patch, late-render-css-js-2.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
0 bytes

typo, same as #2

joelpittet’s picture

FileSize
5.56 KB

hmm, well there was more typos in the class consolidation... that name sucks anyway. Please suggest a better class name.

Status: Needs review » Needs work

The last submitted patch, late-render-css-js-7.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.57 KB

ok slow down... this is right.

joelpittet’s picture

Issue summary: View changes

added related meta

joelpittet’s picture

Title: Wrap drupal_get_js() and drupal_get_css() in classes to remove process layer » Wrap drupal_get_* function into a class to devert rendering until printed.

better issue title

mlncn’s picture

Status: Needs review » Needs work

Without and with the patch both have this weirdness in the page classes:

page-node page-node- page-node-1

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.

dasjo’s picture

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

+++ b/core/includes/theme.incundefined
@@ -2975,30 +2993,6 @@ function template_process_page(&$variables) {
-  $variables['page_bottom'] .= drupal_get_js('footer');
+++ b/core/includes/theme.incundefined
@@ -2849,6 +2850,23 @@ function template_preprocess_html(&$variables) {
+  $variables['page_bottom'] = new RenderableWrapper('drupal_get_js', $args = array('footer'));
joelpittet’s picture

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

Fabianx’s picture

I suggest working more strongly with Render API here.

I think if page_bottom was an array, that:

{{ page_bottom }}

should still work, so:

  $variables['page_bottom'][] = new RenderableWrapper('drupal_get_js', $args = array('footer'));

might directly make this work.

The $args = array('footer') feels a little strange to me ... Was that intended?

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.55 KB
6.15 KB

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

joelpittet’s picture

Title: Wrap drupal_get_* function into a class to devert rendering until printed. » Wrap drupal_get_* functions to devert rendering until printed.
FileSize
5.01 KB
5.28 KB

Ok moved to #type, probably did this wrong and still need better name for this... but this code works so worth a look see.

joelpittet’s picture

Possible 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

Status: Needs review » Needs work
Issue tags: -Coding standards, -Needs manual testing, -theme system cleanup

The last submitted patch, 2004286-17-renderable-wrapper.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: +Coding standards, +Needs manual testing, +theme system cleanup
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Coding standards, -Needs manual testing

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

tim.plunkett’s picture

What does the word "devert" mean?

joelpittet’s picture

Title: Wrap drupal_get_* functions to devert rendering until printed. » Wrap drupal_get_* functions to defer rendering until printed.

Something about dropping your religion I guess. http://www.urbandictionary.com/define.php?term=devert

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work

Reviewed 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"

joelpittet’s picture

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

joelpittet’s picture

Would 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;)

Fabianx’s picture

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

moshe weitzman’s picture

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

Fabianx’s picture

I totally agree with that. We are trying it with #pre_render only now :).

joelpittet’s picture

Ok 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

joelpittet’s picture

Title: Wrap drupal_get_* functions to defer rendering until printed. » Defer calls to drupal_get_* functions until printed inside a template.
Assigned: Unassigned » joelpittet
joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.09 KB
6.33 KB

Same as #16 with new name(welcome to gave name suggestions if that helps).

star-szr’s picture

Status: Needs review » Needs work

Nice work on this @joelpittet! Documentation nitpickery ahead:

  1. +++ b/core/includes/theme.incundefined
    @@ -2781,6 +2782,26 @@ function template_preprocess_html(&$variables) {
    +  // Previous process layer.
    

    Not sure this comment is needed, I vote we remove it.

  2. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,77 @@
    +/**
    + * @file
    + * Definition of Drupal\Core\Template\RenderWrapper.
    + */
    

    This should be 'Contains \Drupal\Core…' per https://drupal.org/node/1354#file, the standard for this changed.

  3. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,77 @@
    +/**
    + * Renderable wrapper for drupal_get_* functions.
    + */
    +
    +
    +/**
    + * A class that wraps drupal_get_* functions so they can be rendered in a template.
    + *
    + * To use, one may pass in the function name as a string followed by an array of
    + * arguments to the contstructor.
    + * @code
    + *  $variables['scripts'] = new RenderWrapper('drupal_get_js');
    + *  {{ scripts }};
    + *  // Produces
    + *  <script src="http://domain.com/core/misc/script/script.min.js?v=2.6.2"></script>
    + * @endcode
    + *
    + */
    +class RenderWrapper {
    

    I think the 'Renderable wrapper' comment can be removed.

  4. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,77 @@
    +   * Constructor
    +   *
    +   * @param string $callback the callback function name
    +   * @param array  $args
    

    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.

  5. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,77 @@
    +   * @return string the render method's results.
    ...
    +   * @return string the results of the drupal_get_* functions.
    

    Capitalize 'the' after the datatype.

  6. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,77 @@
    +   * Call the callback function.
    

    'Calls' per http://drupal.org/node/1354#functions.

joelpittet’s picture

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

Thanks @Cottser, attached is the changes from #33

star-szr’s picture

Looking much better, a few more suggestions:

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
@@ -0,0 +1,68 @@
+namespace Drupal\Core\Template;
+
+
+/**

Remove extra line between namespace and class docblock.

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
@@ -0,0 +1,68 @@
+ *  {{ scripts }};

If we're using Twig in the code sample there should be no semicolon here.

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
@@ -0,0 +1,68 @@
+ *  // Produces
+ *  <script src="http://domain.com/core/misc/script/script.min.js?v=2.6.2"></script>
+ * @endcode

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.

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
@@ -0,0 +1,68 @@
+   * @param string $callback The callback function name.
+   * @param array $args The arguments to pass to the callback function.
...
+   * @return string The results of the drupal_get_* functions.

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.

joelpittet’s picture

Thanks again. I removed the extra cruft from the code sample including that comment.

moshe weitzman’s picture

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

joelpittet’s picture

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

Fabianx’s picture

I re-reviewed it again.

When the css variable is removed this is RTBC in my opinion.

azinoman’s picture

Status: Needs review » Needs work

I applied the patch but the styles for the tool bar seem like they're missing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
93.23 KB
93.24 KB
93.3 KB

It 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 in drupal_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.

Status: Needs review » Needs work

The last submitted patch, render-wrapper-2004286-41.patch, failed testing.

jenlampton’s picture

Status: Needs work » Postponed

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

jenlampton’s picture

Issue summary: View changes

Added related / duplicate issue.

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Status: Needs work » Needs review

unblocked now that #2003814: Remove Bartik process layer functions, refactor color module alterations is committed :)
Probably needs a reroll.

jenlampton’s picture

Status: Postponed » Needs review
Issue tags: -theme system cleanup

#41: render-wrapper-2004286-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +theme system cleanup

The last submitted patch, render-wrapper-2004286-41.patch, failed testing.

jenlampton’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

tagging for later today

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.14 KB
5.97 KB

okay, let's try this again.

star-szr’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Needs work
Issue tags: +Novice

Yay green! Here's a review of things that stood out to me, otherwise this is looking very good I'd say!

  1. +++ b/core/includes/theme.incundefined
    @@ -3040,9 +3024,11 @@ function template_process_maintenance_page(&$variables) {
    +  $variables['styles'] = new RenderWrapper('drupal_get_css', $args = array($css));
    

    This should just be new RenderWrapper('drupal_get_css', array($css)); - the $args = is unnecessary.

  2. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,67 @@
    + * To use, one may pass in the function name as a string followed by an array of
    + * arguments to the contstructor.
    + * @code
    + *  $variables['scripts'] = new RenderWrapper('drupal_get_js', array('footer'));
    + * @endcode
    

    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.

  3. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,67 @@
    + * arguments to the contstructor.
    

    Typo: constructor.

  4. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,67 @@
    +   * Returns a string provided by the callback function.
    +   *
    +   * @return string
    +   *   The results of the drupal_get_* functions.
    

    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.

  5. +++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
    @@ -0,0 +1,67 @@
    +  public function render() {
    +   if (!empty($this->callback) && is_callable($this->callback)) {
    +      return call_user_func_array($this->callback, $this->args);
    +    }
    +  }
    +}
    

    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.

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +theme system cleanup
FileSize
2.18 KB
6.12 KB

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

The results of the callback function.

Looks like I forgot...

Also, there should be a blank line between the last method and the end of the class.

Issue tags: -Novice, -theme system cleanup

The last submitted patch, twig-render_wrapper-2004286-50.patch, failed testing.

jenlampton’s picture

Issue tags: -Novice, -theme system cleanup

Status: Needs review » Needs work
Issue tags: +Novice, +theme system cleanup

The last submitted patch, twig-render_wrapper-2004286-50.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
6.06 KB

bah, okay, trying again.

joelpittet’s picture

Issue tags: -Novice

@jenlampton @Cottser thank you for all the changes on that one.

joelpittet’s picture

Issue summary: View changes

update dupe issue

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

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

This needs a reroll and a small docs tweak, otherwise this looks good to me.

+++ b/core/lib/Drupal/Core/Template/RenderWrapper.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Stores the callback function to be called when rendered.
+   *
+   * @var array
+   */
+  public $callback = NULL;

This should be changed to @var string, see the constructor docblock.

LinL’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.03 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, twig-render_wrapper-2004286-58.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
7.06 KB

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

star-szr’s picture

Title: Defer calls to drupal_get_* functions until printed inside a template. » Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class

Re-titling, this really needs a review so we can remove the process layer.

jenlampton’s picture

Assigned: Unassigned » jenlampton

dibbs!

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

I 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! :)

jenlampton’s picture

Assigned: jenlampton » Unassigned

yah

jenlampton’s picture

Issue summary: View changes

remove related

star-szr’s picture

Assigned: Unassigned » star-szr
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Talked to @alexpott and this should have test coverage, I'm on it.

star-szr’s picture

Also, per @alexpott on IRC:

  • We allow $callback to be NULL but that doesn't really make sense, that's the whole purpose of this class.
  • We should throw an InvalidArgumentException if the callback is not callable.
star-szr’s picture

Issue summary: View changes

Update summary to reflect latest patch

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.75 KB
9.87 KB

Changes:

  • Added PHPUnit test coverage for the RenderWrapper class: simple string manipulation callback, callback with no arguments, and namespaced callback.
  • Now throwing an exception when the callback is not callable, with test coverage.
  • Now throwing an exception when the arguments are not passed as an array, with test coverage.
c4rl’s picture

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

  • abstract class PresenterBase
  • CssPresenter extends PresenterBase
  • JsPresenter extends PresenterBase
  • HtmlHeadPresenter extends PresenterBase
c4rl’s picture

Issue summary: View changes

Code block -> list

jenlampton’s picture

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

star-szr’s picture

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

okay, 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)

star-szr’s picture

FileSize
1.74 KB
9.31 KB

Simpler - don't need to throw an exception for non-array arguments. Just type-hint and PHP handles it. Thanks @alexpott :)

star-szr’s picture

FileSize
892 bytes
9.11 KB

PHPUnit awesomeness - I am definitely a fan. Thanks again goes to @alexpott :)

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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

jessebeach’s picture

FileSize
107.59 KB

We're seeing the gray messages bar in appear on the page when no messages exist.

messages bar

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.

nod_’s picture

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.

jessebeach’s picture

jessebeach’s picture

nod_, re: #77, maybe we just need the right JS/CSS then to hide the messages area when it's empty?

Wim Leers’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

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


The main criteria are that these features have to be self contained (no follow-up tasks) and easy to roll back (limited inter-module dependencies). If a single critical or major is filed as a result of these commits, we will favour rollback over going forward.

http://buytaert.net/drupal-8-api-freeze

catch’s picture

Category: bug » task
Priority: Critical » Normal

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

alexpott’s picture

Status: Active » Needs work

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

star-szr’s picture

Sorry folks :(

Wim Leers’s picture

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

joelpittet’s picture

+++ b/core/includes/theme.inc
@@ -2690,6 +2691,24 @@ function template_preprocess_html(&$variables) {
+
+  // Place the rendered HTML for the page body into a top level variable.
+  $variables['page'] = drupal_render($variables['page']);

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
9.03 KB

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

star-szr’s picture

Assigned: Unassigned » star-szr
Status: Needs review » Needs work
Issue tags: +Needs tests

Working on tests to avoid the bug reported in #2033275: Contextual links broken because of JS error from recurring.

joelpittet’s picture

@jenlampton could you have a look at #86 to make sure it doesn't break what you were fixing in #41?

jenlampton’s picture

Status: Needs work » Reviewed & tested by the community

Sure 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! :)

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Working on the test coverage still.

star-szr’s picture

This is still on my radar. Will likely happen Thursday or early next week.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.92 KB
10.95 KB
11.03 KB

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Nice! :) Patch still looks good, new tests passing. Assets, messages, titles, and breadcrumbs still appearing as normal. Code looks great! back to RTBC :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/seven/seven.themeundefined
@@ -144,6 +144,8 @@ function seven_tablesort_indicator($variables) {
+  $variables['styles'] = drupal_get_css();
+  $variables['scripts'] = drupal_get_js();

Lets wrap these in the render wrapper too... like so :)

  $variables['styles'] = new RenderWrapper('drupal_get_css');
  $variables['scripts'] = new RenderWrapper('drupal_get_js');
jenlampton’s picture

Status: Needs work » Needs review
FileSize
888 bytes
10.99 KB

yah, agree.

Status: Needs review » Needs work

The last submitted patch, drupal-render_wrapper-2004286-92.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
888 bytes
10.99 KB

trying again (thanks @cottser!)

jenlampton’s picture

trying again (thanks @cottser!)

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed with Dreditor, patch passes!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8b4fa68 and pushed to 8.x. Thanks!

star-szr’s picture

Title: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class » Change notice: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class
Priority: Normal » Critical
Status: Fixed » Needs review

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

function template_process_html(&$variables) {
  $variables['scripts'] = drupal_get_js();
}

After:
Preprocess function instead of process function and using RenderWrapper to postpone the call to drupal_get_js().

function template_preprocess_html(&$variables) {
  $variables['scripts'] = new RenderWrapper('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:

function seven_preprocess_html(&$variables) {
  $variables['scripts'] = new RenderWrapper('drupal_get_js');
}

Impacts:

  • Module developers
  • Themers
star-szr’s picture

Title: Change notice: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class » Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class
Priority: Critical » Normal
Status: Needs review » Fixed

Ran the change notice by @Fabianx and posted:
https://drupal.org/node/2052023

longwave’s picture

This 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

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

Anonymous’s picture

Issue summary: View changes

Add drupal_get_title() to list of functions that this would be used to late render

sun’s picture