Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Related to #2393329: Replace all drupal_render calls with the service, and inject it, if possible.
blocking: #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple()
Problem/Motivation
In template_preprocess_table() cells are rendered by calls to drupal_render() but they should be rendered by Twig instead of they are render arrays.
This change should be transparent to calling code.
Proposed resolution
- Avoid rendering manually by letting the template who is printing the variable render it.
Remaining tasks
Patch including code and docs changes.
User interface changes
None
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | Testing___drupal_8_0_x.png | 149.3 KB | joelpittet |
| #11 | Recent_log_messages___drupal_8_0_x.png | 111.82 KB | joelpittet |
| #11 | Extend___drupal_8_0_x.png | 164.8 KB | joelpittet |
| #9 | increment.txt | 3.7 KB | pwolanin |
| #9 | 2505469-9.patch | 4.55 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin at Acquia commentedInitial patch, was already green on another issue.
Comment #3
joelpittetIt's early rendering which can prevent some bubbling and modification in the render pipeline. We don't need to call this because twig will call it when the cells are rendered.
Comment #5
pwolanin CreditAttribution: pwolanin at Acquia commentedHere's a remade patch. Let's check docs before rtbc
Comment #6
xjmThis issue helps address a critical and has all sorts of potential benefits for removing overhead, reducing memory use, improving cacheability, etc., so bumping to major and signing off on the issue as allowed during beta barring some gnarly disruption we haven't identified.
Um... so crosspost. That said, while I'm as excited as y'all are. I'd like to see some careful manual testing and documentation of the existing automated test coverage, or addition of some specific tests. :) Also let's check the documentation.
Comment #7
joelpittetOh I bumped it to major because it may be blocking another major for for safemarkup #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple()
Comment #8
xjmComment #9
pwolanin CreditAttribution: pwolanin at Acquia commentedHere's a couple test cases to verify string or render array work.
Comment #10
xjmIn case anyone's wondering, the early render dates to the introduction of support for render arrays in table cells in #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable back in 2009.
Comment #11
joelpittetWould love short array syntax. Willing to do the conversion if you are cool with that @pwolanin otherwise this is good to go in.
Not a heavy manual testing needed here because the test shows it all but here are a couple screenshots of tables rendering in core.
Comment #12
xjmMuch exciting! Thanks for the manual testing and the automated tests. I'd rather commit this than wait to change the array format, so doing that.
This issue helps unblock another major child of a critical issue and can potentially reduce overhead and memory use, improve cacheability for tables, and so on, so I'm approving this change for the Drupal 8 beta. Yay for less early rendering!
Comment #14
Wim LeersThis is not actually true; rendering in preprocess functions is supported, because theme preprocess functions are invoked by the renderer. I.e. they're invoked as *part* of the render process, hence they don't constitute "early rendering".
(If this didn't work, then you also wouldn't be able to use
$variables['#attached']in preprocess functions.)That being said, this is still a net improvement, it makes the rendering easier to understand. And of course, the other benefits listed here still apply.
Comment #15
joelpittetAh thanks for the clarification on th bubbling wim