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.

Files: 
CommentFileSizeAuthor
#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 ]

Comments

Status:Active» Needs review
StatusFileSize
new4.43 KB
PASSED: [[SimpleTest]]: [MySQL] 34,998 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

all good

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

StatusFileSize
new587 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...

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.

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

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

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.

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.