JavaScript Patch #1: $options

I've been mulling over Moshes comment about moving to an options array and it being IDE unfriendly. As an IDE user I see he has a point. My IDE tells me what the parameters are. Should we move everything to a $options array?

Don't forget that it needs lots of documentation. :)

I'm +1 for code legibility over IDE friendliness, personally. IDEs will also bring up the PHPDoc for a function, so...

One of these things is not like the others.

+ *   - preprocess
+ *       Should this JS file be aggregated if this
+ *       feature has been turned on under the performance section?

- this one is in the form of a question, which the others are not
- doesn't specify the default setting
- and uses "JS" when everything else uses "JavaScript"
- "has been turned on" can be "is enabled"
- Also, "performance section" may need to be a menu path or URL -- off the top of my head, I'm not sure how we refer to it in other instances, so this may be right.

@keith.smith while I don't disagree that the comment about preprocess could be better it is the same comment describing the argument that is currently used in drupal 6. Any suggestions for the wording?

@Rob Loach this path had 13 failures and 9 exceptions when I ran the tests.

At the beginning of the drupal_add_js function we have:

else {
  $options = array();
}

Do we need to set options to be an array in this else clause? It's default value set in the arguments is an array. If it's not an array the value passed in is turned into a variable on an array. Is there a case when the else would ever be executed?

Why was the if (isset($data)) { moved to before the core js files were added? Previously I could call drupal_add_js() and jquery.js and drupal.js were added to the page. That behavior has been changed and won't happen now.

This is just from a brief review. I'll try to give it a more thorough look later this week.

@Rob Loach I just looked over the if (isset($data)) part again and the reason this comes before and envelops the section that adds the jquery.js and drupal.js is so that when you call drupal_add_js() to get the array containing the javascript that has been added it DOESN'T add any javascript to the page.

If that if statement is moved after the core js files, like the patch does, when drupal_get_js calls drupal_add_js it will add the core js files if they aren't there and we have them added to every page whether we are using them or not.

The attached patch moves the if statement back where it was. Sadly, this change causes more exceptions when running the tests.

Well, I'd propose to leave the $type option as a regular argument instead of an array key in the options array, as the $type significantly changes the general meaning of the function (for $type = 'module' the first argument, $data, is to be a path, while for $type = 'setting' it's an array of variables to be converted to JavaScript, and maybe soon there's also a $type = plugin/library which overloads drupal_add_js yet another time).

So: -1 to squashing the $type into an options array, as it completely controls the meaning of the first argument, and a huge +1 for combining the other arguments in an options array.

@Frando also, please look at this in relation to #315798: JavaScript Patch #2: Weight where type will most likely change to something like file, setting, and inline where it is a true type and not a cross between type and context.

If you review the patch you will see that code like:

drupal_add_js('alert("Hello World!")', 'inline');
drupal_add_js(array('test' => 'me'), 'setting');

still works just like it did in D6.

The patch will load jquery.js and drupal.js on every page whether they are called or not.

This looks better but I'll have to actually test it. Why to we pass NULL in as the second argument to drupal_add_js? Why not require passing array() in instead of NULL?

I reviewed the patch and it looks good and seems to function correctly.

I'm not marking this RTBC until we have tests. The previous patches made attempts at them and the larger issue this is derived from has them. We should add a few tests before marking RTBC.

$options is always set due to the function signature:

+function drupal_add_js($data = NULL, $options = array(), $reset = FALSE) {
   static $javascript = array();
-
+  
+  // Construct the options, taking the defaults into consideration.
+  if (isset($options)) {
+    if (!is_array($options)) {
+      $options = array('type' => $options);
+    }
+  }

Should probably be NULL.

Also, can we leave out the added $reset option, please? It unnecessarily complicates this patch, and I'm yet unsure whether I would agree with Frando.

@sun - what's the issue? The full signature is:

+function drupal_add_js($data = NULL, $options = array(), $reset = FALSE) {
   static $javascript = array();
+  
+  // Construct the options, taking the defaults into consideration.
+  if (isset($options)) {
+    if (!is_array($options)) {
+      $options = array('type' => $options);
+    }
+  }
+  else {
+    $options = array();
+  }

If NULL is passed in the else statement is executed making $options = array(). If array() is passed in the if is executed but it's an array so nothing else happens. If a string is passed in both if statements are executed and the string becomes the value of 'type' on the array.

Am I missing something?

Thanks for the explanation. We do not use such logic in core. Functions that accept an optional, last $reset parameter, use just booleans in the function signature, i.e.

function drupal_add_js($data = NULL, $options = NULL, $reset = FALSE) {

That's possible in this case, too, and will simplify the required logic for treating $options.

This patch changes function declaration to:

function drupal_add_js($data = NULL, $options = array(), $reset = FALSE)

I'm not sure I like the idea of merging $reset into type. That wouldn't be consistent with the rest of core.

I ran the JavaScript tests and they passed. Manually tested several pages and they worked.

Much better now. But:

- A string for the type ('core', 'module', 'theme', 'setting', 'inline') is hard to understand.
- or an array which can have any or all of the following keys (these are not valid with type => 'setting'): not limited to 'setting', I suggest to move the info whether an option is allowed into the description for each option. We also might want to add example code for each possible invocation into the PHPdoc.
- Default values are not quickly graspable. Maybe each option description should start with "Defaults to xyz".

Also, there a some changes in whitespace, which should be removed. The changed/added code also uses leading spaces on blank lines, while blank lines should always be blank.

Some of the added blank lines have spaces on them. These need to be fixed.

Updated the comment block for drupal_add_js. There were references to $type which is now part of the options array. Removed the $reset part of the if (isset($data) || $reset) because we handle that in the logic directly above it. No need for this now. $options['preprocess'] should only default to true if caching is true. If caching is false preprocessing should be false. Updated to reflect this and added a test for it.

Also, update to a moving head.

oops. Why do we have:

  $options['preprocess'] = !$options['cache'] ? FALSE : $options['preprocess'];

Wouldn't it be one less logic argument to have:

  $options['preprocess'] = $options['cache'] ? $options['preprocess'] : FALSE;

If we are setting it here we don't need it in the default options array.

Looks good to me.

Ran tests and looked it over. Looks good. Someone other than Rob Loach or I able to take a look and mark RTBC?

I'm only looking at the tests and I'd like to see those refactored a bit. Since we're doing unit testing rather than functional testing I think you should break the single test function into multiple functions. Each function should focus on a specific aspect of the function's operation: testReset, testInline(), testSettings(), testModule(), testTheme(), etc.

The "kitchen sink" style of tests is common in core right now but in IMHO it's very bad practice. Test should serve two purposes:

• ensure that the code operates in a certain way
• provide a runnable example of the code's usage

Monolithic tests generally fail badly because once one portion of the tests fails it cascades through the later tests. This makes it hard to isolate exactly where the fault lies. By breaking the code up into separate tests you may duplicate some code--though if you're duplicating code between a majority of the tests it should probably go into the setUp() function--but you'll have an isolated test that passes or fails on its own.

You also get several positive side effects by writing smaller tests:

• Each test becomes a simple, self-contained example of how the function can be used, answering the question "Given these inputs what behavior can I expect?"
• Since each test only looks at a single aspect you'll find that you're much more willing to test edge cases. Unlike kitchen sink test you won't have to worry about "breaking up the flow" with an extra test.

Wow, that's starting to sound like something for the handbook.

This patch breaks up the tests as drewish suggests. Also, updates for a moving head.

This patch cleans up a few of the comments in the tests and makes them consistent.

mfer, those look great. makes it much easier to see what's going into the function and happening.

took a bit closer look and noticed a couple of things.

testAddSetting() seems like it should also test for the 'drupal' => 'rocks' setting.

example code should be in the following PHPDoc blocks:

* @code
* // example code goes here...
* @endcode

I don't like that the $options parameter takes either a string or an array... it seems like it should always be an array. if you are going to have it as separate options i'd suggest testing both ways more explicitly.

@drewish - For examples is it better to use @code rather then @example?

mfer, i'm not sure if it really matters but the example that's being added to drupal_add_js() isn't using either.

I took a look at the docs on PHPdoc and @Rob Loach is right. @example references an external file example. @code would be more appropriate for this case.

Subscribing...on my list to review on the train

I've reviewed this, ran the tests and everything seemed fine to me. Committed to CVS HEAD. Thanks all.