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().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eric_A’s picture

Status: Active » Needs review
FileSize
484 bytes

Let's see if anything breaks if we change the definition to taking one argument with plain data instead of a render element.

Eric_A’s picture

Assigned: Unassigned » Eric_A
Issue tags: -Needs manual testing

#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.

Eric_A’s picture

Issue summary: View changes

Changed "type" to "variables/ render element".

Eric_A’s picture

Issue summary: View changes

Added issues and the #theme example.

Eric_A’s picture

Issue summary: View changes

Made it clear that this is about theme() not drupal_render().

Eric_A’s picture

Assigned: Eric_A » Unassigned
Priority: Major » Critical
Eric_A’s picture

Issue summary: View changes

changed set to not empty to be accurate.

thedavidmeister’s picture

Issue tags: +Needs manual testing

I'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.

catch’s picture

Assigned: Eric_A » Unassigned
Issue tags: +Needs tests, +Needs manual testing

This 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?

Eric_A’s picture

I 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 and Drupal\system\Tests\Upgrade\ExistingModuleNameLengthUpgradePathTest. See https://qa.drupal.org/pifr/test/573878
Incorporating 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.

Eric_A’s picture

Fixed the link to qa in my previous post. The correct link is https://qa.drupal.org/pifr/test/573878

catch’s picture

Status: Needs review » Reviewed & tested by the community

OK so the tests pass now but will fail with those conversion without this patch applied. In that case this looks RTBC.

pplantinga’s picture

There are several issues blocked on this, so it'd be nice to get it in!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 780bc62 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

drupal_render => drupal_render()