Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().

CommentFileSizeAuthor
#18 user-replace_theme_function-2009690-18.patch5.54 KBhussainweb
#16 user-replace_theme_function-2009690-16.patch5.31 KBjeroent
#15 user-replace_theme_function-2009690-15.patch5.35 KBjeroent
#13 user-replace_theme_function-2009690-13.patch5.37 KBjeroent
#8 replace_theme_function_with_drupal_render_in_user_module-2009690-8.patch5.32 KBinternetdevels
#3 replace_theme_function_with_drupal_render_in_user_module-2009690-1.patch3.05 KBZdorovega
#1 replace_theme_function_with_drupal_render_in_user_module-2009690-1.patch3.76 KBZdorovega

Comments

Zdorovega’s picture

Zdorovega commented
StatusFileSize
new replace_theme_function_with_drupal_render_in_user_module-2009690-1.patch3.76 KB
thedavidmeister’s picture

thedavidmeister commented
Status: Active » Needs work 
+    // Set the request in the container to the new created Request above
+    // so it is available to the rest of the installation process.
+    $container
+      ->set('request', $request);

Just wondering why we need this bit?

-          '#description' => theme('user_permission_description', array('permission_item' => $perm_item, 'hide' => $hide_descriptions)),
+          '#description' => array(
+            '#theme' => 'user_permission_description',
+            '#permission_item' =>$perm_item,
+            'hide' => $hide_descriptions,

This should be #hide right?

-  $output = theme('system_compact_link');
-  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'permissions')));
+  $output = array('#theme' => 'system_compact_link');
+    $output .= array(
+      '#theme' => 'table',
+      'header' => $header,
+      'rows' => $rows,
+      'attributes' => array('id' => 'permissions')),
+    );

and this #header, #rows, #attributes?

Zdorovega’s picture

Zdorovega commented
StatusFileSize
new replace_theme_function_with_drupal_render_in_user_module-2009690-1.patch3.05 KB

thank you, the above issues have been fixed.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review

Sending to testbot for now. Thanks @redfoxxx.ua!

Status: Needs review » Needs work

The last submitted patch, replace_theme_function_with_drupal_render_in_user_module-2009690-1.patch, failed testing.

samvel’s picture

samvel commented

@redfoxxx.ua

instead this one:

+  $form['pager'] = array(
+    '#markup' => array('#theme' => 'pager'),
+  );

need:

+  $pager = array('#theme' => 'pager');
+  $form['pager'] = array(
+    '#markup' => drupal_render($pager),
+  );
internetdevels’s picture

internetdevels commented
Assigned: Unassigned » internetdevels

We are working today with this issue during Code Sprint UA.

internetdevels’s picture

internetdevels commented
Assigned: internetdevels » Unassigned
Status: Needs work » Needs review
StatusFileSize
new replace_theme_function_with_drupal_render_in_user_module-2009690-8.patch5.32 KB

Patch attached.

internetdevels’s picture

internetdevels commented
Issue tags: +CodeSprintUA
podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Reviewed & tested by the community

#8 nice work

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Novice, -CodeSprintUA

#8: replace_theme_function_with_drupal_render_in_user_module-2009690-8.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +CodeSprintUA

The last submitted patch, replace_theme_function_with_drupal_render_in_user_module-2009690-8.patch, failed testing.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new user-replace_theme_function-2009690-13.patch5.37 KB

Replaced theme() with drupal_render().

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Needs work

+ '#theme' => 'pager',

Trailing whitespace issue here.

$form['pager'] = array('#markup' => theme('pager'));
+  $pager = array(
+    '#theme' => 'pager', 
+  );
+  $form['pager'] = array('#markup' => drupal_render($pager));

This looks weird to me.

$form['pager'] = array('#theme' => 'pager'),

Should be enough, there's no point in rendering an array into #markup to be passed back to drupal_render() a second time as drupal_render() is already recursive.

+    '#attributes' => array(
+      'id' => 'permissions'
+      ),

Closing parenthesis is indented too far here.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new user-replace_theme_function-2009690-15.patch5.35 KB

Ok, thanks!

Made changes as suggested by thedavidmeister

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new user-replace_theme_function-2009690-16.patch5.31 KB

I forgot to remove: 

+  $pager = array(
+    '#theme' => 'pager',
+  );

This is the right patch.

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Needs work 
+    '#attributes' => array(
+      'id' => 'permissions'
+    ),

This should either be one line or have a trailing comma after 'permissions'

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new user-replace_theme_function-2009690-18.patch5.54 KB

Added the trailing comma and removed some extra whitespace.

scor’s picture

scor commented
Status: Needs review » Reviewed & tested by the community

looks good. all comments have been addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Committed 9dd08c0 and pushed to 8.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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