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

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

Files: 
CommentFileSizeAuthor
#11 Testing___drupal_8_0_x.png149.3 KBjoelpittet
#11 Recent_log_messages___drupal_8_0_x.png111.82 KBjoelpittet
#11 Extend___drupal_8_0_x.png164.8 KBjoelpittet
#9 increment.txt3.7 KBpwolanin
#9 2505469-9.patch4.55 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,782 pass(es). View
#5 preprocess-table-2505469-3.patch1023 bytespwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,857 pass(es). View
#1 2502781-table-preprocess-7.patch1023 bytespwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,845 pass(es). View

Comments

pwolanin’s picture

Initial patch, was already green on another issue.

Status: Needs review » Needs work

The last submitted patch, 1: 2502781-table-preprocess-7.patch, failed testing.

joelpittet’s picture

Version: 8.1.x-dev » 8.0.x-dev
Priority: Normal » Major
Status: Needs work » Reviewed & tested by the community

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

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1023 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,857 pass(es). View

Here's a remade patch. Let's check docs before rtbc

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Needs tests

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

joelpittet’s picture

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

xjm’s picture

Issue tags: +SafeMarkup
pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +D8 Accelerate
FileSize
4.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,782 pass(es). View
3.7 KB

Here's a couple test cases to verify string or render array work.

xjm’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs tests
FileSize
164.8 KB
111.82 KB
149.3 KB

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

xjm’s picture

Title: Remove drupal_render calls from template_preprocess_table() » Remove drupal_render() calls from template_preprocess_table()
Status: Reviewed & tested by the community » Fixed

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

  • xjm committed 77e9bbe on 8.0.x
    Issue #2505469 by pwolanin, joelpittet: Remove drupal_render() calls...
Wim Leers’s picture

It's early rendering which can prevent some bubbling and modification in the render pipeline.

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

joelpittet’s picture

Ah thanks for the clarification on th bubbling wim

Status: Fixed » Closed (fixed)

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