Problem/Motivation

In #2171071-8: Rename drupal_add_library() to _drupal_add_library() and remove its uses, moshe writes:

I can ignore hideous code in test files, but this put some hideous code into form.inc and OpenDialogCommand.php. How is this clearer than drupal_add_library():

$attached['#attached']['library'][] = array('system', 'drupal.tableselect');
+      drupal_render($attached);

Is that how theme functions are supposed to add libraries? I would have expected this library to be declared in the form element definition.

This issue is a followup that addresses the DX and quality issues noted by moshe in the original issue to remove drupal_add_library.

Proposed resolution

Move attachments into renderable arrays where possible rather than simply passing an array to drupal_render in-place in a theme function.

User interface changes

None.

API changes

None.

Original report by @jessebeach

CommentFileSizeAuthor
#1 cleanup-remove-drupal-add-library-2173881-1.patch7.12 KBjessebeach

Comments

jessebeach’s picture

jessebeach commented
Status: Active » Needs work
StatusFileSize
new cleanup-remove-drupal-add-library-2173881-1.patch7.12 KB

This patch cleans up theme_table and theme_tableselect.

I'm still working on

  • template_preprocess_html
  • setDialogTitle
  • views_preprocess_page
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'm afraid I've found many flaws in this patch :(

  1. +++ b/core/includes/form.inc
@@ -1565,9 +1565,6 @@ function theme_tableselect($variables) {
-      // Add a "Select all" checkbox.
-      $attached['#attached']['library'][] = array('system', 'drupal.tableselect');
-      drupal_render($attached);

+++ b/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -512,6 +512,12 @@ function system_element_info() {

@@ -512,6 +512,12 @@ function system_element_info() {
     '#options' => array(),
     '#empty' => '',
     '#theme' => 'tableselect',
+    '#attached' => array(
+      'library' => array(
+        // Add a "Select all" checkbox.
+        array('system', 'drupal.tableselect'),
+      ),
+    ),

    You're always attaching the tableselect library, whereas before it was only conditionally added! 

  2. +++ b/core/includes/theme.inc
@@ -1556,25 +1574,6 @@ function theme_table($variables) {
-  // Add sticky headers, if applicable.
-  if (count($header) && $sticky) {

@@ -1458,6 +1458,24 @@ function drupal_pre_render_table(array $element) {
+  // Add sticky headers, if applicable.
+  if (count($element['#header']) && $element['#sticky']) {

    Moving this from theme_table() to drupal_pre_render_table() is going to break things AFAICT: not every theme_table() call is going to result in a call to drupal_pre_render_table()! 

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
@@ -25,9 +25,13 @@ public static function getInfo() {
   function testThemeTableStickyHeaders() {
...
+      '#type' => 'table',
...
+    $table = array(
...
+    $this->content = drupal_render($table);
...
-    $this->content = theme('table', array('header' => $header, 'rows' => $rows, 'sticky' => TRUE));

    I think this is incorrect: this explicitly tests #theme => table, yet you're moving it to #type => table… 

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
@@ -38,12 +42,15 @@ function testThemeTableStickyHeaders() {
   function testThemeTableNoStickyHeaders() {

    Same here.

jessebeach’s picture

jessebeach commented

You're always attaching the tableselect library, whereas before it was only conditionally added!

Right, this was intentional. Maybe not the best choice, but intentional in order to avoid having to add a pre_render function in which the inclusion of the library would be toggled. Since the default of the element is to include the select all checkbox, the element just includes the associated JS file by default as well. As I was discussing this issue with Moshe, he was advocating for less complex code at the cost of flexibility. I don't think I've struck that balance well yet, as you pointed out.

Moving this from theme_table() to drupal_pre_render_table() is going to break things AFAICT: not every theme_table() call is going to result in a call to drupal_pre_render_table()!

No direct call to theme('table', array()) will result in a call to drupal_pre_render_table. That's because calling theme() directly steps into middle of the build process and skips crucial steps. As I was working on this issue, this became more and more apparent. It's ultimately why I opened #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system. If we don't discourage direct calling of theme(), then we can't avoid calling drupal_render() in theme functions because we can't rely on drupal_process_attachments being called on a renderable.

I think this is incorrect: this explicitly tests #theme => table, yet you're moving it to #type => table…

Sure, agreed. I think we're ultimately abusing theme_table() here. If flags to the theme function can vary included resources, then we can't simply call theme directly as the build process currently stands because this function isn't structured to handle outside resources; it simply turns variables into printable strings.

Wim, thanks for the feedback. Given your take, I think we need to postpone this issue on #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system. Your original approach to #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses was appropriate given the current state of the render pipeline and we can't make meaningful refactoring changes unless we privatize theme() first.

jessebeach’s picture

jessebeach commented
Status: Needs work » Postponed

Postponed on #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Postponed » Closed (duplicate)

Reverted #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses, so let's deal with this over there.