Create "scripts" element to align rendering workflow to how "styles" are handled

RTBC, but this will need a change record before commit.

+++ b/includes/common.inc
@@ -4574,9 +4599,15 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    '#markup' => implode('', $processed) . $output,

We really shouldn't introduce #markup, even temporarily. I'm confused with why exactly this needs to be split out. #2990123: Ensure CSS and JS are rendered using the same workflow seems like a more solid solution which doesn't include #markup.

I'm in favor of just closing this as a dup of that issue and focus our efforts there. It's not like this is a lot of code.

Nevermind...

#11 I am in agreement (and will take it in mind for further re-work), but workflow-wise it was simpler to work on it one step at a time.

Yes, I just saw that comment, from one hour ago...

I appreciate that we're finally doing at least something with that issue... but seems like this new direction (which is the first we're all hearing about this) is being snuck in without any real sort of community feedback/review.

Feels a bit shady.

I will note that I'm not disagreeing with this approach.

It certainly will help address the fact that both CSS and JS differ greatly in how they're handled. I've reviewed the patch and it works, aside from the #markup bit. As that will be taken care of by another issue, I'm fine with it I guess since it's already committed.

I just don't appreciate a years-long issue suddenly being taken in a different direction with commits, all within a span of an hour, before the 77+ people actually following the issue have at least had a little time to digest this news.

A week or, at the very least, a day would have been a more appropriate length of time before commits were made.

You are right, I will revert. We will work the theme('html_tag') into this issue.

1. +++ b/includes/common.inc
   @@ -4569,14 +4593,20 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   +  // Keep the order of JS files consistent as some are preprocessed and others
   +  // are not.
   +  // Make sure any inline or JS setting variables appear last after libraries
   +  // have loaded.
   

   I get that this was changed to respect the 80 character limit, but this kind of just irks me. We should just put the second sentence on the second line; no need to have that much whitespace.

2. +++ b/includes/common.inc
   @@ -4569,14 +4593,20 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   +  foreach (array_merge($processed, $output) as $item) {
   +    $elements[] = $item;
   +  }
   

   While this is technically "okay" (keeps order), we lose the real associative keys used by aggregation. Given that this is becoming a #pre_render callback, we shouldn't obfuscate this in case contrib/custom code needs/wants to alter this in additional #pre_render callbacks.

   I honestly preferred the approach and variable renames in #2990123: Ensure CSS and JS are rendered using the same workflow here.

#20 1,2: Agree 100%.

I honestly preferred the approach and variable renames in #2990123: Ensure CSS and JS are rendered using the same workflow here.

Yes, we can bring part of that back.

- $output -> $settings -- yes
- $processed -> $items -- yes

As for the #pre_render issue:

What about we return two keys:

$elements += array(
  'scripts' => $items,
  'settings' => $settings,
);

Ah, didn't catch that.

I like the idea of adding two new child keys to the element. The only drawback is making sure scripts (which should also be the variable name IMO, not $items) is rendered before settings. We should probably add '#weight' => 1 to $settings to ensure this.

1. +++ b/includes/common.inc
   @@ -4569,14 +4595,18 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   +  return $elements + array(
   

   Also, now that I think about this, I don't think we want to risk introducing another Drupal WTF moment here.

   What if, someone, supplies these keys (for whatever reason) prior to this being invoked. These won't get added because the keys already exist.

   I think this should use any existing children as a default starting point (will likely need to use this as the starting $index) and then definitively set the children on the element at the end.

2. +++ b/includes/common.inc
   @@ -4569,14 +4595,21 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   +      'settings' => array(
   +        '#weight' => 1,
   +        $settings,
   +      ),
   

   This looks... odd. A simple $settings['#weight'] = 1; at the top would suffice.

@Pol, see #30.

#30 I am not sure I agree as we introduce this element newly here.

I think the sub-keys are fine to use, however I suggest we use #weight => 0 for settings and, #weight => 10 for 'scripts'.

If contrib / custom wants to add before 'settings', they can use a negative weight.

If they want to add it in between, they can use a weight between 0 and 10, etc.

What do you think?

+++ b/includes/common.inc
@@ -4473,34 +4509,24 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // The index counter is used to keep aggregated and non-aggregated files in
+  // order by weight.
+  $index = 1;
+  $files = array();
+  $settings = array();
+  $scripts = array();

I was thinking something a little simpler:

$files = array();

$scripts = isset($element['scripts']) ? $element['scripts'] : array();
if (!isset($scripts['#weight']) {
  $scripts['#weight'] = 0;
}

$settings = isset($element['settings']) ? $element['settings'] : array();
if (!isset($settings['#weight']) {
  $settings['#weight'] = 10;
}

// The index counter is used to keep aggregated and non-aggregated files in
// order by weight. Use existing script count as a starting point.
$index = count(element_children($scripts)) + 1;

And then just explicitly set them at the end:

$element['scripts'] = $scripts;
$element['settings'] = $settings;

return $element;

edit: accounting for new weights and counting element children on $scripts

#30 I am not sure I agree as we introduce this element newly here.

True, but it's also possible for hook_element_info_alter() to allow callbacks to be prepended prior to this being invoked. If some contrib/custom code already populates this, then this needs to account for that.

If this were staying internal (like it was) then I wouldn't have an issue with the current approach.

However, given that this is becoming a #pre_render callback this means it's being opened up to the entire system that is the "Render API".

It needs to follow the paradigm of allowing the possibility of #pre_render callbacks being invoked prior to this one (because contrib/custom code can inject their own, before cores array_unshift()) and populate similar keys/properties.

However, if a previous pre_render modifies the weight of scripts and set a value higher than 10, then it will break.

True, yes, it should be $settings['#weight'] = $scripts['#weight'] + 10;

#39 I agree, but I think it's feasible that we reserve the right to exclusively set 'scripts' and 'settings' keys in our #pre_render callback return.

Would it not be also a kinda WTF if the default settings would not be there anymore once you set 'settings' in your pre-pre-render callback?

Also we wanted the possibility to change the original $keys, so I think preserving that has a priority?

we reserve the right to exclusively set 'scripts' and 'settings' keys

No, that went out the window the moment this became a #pre_render callback in the render system.

What happens to core callbacks that don't work with contrib/custom code? They get replaced with duplicate code that fixes the code so it can work with prior callbacks.

I'm not saying that I have a solid use-case here, other than it is now technically possible and to ignore that would be extremely shortsighted.

Would it not be also a kinda WTF if the default settings would not be there anymore once you set 'settings' in your pre-pre-render callback?

This is exactly what I'm saying the WTF is and why #35 is a must for a #pre_render callback.

If we check for these children and use them as a starting point (instead of an empty array), we're actually appending new data to existing data (from a pre-pre-render callback) and then explicitly [re]setting the child elements at the end.

To clarify, #33 and #35 are two different approaches.

#33 creates empty array variables and ignores any pre-pre-render children set

#35 creates array variables from existing children of a pre-pre-render, if set, or an empty array if not set

To further clarify:

#33 re-loops over the items unnecessarily and re-introduces the issue I brought up in #20.2 by re-indexing array items and losing any aggregated associative keys.

While #33 does technically "add" using the array default method and then subsequently iterates over everything to add it to the element, this does not constitute the "same result" as #35 and why I consider them vastly different approaches.

A little cleaner using the default array method for the weights:

$files = array();

// Assign scripts and settings.
$scripts = isset($element['scripts']) ? $element['scripts'] : array();
$settings = isset($element['settings']) ? $element['settings'] : array();

// Assign default weights.
$scripts += array('#weight' => 0);
$settings += array('#weight' => $scripts['#weight'] + 10);

// The index counter is used to keep aggregated and non-aggregated files in
// order by weight. Use existing script count as a starting point.
$index = count(element_children($scripts)) + 1;

+++ b/includes/common.inc
@@ -4447,12 +4447,48 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * #pre_render callback to add the elements needed for scripts tags to be
+ * rendered.

One tiny doc nit after re-reading the entire thing again. This should start with a normal word/capital letter and a single line according to docs docs.

+++ b/includes/common.inc
@@ -4542,7 +4573,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
-          $processed[$key] = '';
+          $scripts[$key] = '';

I think this will break - I just noticed it.

Would that be an empty render array child or empty markup?

Ohhh - I see now that as '' == [] => empty('') == TRUE, so this will work fine.

Even if it looks a little bit strange.
---

Marking for commit. The plan would be to commit this within the next week or so.

Great work!

Thanks for the explanation, Mark. I finally got how you wanted to structure this.

I agree that the solution that we now have is nicely documented and allows easy extending things.

++

+++ b/includes/common.inc
@@ -4447,12 +4447,48 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * @param array $elements
+ *   A render array containing:
+ *   - '#items': The JS items as returned by drupal_add_js() and altered by
+ *     drupal_get_js().
+ *
+ * @return array
+ *   A render array that will render to a string of XHTML "<script>" tags.

Hmmmm - thinking about this a little bit I think we should explain the returned structure and also show what can be passed in, in here.

I think this is good to go now again, but leaving up for feedback from Mark and potentially others.

Planned commit of this is within 1 week.

LGTM