Replace all calls to theme() with a render array that is passed to drupal_render().

Change record: theme() renamed to _theme() and should not be called directly.

CommentFileSizeAuthor
#14 2425089-12.patch1.31 KBpfrenssen
#6 2425089-6.patch1.65 KBPalashvijay4O
#4 2425089-4.patch1.63 KBPalashvijay4O
#1 2425089-1.patch1.59 KBPalashvijay4O

Comments

Palashvijay4O’s picture

Palashvijay4O commented
Status: Active » Needs review
StatusFileSize
new 2425089-1.patch1.59 KB

Replaced all the instances! Please review.

jonathan1055’s picture

jonathan1055 commented

Nice work. But may I suggest the variable name $markup is confusing (a) because it does not say what it contains, and (b) because it is reused again with different contents. Also you have concatenated to it second time, but that does not look the correct thing to do, as it may give the wrong result for if (count($rows) && $markup)

But thanks for starting this. It's easy to fix the above.

Jonathan

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Needs work
Palashvijay4O’s picture

Palashvijay4O commented
Status: Needs work » Needs review
StatusFileSize
new 2425089-4.patch1.63 KB

Please Suggest the changes if you have some good variable names :). Thanks.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Needs work

Thanks for working on this!

A few suggestions:

  1. +++ b/scheduler.admin.inc
@@ -237,7 +237,11 @@ function _scheduler_generate_key($form, &$form_state) {
+  $scheduler_timecheck = array(
+    'now' => REQUEST_TIME,
+  );

    This is missing the line '#theme' => 'scheduler_timecheck'. 

  2. +++ b/scheduler.admin.inc
@@ -237,7 +237,11 @@ function _scheduler_generate_key($form, &$form_state) {
+  $render_array = drupal_render($scheduler_timecheck);
+  return $render_scheduler;

    This is not entirely correct, drupal_render() does not return a render array, but it does accept a render array as an argument.

    I would simply replace it like this:

    return drupal_render($scheduler_timecheck); 

  3. +++ b/scheduler.admin.inc
@@ -371,17 +375,23 @@ function scheduler_list() {
+  $render_pager .= drupal_render($pager);

    It doesn't seem needed to concatenate the string.

Palashvijay4O’s picture

Palashvijay4O commented
Status: Needs work » Needs review
StatusFileSize
new 2425089-6.patch1.65 KB

Thanks for pointing out the mistakes . Done with the changes please review.

kalinchernev’s picture

kalinchernev commented
Assigned: Unassigned » kalinchernev
kalinchernev’s picture

kalinchernev commented

Good progress in overall. Few remarks I would add:

  1. +++ b/scheduler.admin.inc
@@ -238,7 +238,11 @@ function _scheduler_generate_key($form, &$form_state) {
+    'now' => REQUEST_TIME,

    Honestly, I'm not sure this is the correct way to prepare the render array here.

    Look at the definition from the hook_theme implementation:

    /**
 * Implements hook_theme().
 */
function scheduler_theme() {
  return array(
    'scheduler_timecheck' => array(
      'arguments' => array('now' => NULL),
    ),
  );
}

    I think the 'now' element should actually be prepared in 'arguments' property? 

  2. +++ b/scheduler.admin.inc
@@ -362,18 +366,23 @@ function scheduler_list() {
+    $username = array(
+      '#theme' => 'username',
+      'account' => $node,
+      );
...
+      ($node->type ? drupal_render($username) : ''),

    Looks better after implementing the feedback.

    However, the'username' theme type is part of D7: https://api.drupal.org/api/drupal/includes!theme.inc/function/theme_user..., but I can't find the same mapping in Drupal 8. I think the closest you can take is 'user' from the list https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph...

    Theferore, I would double-check this implementation. 

  3. +++ b/scheduler.admin.inc
@@ -362,18 +366,23 @@ function scheduler_list() {
+  $pager = array('#theme' => 'pager');
+  if (count($rows) && (drupal_render($pager))) {

    This seems a correct implementation. It might be possible to optimize 1 step forward placing the $pager contents directly inside the drupal_render(). 1 variable less in memory initialization

Also, by the way, it seems that drupal_render() is another function which is going to be deprecated very soon in favor of an OOP method. Look at this one: https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup...

Wouldn't it be a good try to jump on this approach directly?

PS: Latest #6 patch is visible in the list now. The rest are older versions which we could hide to avoid confusion.

kalinchernev’s picture

kalinchernev commented
Assigned: kalinchernev » Unassigned
Status: Needs review » Needs work
ieguskiza’s picture

ieguskiza commented
Assigned: Unassigned » ieguskiza
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Issue tags: +Needs reroll

Needs to be rerolled.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: ieguskiza » Unassigned
Status: Needs work » Fixed
Issue tags: -Needs reroll
StatusFileSize
new 2425089-12.patch1.31 KB

There were only two instances left, and the original patch was outdated since drupal_render() has been deprecated in the meanwhile. Did a new conversion from scratch.

Committed to 8.x-1.x, thanks all!

Status: Fixed » Closed (fixed)

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