Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.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 :)
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 1975442-16-drupal-render.patch | 3.68 KB | dcam |
| #4 | 1975442-4.patch | 3.89 KB | thedavidmeister |
| #3 | 1975442-failing-tests-only-3.patch | 2.55 KB | thedavidmeister |
| #1 | 1975442-1.patch | 1.34 KB | thedavidmeister |
Comments
Comment #0.0
thedavidmeister commentedUpdated issue summary.
Comment #0.1
thedavidmeister commentedUpdated issue summary.
Comment #1
thedavidmeister commentedsee what the test bots think...
Comment #1.0
thedavidmeister commentedUpdated issue summary.
Comment #1.1
thedavidmeister commentedupdating summary
Comment #1.2
thedavidmeister commentedupdated issue summary
Comment #2
fabianx commentedI 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!
Comment #3
thedavidmeister commentedBunch of failing tests. My favourite is that an empty string
''is "rendered" intoNULLby drupal_render().Comment #4
thedavidmeister commentedTests + fix.
Comment #5
widukind commentedExcellent 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 ofdrupal_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.
Comment #5.0
widukind commentedupdating issue summary
Comment #6
thedavidmeister commented#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) $outputthanisset($output).Comment #7
fabianx commentedPasses 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 :-).
Comment #8
webchickLooks good, and fixes an annoying inconsistency.
Committed and pushed to 8.x. Thanks!
Will need a small change notice.
Comment #9
catchEek 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.
Comment #10
fabianx commentedThis still needs a change notice before it is back to normal for backport :).
Comment #11
David_Rothstein commentedDoes 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.)
Comment #12
catchI usually move stuff for backport first before change notices - the change notice may or may not have to reference 7.x too.
Comment #13
Jooblay.net commentedWow great work:) Drupal community very impressive:) Showing the rest of us how it done:)
Comment #14
chx commentedThere'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.
Comment #15
chx commentedComment #16
dcam commentedBackported #4 to D7.
Comment #17
fabianx commentedSimple patch, tests pass => RTBC
Comment #18
fabianx commentedRemoving tag
Comment #19
David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/68f6a7c
Comment #20
David_Rothstein commentedAttempting to add the "7.23 release notes" tag again...
Comment #21
thedavidmeister commentedRelated #2061835: theme() doesn't enforce that what it returns is a string or FALSE
Comment #22.0
(not verified) commentedupdating issue summary