Split off from #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page:Needs to check if $javascript isn't empty before trying to group.

#4 i1554122.patch587 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 35,007 pass(es). View
#1 i1554122.patch4.43 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 34,998 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


attiks’s picture

Status: Active » Needs review
4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 34,998 pass(es). View
nod_’s picture

Status: Needs review » Reviewed & tested by the community

all good

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

Similarly to #1554142: drupal_array_merge_deep_array assumes $array is actually an array, I would think this was the caller's responsibility. The function is documented as taking an array, and doesn't not provide any defaults, so it shouldn't need to check for an array.

Furthermore, if it were to perform the check itself, it might as well as do if (empty($javascript)) { return $groups; } right at the top and not needlessly indent (see this for more on guard clauses).

attiks’s picture

587 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,007 pass(es). View

New patch attached

drupal_group-jss is defined at http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

tim.plunkett’s picture

Maybe I'm just confused, but under what conditions is $javascript empty? And what is the value of $javascripts?

drupal_get_js returns a render array with #items set to at least an empty array, which is passed to drupal_group_js. If the array is empty, the foreach is smart enough to not loop through it.

The only reason this would be needed would be if its not an array at all, which would mean a bug elsewhere.

attiks’s picture

Assigned: Unassigned » attiks
Status: Needs review » Postponed

@Tim drupal_get_js can also return an empty string:

  if (empty($javascript)) {
    return '';

but the current code uses drupal_add_js to get the javascript.

I gonna postpone this until there's a decision in #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page

tim.plunkett’s picture

AFAICS, that return value should be one of the #items, not the only one.

'#type' => 'script',
'#items' => array(
  // each entry in here is the result of drupal_get_js, so it can be an array or ''
  // but #items itself is what drupal_group_js operates on and it should always be an array
sun’s picture

Status: Postponed » Needs review
+++ b/core/includes/common.inc
@@ -4417,6 +4417,9 @@ function drupal_pre_render_scripts($elements) {
 function drupal_group_js($javascript) {

Adding array type-hinting for $javascript can't hurt either.

$javascript == #items, as passed from drupal_pre_render_scripts, which defaults to an array in system_element_info(), but that's nowhere enforced.

nod_’s picture

Version: 8.x-dev » 7.x-dev

The issue is solved another way for D8, relevant if we want to fix the other issue for D7.

mgifford’s picture

Assigned: attiks » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

Ok, then it needs a D7 patch. Not sure if this is still needed though.