On my first pass these seem relatively related to be accommodated by a single patch. If it gets unwieldily, I'll create separate issues.

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Assigned: c4rl » Unassigned
Issue tags: +Novice

@c4rl, feel free to re-assign but somebody else will probably pick this up for you :)

anirudha_3083’s picture

Assigned: Unassigned » anirudha_3083
anirudha_3083’s picture

Status: Active » Needs review
FileSize
107.51 KB

covered authorize.php, install.php, update.php and core/includes/* files. Let's try

Status: Needs review » Needs work

The last submitted patch, Replace_theme_with_drupal_render-2007124.patch, failed testing.

anirudha_3083’s picture

Looks like "theme_form_element_label" receives $variables as parameter. Cannot change theme() to drupal_render() in form.inc at lines 4731 & 4737

anirudha_3083’s picture

Status: Needs work » Needs review
FileSize
107.43 KB

forgot to change the status.

Status: Needs review » Needs work

The last submitted patch, Replace_theme_with_drupal_render-2007124-1.patch, failed testing.

anirudha_3083’s picture

Status: Needs work » Needs review
FileSize
22.26 KB

messed up with previous patch due to old master repository. Created a fresh patch with latest repository but #5 still pending.Feel free to assign it to yourself and complete this.

Status: Needs review » Needs work

The last submitted patch, Replace_theme_with_drupal_render-2007124-3.patch, failed testing.

anirudha_3083’s picture

Fixed the issues in last patch. Re-uploading the patch. #5 still remains.

anirudha_3083’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, Replace_theme_with_drupal_render-2007124-4.patch, failed testing.

anirudha_3083’s picture

Status: Needs work » Needs review
FileSize
22.33 KB

incorrect variable name was passed. Fixed. Re-submitting the patch.

Status: Needs review » Needs work

The last submitted patch, Replace_theme_with_drupal_render-2007124-5.patch, failed testing.

star-szr’s picture

Assigned: anirudha_3083 » Unassigned

Unassigning, it's been a while. Anyone is welcome to work on this.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
15.76 KB

Feel free to re-assign if you want this again anirudha

Here's my stab at it. I wasn't about to sort through all those fails/exceptions, so here's a clean slate attempt.

Status: Needs review » Needs work

The last submitted patch, replace_theme_in_includes-2007124-15.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
15.47 KB

Running into the same problem as anirudha: can't convert theme() to drupal_render() in form.inc at lines 4731 & 4737 because theme_form_element_label takes a render element. We've run into this issue on several other occasions, e.g. #2041283: theme_status_report() is severely broken

One solution is to change the argument from a 'render element' to a 'variable', as in the referenced issue. Another is to fix theme_form_element_label.

Status: Needs review » Needs work

The last submitted patch, replace_theme_in_includes-2007124-18.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
15.47 KB

Apparently I can't spell maintenance. "mantenance" and "mainenance" both happened.

thedavidmeister’s picture

Status: Needs review » Postponed
   switch ($element['#title_display']) {
     case 'before':
     case 'invisible':
-      $output .= ' ' . theme('form_element_label', $variables);
+      $output .= ' ' . theme('form_element_label', array('element' => $variables['element']));
       $output .= ' ' . $prefix . $element['#children'] . $suffix . "\n";
       break;
 
     case 'after':
       $output .= ' ' . $prefix . $element['#children'] . $suffix;
-      $output .= ' ' . theme('form_element_label', $variables) . "\n";
+      $output .= ' ' . theme('form_element_label', array('element' => $variables['element'])) . "\n";
       break;

If we need to open a new issue for this to be cleaned up, I think we should do so.

thedavidmeister’s picture

pplantinga’s picture

Re-roll.

Tor Arne Thune’s picture

Title: Replace theme() with drupal_render() in authorize.php, install.php, update.php and core/includes/* » Replace theme() with drupal_render() in authorize.php, install.php and core/includes/*
pplantinga’s picture

Status: Postponed » Needs review
FileSize
13.21 KB

This implements the change in #2046849: Fix theme_form_element_label so that it is compatible with drupal_render()

Let's see if it passes tests.

pplantinga’s picture

Status: Needs review » Postponed
Eric_A’s picture

Status: Postponed » Needs work
+  $form_element_label = array(
+    '#theme' => 'form_element_label',
+    '#element' => $variables['element'],
+  );

This is why the tests were failing.

See my comments in #2030805: Make theme_status_report compatible with drupal_render() for the correct way to build the render element for this type of theme hook and then you won't need to change the type of the theme_hook.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
12.51 KB

Okay, here's the updated patch.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, replace_theme_in_includes-2007124-28.patch, failed testing.

Eric_A’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, replace_theme_in_includes-2007124-28.patch, failed testing.

Eric_A’s picture

@@ -4794,7 +4803,11 @@ function theme_form_element_label($variables) {
   }
 
   // If the element is required, a required marker is appended to the label.
-  $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
+  $form_required_marker = array(
+    '#theme' => 'form_required_marker',
+    '#element' => $element,
+  );
+  $required = !empty($element['#required']) ? drupal_render($form_required_marker) : '';
 
   $title = filter_xss_admin($element['#title']);
 

The $form_required_marker element needs the same fix as $form_element_label...

@@ -4713,16 +4720,18 @@ function theme_form_element($variables) {
   $prefix = isset($element['#field_prefix']) ? '<span class="field-prefix">' . $element['#field_prefix'] . '</span> ' : '';
   $suffix = isset($element['#field_suffix']) ? ' <span class="field-suffix">' . $element['#field_suffix'] . '</span>' : '';
 
+  $form_element_label = array('#theme' => 'form_element_label') + $variables['element'];
+
   switch ($element['#title_display']) {
     case 'before':
     case 'invisible':
-      $output .= ' ' . theme('form_element_label', $variables);
+      $output .= ' ' . drupal_render($form_element_label);
       $output .= ' ' . $prefix . $element['#children'] . $suffix . "\n";
       break;
 
     case 'after':
       $output .= ' ' . $prefix . $element['#children'] . $suffix;
-      $output .= ' ' . theme('form_element_label', $variables) . "\n";
+      $output .= ' ' . drupal_render($form_element_label) . "\n";
       break;
 
     case 'none':

Here I think it is better to be consistent and use $element instead of $variables['element'].
Also, do we need the new line before building the $form_element_label element?

thedavidmeister’s picture

pplantinga’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
12.48 KB

Here are changes @Eric_A recommended, but manual testing shows that it isn't going to work. #2046849: Fix theme_form_element_label so that it is compatible with drupal_render() needs to happen first.

Status: Needs review » Needs work

The last submitted patch, replace_theme_in_includes-2007124-34.patch, failed testing.

pplantinga’s picture

pplantinga’s picture

Status: Postponed » Needs review
FileSize
885 bytes
12.67 KB

How about this? Just put only the things that theme_form_element_label needed into the array. Checked them with the same method in #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter().

At some point $element should stop being sent to theme_form_required_marker, since it doesn't actually need it. Either that or it should handle the required logic.

thedavidmeister’s picture

At some point $element should stop being sent to theme_form_required_marker, since it doesn't actually need it. Either that or it should handle the required logic.

Sounds like a followup issue?

pplantinga’s picture

Status: Needs review » Needs work

The last submitted patch, replace_theme_in_includes-2007124-37.patch, failed testing.

pplantinga’s picture

Well at least it installed...

It looks like to fix it the rest of the way we'll have to include the fix in the follow-up issue. So this either needs to be postponed, or that issue should just be marked as duplicate or whatever.

Also, since we're only passing part of a render element, it's starting to look a whole lot like variables, not render elements. I think passing render elements to multiple theme() functions can't be changed to drupal_render() because it double-renders them...

pplantinga’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, replace_theme_in_includes-2007124-41.patch, failed testing.

Eric_A’s picture

I think passing render elements to multiple theme() functions can't be changed to drupal_render() because it double-renders them...

Don't have much time now to look into this, but perhaps this is an issue with us passing a new version of an element (copy with changed #theme property) instead of passing the same variable or a passing a reference to the variable. This means we should take care to proceed with the correct variable where it matters. (Or stop starting out with a copy, but just change the theme property.)

Might this be the code that creates the double rendering?

+  $form_element_label = array('#theme' => 'form_element_label') + $element;
 
   switch ($element['#title_display']) {
     case 'before':
     case 'invisible':
-      $output .= ' ' . theme('form_element_label', $variables);
+      $output .= ' ' . drupal_render($form_element_label);
       $output .= ' ' . $prefix . $element['#children'] . $suffix . "\n";
       break;
 
     case 'after':
       $output .= ' ' . $prefix . $element['#children'] . $suffix;
-      $output .= ' ' . theme('form_element_label', $variables) . "\n";
+      $output .= ' ' . drupal_render($form_element_label) . "\n";
       break;
 
     case 'none':
Eric_A’s picture

Yeah, we need the #children property of $form_element_label here, not of $element. @pplantinga, can you try the effect on the patch from #34?

Eric_A’s picture

#45 was shouted out in a hurry, lets just think this through a bit. I don't want to rush anyone into making changes that are not analyzed well.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
13.31 KB

Well here's one option that (hopefully) passes tests. I guess I'll give Eric_A's suggestion a try.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, replace_theme_in_includes-2007124-47.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
Issue tags: +Novice
pplantinga’s picture

Was not able to make Eric_A's method work. Perhaps he should take a crack at it if this isn't good enough.

drupalmonkey’s picture

Is this issue actually ready to be reviewed? Is this issue still waiting on #2046849: Fix theme_form_element_label so that it is compatible with drupal_render()??

Also, if it is ready for review, how to test the patch for this?

royko_at_duo’s picture

Status: Needs review » Reviewed & tested by the community

Patch #47 works for me...no instances of theme() found in any of these files after installing patch. Reviewed relevant affected pages.
Notes:
-- #41 Double rendering issue being addressed in drupal.org/node/1920886

Eric_A’s picture

Status: Reviewed & tested by the community » Needs work

@pplantinga, I can see that you tried to implement my remark in #32 about theme_form_required_marker(). I think you got confused by what is in fact very confusing indeed: this function should be passed a render element (in the end mapped to a theme hook variable key "element") which has a *child* element named "element". Core doesn't use this child element in its implementation of theme_form_required_marker() and it seems there are no tests for this in test themes.

pplantinga’s picture

Status: Needs work » Postponed
markhalliwell’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 47: replace_theme_in_includes-2007124-47.patch, failed testing.

markhalliwell’s picture

Issue summary: View changes
Issue tags: +Needs reroll
star-szr’s picture

Most of this territory is now covered in new sub-issues of the parent issue, many of which are in needs review. So leaving this open in case there are some strays but a reroll is probably not going to be productive.

Status: Needs review » Needs work

The last submitted patch, 60: replace-theme-in-includes-2007124-60.patch, failed testing.

me-taras’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: replace-theme-in-includes-2007124-60.patch, failed testing.

star-szr’s picture

Title: Replace theme() with drupal_render() in authorize.php, install.php and core/includes/* » Replace theme() with drupal_render() in authorize.php

Alright, I looked at the other issues in the parent meta and authorize.php and install.php are not covered, but I couldn't find any occurrences in install.php.

So the patch here should only change authorize.php.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
1.46 KB
8.44 KB

Removed all changes except those in core/authorize.php

Messages in test output are not enlightening ... will look again when bot comes back

Status: Needs review » Needs work

The last submitted patch, 65: replace-theme-in-includes-2007124-65.patch, failed testing.

martin107’s picture

The failure point in the test is highlighted below

It is not related to the patch. So HEAD should also fail here.

Also running this local work with and without the patch

php core/scripts/run-tests.sh  --verbose --file core/modules/system/lib/Drupal/system/Tests/Common/RenderTest.php 

show that everything passes... asking for retest

    $xpath = new \DOMXPath($dom);
    $nodes = $xpath->query('//*[@token]');
** $token = $nodes->item(0)->getAttribute('token'); **
    $expected_element = array(
      '#markup' => '<foo><drupal:render-cache-placeholder callback="common_test_post_render_cache_placeholder" context="bar:' . $context['bar'] .';" token="'. $token . '" /></foo>',
      '#post_render_cache' => array(
        'common_test_post_render_cache_placeholder' => array(
          $token => $context,
        ),
      ),
    );
martin107’s picture

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! pretty straightforward

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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