Add #attached_library FAPI property for drupal_add_library()

Subscribe.

Definitely.

Here's a patch to start it off. Of course, we're still waiting for #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries).

$form['mystuff'] = array(
  '#type' => 'textfield',
  '#attached_library' => array(
    array('system', 'ui.accordion'),
  )
);

This really shows how powerful the contextual addition of libraries would be.

Just to do what I can.

different then -> different than
rather then -> rather than

Good find! I also added a test for #attached_library, and made use of it in a couple places.

Uhhh, whoops. Let's try that again....

call_user_func
missing () and a period.

It looks like $base = drupal_get_path('module', 'color'); never gets used now in the rest of the function?

i think it is time to create a drupal_process_attached() that gets called from drupal_redner(). In there, we put all the code for #attached handling. Lets keep drupal_render() tidy.

Fixed docs as catch suggested. $base is still being used for #attached_js and #attached_css

I didn't include any of moshe's ideas as I don't know core well enough yet to understand what he would like.

Edit: and apparently it's the devils patch with a 6.66 size so moshe's suggestions must be included quickly. :)

Added a function, like Moshe suggested, which adds the given libraries, JavaScript and CSS. It makes use of it in both drupal_add_library(), and drupal_render(). I went with drupal_process_components() because its not limited to "attached".

Thanks for the function. IMO, 'component' has little meaning. What is the problem with attached? The properties that we are handling are attached_library, atatched_css, attached_js. Why would drupal_process_attached() be undesireable?

Also, I was thinking drupal_render() would just call drupal_process_attached($elements) and leave the rest to drupal_process_attached. This keeps the render cleaner and there is one place where we deal with attached stuff.

This function is also used within drupal_add_library() to process the libraries, JavaScript and CSS so that we don't have druplicate code in core. Doesn't quite make sense to make a full elements array from drupal_add_library with the #attached_css/js/library properties when we could just pass them as separate arguments. But, I have really no problems with the naming of this function so drupal_process_attached() it is. We just can't pass the $elements array directly because it's a common function that's also used with drupal_add_library(), which has no $elements.

This patch does two things:

• Renames the function to drupal_process_attached() like Moshe suggested
• Adds a $jsweight parameter for the default weight of JavaScript being added. JavaScript being added through drupal_add_library() must have a default weight of JS_LIBRARY.

Not all the drupal_process_components were switched over. Fixed in this patch. Testbot lets try again.

bad trial run of new testing bot

Looks good to me. could use another review.

Updated to HEAD and added Vertical Tabs as a library.

This looks good. I'm going to mark this RTBC since the only change in this patch is to move with the offset and fix a mistake in a comment. Functionally it looks good, passes test, and it works.

+++ includes/common.inc	2009-08-25 12:05:46 +0000
@@ -3120,6 +3120,73 @@ function drupal_get_js($scope = 'header'
+ * @param $jsweight
+ *   The default weight of JavaScript that is being added.

We don't abbreviate variable names.

Also, why does only JS get a weight, not also libraries, css, etc?

+++ includes/common.inc	2009-08-25 12:05:46 +0000
@@ -3120,6 +3120,73 @@ function drupal_get_js($scope = 'header'
+ * @param $failsafe
+ *   When TRUE, will exit when an error occurs when adding one of the libraries.
+ *   Defaults to FALSE.
+ * @return
+ *   Will return TRUE on success, and FALSE if there were any troubles adding
+ *   the components.
+ *

I'd like a bit more detail here. What sort of errors occur? Why would it be desirable to default this to FALSE?

Also, does the @return value still return FALSE even if $failsafe is set to TRUE?

Maybe make $failsafe into this $exit_on_error or something, so it's more explicit what this parameter actually does.

+++ includes/common.inc	2009-08-25 12:05:46 +0000
@@ -3120,6 +3120,73 @@ function drupal_get_js($scope = 'header'
+ * @see drupal_add_library
+ * @see drupal_render

Should be @see drupal_add_library(), drupal_render()

The rest of the code I can follow long with pretty well.

I'm on crack. Are you, too?

Thanks for the documentation checks, webchick. Weight on the CSS wasn't in this patch because #92877: Add numeric weight to drupal_add_css wasn't in at the time the patch was made. After applying the weight to the CSS, I realized that both the for loops were the same, so I combined them into one.

The bot likes it, so pushing back to RTBC for webchick to check out.

This looks good to me. Makes me wonder why we have separate weights for JS and CSS when the values are the same. I don't see needing to change that here but it's something we should consider consolidating.

Just a nitpick, where does VT's

+    'version' => '1.5',

come from?

mfer at #27:

Makes me wonder why we have separate weights for JS and CSS when the values are the same. I don't see needing to change that here but it's something we should consider consolidating.

I think this was to avoid bikeshedding on a name for both JavaScript and CSS. These could be consolidated when drupal_add_js/css get merged into drupal_add_component/item/pimp. The constents would then become COMPONENT_SYSTEM, ITEM_DEFAULT, PIMP_THEME, etc.

kika at #28:

Just a nitpick, where does VT's
+ 'version' => '1.5',
come from?

Really not sure, might've been the revision number at the time? Seems it's at 1.6 now. Might be a bit difficult to keep this number up to date with the CVS revision number. Anyone have a suggestion on what else would could use here?

@Rob Loach - do we have to have a version number? If so, and it's drupal internal we could make it 1.0 because that's what it is, isn't it? Or, should the script be released elsewhere and get a version number there?

Sounds good! 1.0 it is. The version number is required so that contrib could update it to a later version if they want.

Nice catch, kika. And thanks for the docs clarifications, Rob Loach! This looks great now.

Committed to HEAD. Please document this in the upgrade guide.

I believe this patch might have introduced a problem with collapsing fieldsets. In issue 559838 chris.cohen reported that the 'Advanced options' fieldset is no longer available during install.

It appears the problem was introduced with this new line of code in includes/form.inc:
$element['#attached_js']['misc/collapse.js'] = array();

This code was introduced in version 1.366 of form.inc in CVS commit 256032 for this issue (505084).

Does this make sense? Let me know if there's anything else I can provide to help. See issue 559838 for further info including a screenshot and a work-around. Thanks.

The collapsible fieldset issue is fixed in HEAD -- see #560944: move collapsible fieldset logic from theme_fieldset hook to form_process_fieldset.

#653068: Update documentation is incomplete

e.g. ajax.inc:

  // Adding the same JavaScript settings twice will cause a recursion error,
  // we avoid the problem by checking if the JavaScript has already been added.
  if (!isset($js_added[$element['#id']]) && isset($element['#ajax']['event'])) {
    $element['#attached']['library'][] = array('system', 'form');
    $element['#attached']['js']['misc/ajax.js'] = array('weight' => JS_LIBRARY + 2);