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 CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #0.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedsee what the test bots think...
Comment #1.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.1
thedavidmeister CreditAttribution: thedavidmeister commentedupdating summary
Comment #1.2
thedavidmeister CreditAttribution: thedavidmeister commentedupdated issue summary
Comment #2
Fabianx CreditAttribution: 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 CreditAttribution: thedavidmeister commentedBunch of failing tests. My favourite is that an empty string
''
is "rendered" intoNULL
by drupal_render().Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedTests + fix.
Comment #5
widukind CreditAttribution: 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 CreditAttribution: widukind commentedupdating issue summary
Comment #6
thedavidmeister CreditAttribution: 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) $output
thanisset($output)
.Comment #7
Fabianx CreditAttribution: 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 CreditAttribution: Fabianx commentedThis still needs a change notice before it is back to normal for backport :).
Comment #11
David_Rothstein CreditAttribution: 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 CreditAttribution: Jooblay.net commentedWow great work:) Drupal community very impressive:) Showing the rest of us how it done:)
Comment #14
chx CreditAttribution: 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 CreditAttribution: chx commentedComment #16
dcam CreditAttribution: dcam commentedBackported #4 to D7.
Comment #17
Fabianx CreditAttribution: Fabianx commentedSimple patch, tests pass => RTBC
Comment #18
Fabianx CreditAttribution: Fabianx commentedRemoving tag
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/68f6a7c
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedAttempting to add the "7.23 release notes" tag again...
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedRelated #2061835: theme() doesn't enforce that what it returns is a string or FALSE
Comment #22.0
(not verified) CreditAttribution: commentedupdating issue summary