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

CommentFileSizeAuthor
#8 2061835-8.patch1.69 KBthedavidmeister
#5 2061835-5.patch1.65 KBstar-szr
#5 interdiff.txt867 bytesstar-szr
#3 2061835-3.patch1.65 KBthedavidmeister
#2 2061835-2.patch1.32 KBthedavidmeister
#1 2061835-1.patch334 bytesthedavidmeister

Comments

thedavidmeister’s picture

thedavidmeister commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 2061835-1.patch334 bytes

let's see if this breaks any existing tests...

Just setting to CNR to see testbot output, it needs tests before it can be RTBC.

thedavidmeister’s picture

thedavidmeister commented
StatusFileSize
new 2061835-2.patch1.32 KB

failing tests.

thedavidmeister’s picture

thedavidmeister commented
Issue tags: -Needs tests +theme system cleanup
StatusFileSize
new 2061835-3.patch1.65 KB

tests + fix.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Nice find, this makes this more conistent, too, has tests => RTBC

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new interdiff.txt867 bytes
new 2061835-5.patch1.65 KB 
@@ -58,6 +58,24 @@ function testAttributeMerging() {
+    foreach($foos as $type => $example) {

Need a space between the foreach and (.

Fixing here since it's so minor.

thedavidmeister’s picture

thedavidmeister commented

thanks

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +Approved API change 
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
@@ -58,6 +58,24 @@ function testAttributeMerging() {
+      $this->assertTrue(is_string($output), 'theme() returns a string for data type ' . $type);

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

thedavidmeister’s picture

thedavidmeister commented
Status: Needs work » Needs review
StatusFileSize
new 2061835-8.patch1.69 KB

Using format_string() now.

dawehner’s picture

dawehner
Primary language German
commented

I am wondering why we are not give the developers any hint that something is wrong.

thedavidmeister’s picture

thedavidmeister commented

#9 - because we never have before? Is that something we should be doing here?

dawehner’s picture

dawehner
Primary language German
commented

It is always a clear bug if your theme function does not return a string, right? This would improve the DX so why not?

fabianx’s picture

fabianx commented

Unfortunately the is_string function call would probably cost quite a lot in terms of performance ...

I would be okay with it though :-).

thedavidmeister’s picture

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.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: theme() doesn't enforce that what it returns is a string or FALSE » Change notice: theme() doesn't enforce that what it returns is a string or FALSE
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 85963a7 and pushed to 8.x. Thanks!

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
@@ -58,6 +58,24 @@ function testAttributeMerging() {
+  function testThemeDataTypes() {

Feel free to provide a visibility and use String::format instead of format_string.

catch’s picture

catch
he/him
Primary language English
commented
Category: bug » task
catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Major

#2083415: [META] Write up all outstanding change notices before release.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review

https://drupal.org/node/2084443

I didn't really know what else should be written.

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Fixed

seems ok.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Change notice: theme() doesn't enforce that what it returns is a string or FALSE » theme() doesn't enforce that what it returns is a string or FALSE
Priority: Major » Normal

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.