The drupal_render() function could use a bit more protection

This patch is great as it fixes the errors described above for me (in a different scenario) and probably in a lot of other cases (like well-described e.g. in #2958665: Warning: Invalid argument supplied for foreach() in element_children() /includes/common.inc). Thanks!

Reviewed, patch is pretty small and straightforward, don't see any reasons not to proceed next.

Rerolled patch

+1 for patch in comment #7

+1 for patch in #7, works well

Fixed 'Warning: Invalid argument supplied for foreach() in element_children() includes/common.inc' warnings on Drupal 7.60

+1 for patch in comment #7 as well

as beautiful as it is simple +1

+1 RTBC

+1 RTBC

Patch to be ported.

A bit late, but I just run into this problem and the patch works for me too!

+1 RTBC

Reroll for the latest version of 7.x and also for 7.65 in case anyone else needed it.

Seems to do the job over here.

Not sure I agree with this patch. Every situation I have seen this come up, it's due to some module or custom code not following the rules of how a render array should be generated.

I think it might cause more harm than good by obfuscating the root cause of a problem.

@ron_s I think comment #5 and the link provided there may explain the rationale for this issue and the need for "a bit more protection".

@izmeez, I understand what is being said in #5 and the link to Issue #2958665. I still believe this patch covers up the root cause of why drupal_render has issues in certain cases, and will make debugging more difficult.

@ron_s Your point is good from a developers perspective but from an end use perspective and the number of modules with issues listed in #2958665: Warning: Invalid argument supplied for foreach() in element_children() /includes/common.inc the rationale for the fix in this issue:

The drupal_render() function should act in the same way as it does when an empty render array is passed in or a render array with no '#access' element is passed in. That is return '' (empty string).

makes good sense IMHO. We have been using the patch for sometime.

@ron_s perhaps we can add some logging if the value is not set so at least the issue is recorded

@darrenwh, I think that would be a great idea.

In fact, I just found an issue with the Metatag module's latest 7.x release that is due to a faulty patch. The reason why it was so easy to find the problem is drupal_render starting logging errors to watchdog and the screen. If the patch in this issue had been applied, it would have been far more difficult to find.

See the issue here: https://www.drupal.org/project/metatag/issues/3104933

Re-roll for 7.73

Still no logging added as suggested and discussed in comments #26 and #27.

Updating core target, thanks for the patch!

Thanks @tormi, this issue has been added to #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7, and will be tracked from there, or a future "[meta] priorities for 2021-06-02 release of Drupal 7" issue. So it is probably no longer necessary to update "Drupal 7.xx target" tags in individual issues.

From #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77:

Using the e.g. "Drupal 7.74 target" tags frequently gets messed up by security releases, and it's generally harder to keep track.

Still could use some logging as suggested in #26 and #27.

I think logging something useful seems like a good idea, but what would we actually log?

If $elements is not an array, would we log a message containing whatever else it is (most likely a string)? I am a bit concerned that we'd be adding unnecessary complexity to cater for all eventualities there (e.g. what if it's a not a string? what if it's a really really long string? etc..).

Just a plain message that says "elements wasn't an array" would likely not be very useful other than as a "canary in a coal mine" when other changes are deployed, per #27.

What severity would we use, and what type?

I am leaning towards just committing this extra check as it is, but if anyone wants to add a very simple logging implementation before that happens, patches welcome (but don't wait too long!).

In #26 @darrenwh suggested

perhaps we can add some logging if the value is not set so at least the issue is recorded

Right, but I'm not sure exactly what "the value is not set" means?

There's already an early return if (empty($elements), so I'm not sure I'd add new logging in that situation.

I presume we're talking about if $elements is not an array?

In which case, is it helpful to literally just log that fact (without e.g. trying to log some or all of what $elements actually did contain?)

Personally I agree that it should be better to commit this without additional logging, because that will add more complexity to this simple fix. And mcdruid is right that the $elements variable can consist of different data types, so we would need to take care of all of them.

Also we know how much modules does trigger these warnings, so it should be a good idea to "babysit" that in core (as we have done in some PHP 8 issues already).

Yup, I think we're agreed that #28 can be committed (a unit test might be nice, but it's a simple enough change that I don't think we need to block it on that).

If anyone wants to create a follow-up that adds some useful logging in specific circumstances, please feel free.

In fact it was really easy to add to an existing test.

@poker10 if tests go as we expect, and you're happy you can move this back to RTBC and commit it.

The test looks good, thanks! And the patch passes the tests, so setting this back to RTBC.

Thanks everyone who contributed!