Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In order to resolve #2009688: Replace theme() with drupal_render() in update system, theme_status_report needs to be updated to be compatible with drupal_render().
Proposed resolution
Update theme_status_report() and convert all implementations of theme('status_report') to the new drupal_render() approach.
Comments
Comment #0.0
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #0.1
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #1
tsphethean CreditAttribution: tsphethean commentedComment #2
thedavidmeister CreditAttribution: thedavidmeister commented+ $status_report = drupal_render($status); $status_report .= t('Check the messages and <a href="!url">try again</a>.', array('!url' => check_url(drupal_requirements_url($severity))));
No new line here after the first ;
+ return drupal_render($status);
Extra spaces after return.
+ //$helpful_links = update_helpful_links();
Need a space between // and $
This has nothing to do with theme_status_report
Neither does this.
Or this.
Comment #3
Eric_A CreditAttribution: Eric_A commentedThe status_report() theme hook takes a render element as its only argument. This element is passed in $variables['requirements']. See system_theme().
When using this theme hook in a render array #theme property, the render structure should not have a #requirements property. It should have an array of requirements added.
Comment #4
Eric_A CreditAttribution: Eric_A commentedJust try something like:
$status_report = array('#theme' => 'status_report') + $requirements;
I also edited my previous comment: it should have said $variables['requirements'] not $variables['#requirements'].
Comment #5
tsphethean CreditAttribution: tsphethean commentedOk, thanks Eric_A. Doing that still requires a change to theme_status_report() though, as it ends up with us needing to call element_children() on render array so that we only try to render valid entries and not other elements of the render element.
I've attached a patch fixing this, and incorporating the remaining comments from #2.
Comment #6
Eric_A CreditAttribution: Eric_A commentedIndeed, it appears that this theme function is not up to the task. Thanks for sticking with this!
So what this needs now is a patch with a test that renders the example from #5 and a patch that contains both the test and the fix but not the example. @tsphethean, would you give it a spin? Need help?
Comment #7
tsphethean CreditAttribution: tsphethean commentedSure, I'll get that sorted tomorrow hopefully.
Comment #8
tsphethean CreditAttribution: tsphethean commentedEric_A: I took a look at existing tests for the update process (where the example from #5 gets called) and it looks like
Drupal\system\Tests\Update\UpdateScriptTest::testRequirements()
already covers the checks for rendering theme_status_report correctly.So, the two patches I've added to this post demonstrate:
1. Update.php conversion from theme('status_report') to drupal_render(), which should demonstrate the failure
2. Fix to theme_status_report() system.admin.inc which should demonstrate that testRequirements() still passes with this alteration - whether called through the old theme('....') method, or by drupal_render.
Let me know if you think that testRequirements() doesn't cover enough and I can write an additional test
Comment #10
Eric_A CreditAttribution: Eric_A commentedThe fail in Drupal\field\Tests\FieldAttachStorageTest::testFieldAttachSaveEmptyDataDefaultValue is a new one, I guess?
Anyway, the more I think about it the more I wonder why theme_status_report() declares a render element instead of a variable. When you look at the code it's not operating on a render element, but on an array of arrays with nothing but plain strings. (drupal_render() would choke on that.) Except that it checks for a '#type' property on the children. (Why?)
Perhaps we should open an issue to convert it to accept a variable. A straight conversion in system_theme() would probably work right away. (I don't think any caller in core is passing in #type properties. And it's not documented at all.)
Then again, I have no knowledge whatsoever of the history of this function.
Comment #11
tsphethean CreditAttribution: tsphethean commented#8: theme_status_report_compatibility-fix_only--2030805-8.patch queued for re-testing.
Yes, that's a new failure and one I don't get in my dev site so have re-queued for testing as I know there's a few issues about random failures being discussed.
On the refactoring of the function, how do you want to proceed - should we get this in to unblock the conversion to drupal_render from theme(...), or should we refactor system_theme() first and then get this in later?
Just did a quick search for related issues and it looks like #2013258: Simplify render for theme_status_report() is already doing a refactor of theme_status_report for the twig conversion which would might even render (bad pun) this ticket irrelevant.
Comment #12
Eric_A CreditAttribution: Eric_A commentedThe Twig conversion is not going to help us much, I think. I've opened #2041283: theme_status_report() is severely broken.
We should probably just work on both issues. Whichever gets in will unblock the conversion to render elements.
Comment #13
pplantinga CreditAttribution: pplantinga commentedI'm for the approach in #2041283: theme_status_report() is severely broken
Perhaps close this as duplicate?
Comment #14
Eric_A CreditAttribution: Eric_A commented#2041283: theme_status_report() is severely broken got in, which makes the old patches in #2009688: Replace theme() with drupal_render() in update system and #2009674: Replace theme() with drupal_render() in system module suddenly look a lot better :-)
With the decision made to change this theme hook from taking a render element to taking a variable with plain data, the approach here is no longer needed. I'm setting this to closed (duplicate), then.
Credits to @tsphethean who really helped a lot making the theme_status_report() issue getting the attention it needed.
Comment #15
tsphethean CreditAttribution: tsphethean commentedThanks @Eric_A - #2041283: theme_status_report() is severely broken looks a much better solution!
Comment #15.0
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.