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.
Giving theme() a NULL value or an empty string as its second variable causes a fatal error.
Maybe this is intended, but it seems immediately bailing and giving an empty string would be a lot kinder, especially since the value of variables handed to theme functions is something that could potentially change on production, and taking down sites like this is everybody-unfriendly.
Comment | File | Size | Author |
---|---|---|---|
#10 | drupal-theme-system-945908-8-prevent-unsupported-operand-fatal-error.patch | 664 bytes | mlncn |
#2 | drupal-theme-system-945908-2-prevent-unsupported-operand-fatal-error.patch | 465 bytes | mlncn |
Comments
Comment #1
mlncn CreditAttribution: mlncn commentedHere's the call stack - contrived example, if in user.module you call
Comment #2
mlncn CreditAttribution: mlncn commentedThe attached patch sidesteps the unsupported operand fatal error.
***
Related, perhaps. My understanding of #600974: theme() fails for theme functions taking one argument in renderable arrays is that a string is never, ever expected as a parameter of theme(), let alone an empty string. However: A string handed to theme() as a sole render element is turned into an array, $variables, with one value in it– the value of the string keyed to the name given to 'render_element' in our hook_theme() implementation.
This appears to be a magical and unintentional byproduct of this code:
Because
isset($variables['#theme'])
evaluates to true for any string.Maybe we should stop all non-arrays, strings or empty strings or NULL alike, with watchdogged warning before this?
***
To recap, the current patch protects against fatal errors for empty string and NULL parameters passed to theme().
If (or before) this approach is accepted, we should decide whether we are going to:
1) Document that handing a string in turns it into the value for a single-element array, keyed with the name of the render element.
2a) Make it impossible (obviating this patch), or
2b) Tell people not to do this.
3) Pretend it's not there and go on with our merry lives.
Comment #3
furamag CreditAttribution: furamag commentedI have tested #2 patch on Drupal 7 Beta 1. It works for me.
Comment #4
mlncn CreditAttribution: mlncn commentedOK, maybe if i didn't ask at 4am at IRC i'd get a better response for reviews, but furamag has reviewed, this prevents a fatal error from the theme layer, marking RTBC.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe second parameter is an array. Period.
Comment #6
mlncn CreditAttribution: mlncn commentedOK, that's fine, can we catch an exception here and not take down a whole site if something manages to give this a NULL value? Or have theme return nothing in that case?
Really, given all the contexts that theme() is used in, having it cause "Fatal error: Unsupported operand types" is not acceptable, even if giving it a NULL value is never supposed to happen.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf you feel so strongly about it, add an array typehint for the second parameter.
Comment #8
mlncn CreditAttribution: mlncn commentedtypehinting is excellent, but it still returns a site-ending error:
In general i absolutely agree that it is better to cause a fatal error than to silently fail, but theme() is a function that is way too common and handed data in too many ways. If it is given nothing it should return nothing; that's not a silent fail, that's just sensible. That makes more sense than its current behavior, which is to secretly convert strings, but choke on NULLs and empty strings. And it makes more sense to do no error or a softer error for non-arrays than it does to make the third-most common function in Drupal one of the most draconian in its response to unexpected input.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedYep, this function needs to return a fatal error. If you pass the second argument, it *has to* be an array. It is not sensible to silently fail.
Comment #10
mlncn CreditAttribution: mlncn commentedThis patch has theme() immediately return nothing if given nothing.
Comment #11
mlncn CreditAttribution: mlncn commentedEh, crosspost. Still think that a function called thousands of times in unknowable situations should not destroy a site should it get NULL instead of an array.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedBecause it is called thousands of time, we should not make it slower and more convoluted that it is already is. There are plenty of ways to completely destroy a site, and this is clearly doesn't feel like the most common :)
Comment #13
mlncn CreditAttribution: mlncn commentedRudely bumping this to major to get one last review before Drupal 7 is released: What is handed to theme() can change at runtime, unlike most ways of blowing up a site. For the price of an isset() we can have theme('nothing_of_importance', $oops) simply go unprinted instead of causing a WSOD.
I really really hope i'm wrong and never have a chance to point a sobbing siteowner to this issue and say "Hey, i tried."
benjamin, agaric
Comment #14
marcingy CreditAttribution: marcingy commentedIn agreement with Damien we shouldn't be type checking in here if someone passes a none valid value into the function infact providing a fatal error is more helpful that a silent failure. The fatal means that the under lying cause is known about and it get fixed.