Currently drupal_render can either return NULL or a string.

This can cause hard-to-debug issues with Twig templates #1975462: Allow and test for NULL and integer 0 values in Twig templates. and probably in other places too. It also seems to just be adding needless extra complexity to a function that's otherwise already quite complicated.

See #1961872: Convert html.tpl.php to Twig for an example.

- A variable to be rendered is created in the preprocess (page_top)
- The variable is set to #access => FALSE by a module's preprocess function (in this case overlay)
- The variable is rendered by drupal_render() in the process phase and becomes NULL instead of a string
- Twig complains "@item could not be found in _context"
- Only solution is to start special-casing any variable that could likely have access checks with (string) type casting or $foo . ''; or to wrap everything in Twig templates with {% if foo %}. This is kinda clumsy/fragile/ugly.

I don't see any advantage to returning NULL instead of '', only disadvantages, to the point where it looks like a mistake. I'm open to discussion/education though :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Status: Active » Needs review
FileSize
1.34 KB

see what the test bots think...

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

updating summary

thedavidmeister’s picture

Issue summary: View changes

updated issue summary

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I would RTBC that right away, but I know you love tests, so: Added "needs tests" tag and set to "needs work".

Thanks! It is a nice patch, I like it!

thedavidmeister’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.55 KB

Bunch of failing tests. My favourite is that an empty string '' is "rendered" into NULL by drupal_render().

thedavidmeister’s picture

FileSize
3.89 KB

Tests + fix.

widukind’s picture

Excellent idea.
I grepped the current 8.x codebase for 'drupal_render(' and got nearly 400 lines back. Glancing over them this seems to hold up, e.g. there are no explicit calls of isset() directly on the output of drupal_render(). However, that may still be the case in following lines of code, but there is easy way of telling.
Still worth moving forward with IMO, esp. considering that the unit tests apparently all passed.

widukind’s picture

Issue summary: View changes

updating issue summary

thedavidmeister’s picture

#5 - it's hard to imagine a situation where you'd be checking isset() on drupal_render() in a useful way since NULL means the input is either:

- empty
- a non empty renderable array with #access => FALSE
- a non empty renderable array with #printed => TRUE
- a non empty renderable array with a #pre_render callback that sets #printed => TRUE

They're all such different things that the NULL becomes more or less meaningless. If there was an isset() somewhere it would almost certainly be a workaround for this bug, more likely though you'd see something like $output . '' or (string) $output than isset($output).

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Passes tests, has added test coverage, fixes a bug, fixes a WTF (drupal_render('') => NULL), unblocks some twig conversions (page.html.twig), is pretty simple.

Well, that is definitely RTBC then :-).

webchick’s picture

Title: drupal_render() should always return a string. » Change notice: drupal_render() should always return a string.
Category: bug » task
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Looks good, and fixes an annoying inconsistency.

Committed and pushed to 8.x. Thanks!

Will need a small change notice.

catch’s picture

Version: 8.x-dev » 7.x-dev
Category: task » bug
Priority: Critical » Normal
Status: Active » Patch (to be ported)

Eek I'd not realised that it doesn't return a string in all cases, good change.

I think this is worth trying to backport- can't see it breaking any existing code, and it could un-break some.

Fabianx’s picture

Category: bug » task
Priority: Normal » Critical

This still needs a change notice before it is back to normal for backport :).

David_Rothstein’s picture

Does this really need a change notice?? (If we're afraid it's going to break something, that would be a reason for a change notice, and if so it would be a reason not to backport this... but I don't see what the meaningful change is here really.)

catch’s picture

I usually move stuff for backport first before change notices - the change notice may or may not have to reference 7.x too.

Jooblay.net’s picture

Wow great work:) Drupal community very impressive:) Showing the rest of us how it done:)

chx’s picture

Category: task » bug
Priority: Critical » Normal

There's no need for a change notice. drupal_render return value is something meant for concat to a string. That's why noone bothered previously: concat a NULL and get an empty string.

chx’s picture

Title: Change notice: drupal_render() should always return a string. » drupal_render() should always return a string.
dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.68 KB

Backported #4 to D7.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Simple patch, tests pass => RTBC

Fabianx’s picture

Issue tags: -Needs change record

Removing tag

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes
David_Rothstein’s picture

Attempting to add the "7.23 release notes" tag again...

thedavidmeister’s picture

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

Anonymous’s picture

Issue summary: View changes

updating issue summary