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

Files: 
CommentFileSizeAuthor
#16 1975442-16-drupal-render.patch3.68 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,347 pass(es).
[ View ]
#4 1975442-4.patch3.89 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 54,887 pass(es).
[ View ]
#3 1975442-failing-tests-only-3.patch2.55 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 54,910 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#1 1975442-1.patch1.34 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 54,772 pass(es).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Active» Needs review
StatusFileSize
new1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 54,772 pass(es).
[ View ]

see what the test bots think...

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

updating summary

Issue summary:View changes

updated issue summary

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!

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new2.55 KB
FAILED: [[SimpleTest]]: [MySQL] 54,910 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new3.89 KB
PASSED: [[SimpleTest]]: [MySQL] 54,887 pass(es).
[ View ]

Tests + fix.

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.

Issue summary:View changes

updating issue summary

#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).

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 :-).

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.

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.

Category:bug» task
Priority:Normal» Critical

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

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

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

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

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.

Title:Change notice: drupal_render() should always return a string.drupal_render() should always return a string.

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.68 KB
PASSED: [[SimpleTest]]: [MySQL] 40,347 pass(es).
[ View ]

Backported #4 to D7.

Status:Needs review» Reviewed & tested by the community

Simple patch, tests pass => RTBC

Issue tags:-Needs change record

Removing tag

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

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

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

Issue summary:View changes

updating issue summary