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
We let theme functions return whatever they want, but drupal_render() really relies on being able to test if theme() returns FALSE or a string for it to function correctly.
If we enforce that the return of theme() is always a string if a hook is implemented or FALSE otherwise we can avoid potential issues caused by the return of a theme function being different to what we expect.
Proposed resolution
Replace:
return $output;
With:
return (string) $output;
in theme()
Comment | File | Size | Author |
---|---|---|---|
#8 | 2061835-8.patch | 1.69 KB | thedavidmeister |
#5 | 2061835-5.patch | 1.65 KB | star-szr |
#5 | interdiff.txt | 867 bytes | star-szr |
#3 | 2061835-3.patch | 1.65 KB | thedavidmeister |
#2 | 2061835-2.patch | 1.32 KB | thedavidmeister |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedlet's see if this breaks any existing tests...
Just setting to CNR to see testbot output, it needs tests before it can be RTBC.
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedfailing tests.
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedtests + fix.
Comment #4
Fabianx CreditAttribution: Fabianx commentedNice find, this makes this more conistent, too, has tests => RTBC
Comment #5
star-szrNeed a space between the foreach and (.
Fixing here since it's so minor.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedthanks
Comment #7
webchickPretty sure that should be using format_string(), rather than string concatenation.
This is an API change, but makes sense to me, and hopefully will have little impact on module developers (if anything, it's bringing the function inline with what the docs say it does). Marking as approved.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedUsing format_string() now.
Comment #9
dawehnerI am wondering why we are not give the developers any hint that something is wrong.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commented#9 - because we never have before? Is that something we should be doing here?
Comment #11
dawehnerIt is always a clear bug if your theme function does not return a string, right? This would improve the DX so why not?
Comment #12
Fabianx CreditAttribution: Fabianx commentedUnfortunately the is_string function call would probably cost quite a lot in terms of performance ...
I would be okay with it though :-).
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commented*groan* can we avoid doing anything that might be a perf hit here? I was trying to keep the scope of this patch as tiny as possible.
Comment #14
Fabianx CreditAttribution: Fabianx commentedIn that case as we never had that message to Devs before, RTBC of #8.
A display of error message could be discussed as a follow-up. Lets fix the bug first.
Comment #15
alexpottCommitted 85963a7 and pushed to 8.x. Thanks!
Comment #16
dawehnerFeel free to provide a visibility and use String::format instead of format_string.
Comment #17
catchComment #18
catch#2083415: [META] Write up all outstanding change notices before release.
Comment #19
dawehnerhttps://drupal.org/node/2084443
I didn't really know what else should be written.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedseems ok.
Comment #21
star-szrComment #23
xjmUntagging. Please remove the tag when the change notification task is completed.