Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Title: Replace theme() with drupal_render() in tracker module » Replace theme() with drupal_render() in update module

wrong module...

aaronott’s picture

Assigned: Unassigned » aaronott
aaronott’s picture

Status: Active » Needs review
FileSize
2.51 KB

So I'm having an issue with this section

@@ -388,10 +399,19 @@ function update_check_requirements($skip_warnings = FALSE) {
   if ($severity == REQUIREMENT_ERROR || ($severity == REQUIREMENT_WARNING && !$skip_warnings)) {
     update_task_list('requirements');
     drupal_set_title('Requirements problem');
-    $status_report = theme('status_report', array('requirements' => $requirements));
+    $status = array(
+      '#theme' => 'status_report',
+      '#requirements' => $requirements,
+    );
+
+    $status_report = drupal_render($status);

This is the part where the test is failing. If I leave the $status as using the theme() function instead of building the array for the render, all the tests pass. Any idea why?

Samvel’s picture

Status: Needs work » Needs review

this code part looks good

Status: Needs review » Needs work

The last submitted patch, 2009688-replace_theme_with_drupal_render_in_update-3.patch, failed testing.

thedavidmeister’s picture

Assigned: aaronott » Unassigned
Status: Needs review » Needs work

feel free to re-assign if you're still working on this @aaronott

tsphethean’s picture

Assigned: Unassigned » tsphethean
aaronott’s picture

@tsphethean I've worked further on this trying to figure out why i'm not able to switch the theme function as mentioned in #3 but no luck. All tests pass if you undo the changes to that status_report theme.

Hope you have better luck, and please if you figure it out do let me know what I missed.

tsphethean’s picture

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

#3: 2009688-replace_theme_with_drupal_render_in_update-3.patch queued for re-testing.

I'm getting the same SimpleTest errors in the Update Functionality tests regardless of whether I have this patch applied or not so want to see what testbot thinks, just in case its something in my setup.

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

The last submitted patch, 2009688-replace_theme_with_drupal_render_in_update-3.patch, failed testing.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Tests were failing because it looks like the structure of $requirements has changed, hopefully this should do it.

Status: Needs review » Needs work

The last submitted patch, drupal_render_update-2009688-11.patch, failed testing.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
4.9 KB

Ok, tests were failing for other instances of theme('status_report') so have updated them too, and then tweaked theme_status_report() to accept the new variables passed in from drupal_render().

thedavidmeister’s picture

if we're changing the way that a theme function works, we need to open a new issue for that.

tsphethean’s picture

Just did a quick test, and if I update the theme function without doing the drupal_render conversion then it will fail... so should the new issue also do the conversions for all places that theme is used?

thedavidmeister’s picture

@tspphethean - yes, if the new issue was "make theme_status_report compatible with drupal_render()" and that was only achievable by making it incompatible with theme() then you would need to perform the conversion there - but that's exactly the kind of discussion we would be having under the new issue.

When you open that issue, could you link to it from #2015147: [meta] Improve the DX of drupal_render(), renderable arrays and the Render API in general as well?

tsphethean’s picture

heddn’s picture

Status: Needs review » Postponed
Eric_A’s picture

Status: Postponed » Needs work
Eric_A’s picture

Status: Needs work » Postponed
star-szr’s picture

Status: Postponed » Needs work

Re-activating since #2041283: theme_status_report() is severely broken was committed. I haven't read through this thoroughly but I'm guessing the patch will need updating.

tsphethean’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Ok, so #2041283: theme_status_report() is severely broken has gone in, which means this can go back to pretty much the same patch as #3 and should now pass (seems good locally).

This still leaves us with theme('status_report') to be converted in install.core.inc and SystemInfoController::status() - but I think they are covered by other issues.

pplantinga’s picture

Status: Needs review » Needs work

One small coding style nitpick: two arrays are missing commas after the last item

tsphethean’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Missing commas added - thanks @pplantinga

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 36d5732 and pushed to 8.x. Thanks!

Tor Arne Thune’s picture

Title: Replace theme() with drupal_render() in update module » Replace theme() with drupal_render() in update system

Changing title so that it's not confused with the update module in Drupal core. The patches in this issue touch the update system, not the update (manager) module.

Tor Arne Thune’s picture

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