In system_theme() we see that theme_status_report() takes a render element. But in fact all callers pass an array that is not compatible with drupal_render(), which the theme function seems to try to work around. It fails in real life cases though, which we have now in #2009688: Replace theme() with drupal_render() in update system and #2009674: Replace theme() with drupal_render() in system module. Note that the issue is not triggered by the use of drupal_render(). Passing in any real life render element directly into the theme function will wreak havoc. Adding a #theme property is enough to make it fail.
#2030805: Make theme_status_report compatible with drupal_render() tries to fix this by improving the workarounds in theme_status_report(). But perhaps this function was never really meant to take a render element? It was declared as such in #600974: theme() fails for theme functions taking one argument in renderable arrays where it was needed to explicitely set variables/ render element for existing theme hooks. See also #572618: All theme functions should take a single argument to make preprocess sane and meaningful.
I don't have an explanation right now why the code is explicitely excluding keys with an not empty #type property. Could not find where this was added. Could not find calling code that passes anything else than straight arrays coming from hook_requirements().
Comment | File | Size | Author |
---|---|---|---|
#1 | theme_status_report-variables-2041283-1.patch | 484 bytes | Eric_A |
Comments
Comment #1
Eric_A CreditAttribution: Eric_A commentedLet's see if anything breaks if we change the definition to taking one argument with plain data instead of a render element.
Comment #2
Eric_A CreditAttribution: Eric_A commented#2009688: Replace theme() with drupal_render() in update system and #2009674: Replace theme() with drupal_render() in system module are currently blocked on either #2030805: Make theme_status_report compatible with drupal_render() or this. The fix here may make a lot of sense for Drupal 8, but it's an API change so we need to decide fast.
Comment #2.0
Eric_A CreditAttribution: Eric_A commentedChanged "type" to "variables/ render element".
Comment #2.1
Eric_A CreditAttribution: Eric_A commentedAdded issues and the #theme example.
Comment #2.2
Eric_A CreditAttribution: Eric_A commentedMade it clear that this is about theme() not drupal_render().
Comment #3
Eric_A CreditAttribution: Eric_A commentedComment #3.0
Eric_A CreditAttribution: Eric_A commentedchanged set to not empty to be accurate.
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedI'm not sure how much test coverage there is around status messages. It would be great if #1 is all we have to do here but I think we should get someone to eyeball/diff the markup manually for a few different status messages before committing.
From a coding standards POV this seems fine to me, the patch is not touching much.
The opening line in theme_status_report() suggests that #1 is likely the right thing to do:
$requirements = $variables['requirements'];
I agree that this looks suss:
if (empty($requirement['#type'])) {
I can only assume it was intended as a janky test to see if $requirement is a child render array that theme_status_report() should ignore or not as drupal_render() would handle it. Probably #2013258: Simplify render for theme_status_report() is the better place to clean that up though.
Comment #5
catchThis isn't a commonly used theme function so no problem with the API change. It could use a test though even if it's just visiting admin/reports/status, I'm surprised we have no coverage of that - or do we only cover the case that it works.
Also is this a problem in 7.x as well?
Comment #6
Eric_A CreditAttribution: Eric_A commentedI expect the problem to be in 7.x as well.
We have currently passing tests that trigger direct calls to theme_status_report(). When converting these calls to drupal_render() we get an extra #theme property for free and this scenario is enough to cause the fails.
The conversion in #2030805: Make theme_status_report compatible with drupal_render() created problems in
Drupal\system\Tests\Update\UpdateScriptTest
andDrupal\system\Tests\Upgrade\ExistingModuleNameLengthUpgradePathTest
. See https://qa.drupal.org/pifr/test/573878Incorporating this there would fix those.
Same for #2009688: Replace theme() with drupal_render() in update system and #2009674: Replace theme() with drupal_render() in system module although those patches are not quite ready for the final test. These will will show us the same or more relevant tests when near ready.
Comment #7
Eric_A CreditAttribution: Eric_A commentedFixed the link to qa in my previous post. The correct link is https://qa.drupal.org/pifr/test/573878
Comment #8
catchOK so the tests pass now but will fail with those conversion without this patch applied. In that case this looks RTBC.
Comment #9
pplantinga CreditAttribution: pplantinga commentedThere are several issues blocked on this, so it'd be nice to get it in!
Comment #10
alexpottCommitted 780bc62 and pushed to 8.x. Thanks!
Comment #11.0
(not verified) CreditAttribution: commenteddrupal_render => drupal_render()