Currently, a render() structure supports the properties #attached_js, #attached_css and #attached_library to add Javascript, CSS and libraries respectively.

In this issue #87994: Quit clobbering people's work when they click the filter tips link is being introduced #attached_dialog, to allow attaching 'dialogs' to the page structure.

drupal_process_attached() has a fixed number of parameters, one for each type of 'stuff' that needs to be attached. There should be an easy way to 'attach' other stuff further.

Proposition
Allow dynamic attaching of other types of stuff to render() structures.

Change drupal_process_attached() to receive an array of attachments, including JavaScript, CSS, Libraries, and any other custom type.

Each type, should have its corresponding drupal_add_$type($options) function, which will be called from drupal_process_attached() to add the required stuff to the page.

For example, drupal_add_dialog() for dialogs, drupal_add_js() for Javascript, drupal_add_tabledrag() to tabledrags structures. etc...

Example:

   '#attached_dialog' => array(array(
       'trigger_selector' => ".filter-tips-modal",
       'content_selector' => "#filter-tips-modal-dialog",
       'options' => array(
         'width' => 600,
         'height' => 500,
        ),
    )),
   '#attached_customdynamimcstuff' => array(...),

In the case, the functions drupal_add_dialog() and drupal_add_customdynamimcstuff() should exists, to actually add/attach the stuff.

There is already a patch for this at #87994: Quit clobbering people's work when they click the filter tips link.

Will post a separate patch here to implement this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

@dropcube - nice idea. i would really like to do drupal_process_attached($elements) in drupal_render() and drupal_process_attached($cache->data) in drupal_render_cache_get(). that way all the attached logix stays inside of process_attached. one minor hitch is that drupal_render_cache_set() is a bit of an oddball.

dropcube’s picture

Status: Active » Needs review
Issue tags: +API change, +API addition
FileSize
7.36 KB

Initial patch, it's almost done.

@moshe: In the next patch I am going to move all the logic around the calls to drupal_process_attached() to that function.

dropcube’s picture

Updated patch.

- Moved all the logic around drupal_process_attached() to that function, now drupal_process_attached() can be called with an $elements structured array, and all the processing is done inside the function.
- Added documentation.

----
Follow-up patches to convert drupal_add_tabledrag() to '#attached_tabledrag' can be added, but #564880: Change drupal_add_tabledrag() function parameters to an $options array should be fixed first, because the general approach is that drupal_add_() functions should receive an array of options as the unique parameter, for simplicity, consistency, and better DX.

vangorra’s picture

This will compliment #80855: Add element #type table and merge tableselect/tabledrag into it nicely.

@dropcube: with the possibility of having several 'attached' attributes, it may be foreseeable for modules to be able to work with all the attached elements. What do you think about grouping 'attached' items into a single propertie '#attached', then each key of that array references the config used by the module that provides the attachment?

Example:

   '#attached' => array(
     'dialog' => array(array(
       'trigger_selector' => ".filter-tips-modal",
       'content_selector' => "#filter-tips-modal-dialog",
       'options' => array(
         'width' => 600,
         'height' => 500,
        ),
      )),
     'customdynamimcstuff' => array(...),
  );
dropcube’s picture

@vangorra:

The way you propose, would be simpler to implement, as it would have all the #attached structures together and readily accessible, instead of having to iterate through the entire structure looking for #attached_ properties.

From the developer experience point of view, it seems that a single #attached property with a collection of all #attached_$type properties, also would be easier when altering structures and adding defaults.

So, let's see what others have to say ;)

moshe weitzman’s picture

Yes, I think the vangorra approach is great, since the render cache will automatically work with any new properties that are invented in contrib. But if noone is available to code it, we can proceed with the current patch. Perhaps dropcube will reply stating if he plans to re-code pro proceed with what we have.

dropcube’s picture

Yes, I agree, as I commented in #5. I will work on a patch for it.

RobLoach’s picture

Status: Needs review » Needs work

Nice job, looks pretty good. Couple of things:

+++ includes/common.inc	2 Sep 2009 03:43:54 -0000
@@ -3177,14 +3177,16 @@
 
 /**
- * Adds all the attached libraries, JavaScript and CSS to the page.
+ * Add to the page all structures attached to the element like libraries,
+ * JavaScript, CSS and other types of custom structures. Attached structures
+ * are added to the element using attached properties. The property names for
+ * attached structures begin with #attached_ and the rest if the type of the
+ * structure. For example: #attached_library, #attached_js, #attached_css.
+ * A function named drupal_add_<type> must exists, where <type> is the type of
+ * the structure being addded.

The first line of the function declaration has to be a one line summary. So, "Add to the page all structures attached to the element.", and then a couple lines down, the rest of the description.

+++ includes/common.inc	2 Sep 2009 03:43:54 -0000
@@ -3234,6 +3250,13 @@
+
+  // Add additional types of attachments specified in the render() structure.
+  $attached = array_diff($attached, array('#attached_library', '#attached_js', '#attached_css'));
+  foreach ($attached as $type) {
+    call_user_func_array('drupal_add_'. str_replace('#attached_', '', $type), $elements[$type]);
+  }

I don't think we need this, as those functions are already called previously.

+++ includes/common.inc	2 Sep 2009 03:43:54 -0000
@@ -4409,6 +4431,23 @@
+/**
+ * Get attached properties of a structured array element. Attached properties
+ * begin with '#attached_' and the rest after the underscore is used
+ * for the type of the attached structure. For example: #attached_js,
+ * #attached_css, #attached_library.  
+ */
+function element_attached($element) {
+  return array_filter(array_keys((array) $element), 'element_attached_property');
+}

Could we manually construct the array for this instead? Using array_filter and array_keys would go through all array keys. If we were to just manually construct it checking with issets, it could potentially save us a lot of processing power..... As well as needing element_attached_property.

Beer-o-mania starts in -3 days! Don't drink and patch.

dropcube’s picture

Status: Needs work » Needs review
FileSize
29.36 KB

@Rob Loach: thanks for the review.

Updated patch:
- Fixed function summary docs.
- Removed element_attached() and element_attached_property() as they are not required any more. No we have all the #attached property all together in an array.
- Converted all instances of #attached_js, #attached_css and #attached_library in core, to the new proposed structure.

The #attached property now should contains an associative array, where the keys are the the types of the #attached data, and the value the data. For example:

   $build['#attached'] = array(
     'js' => array(drupal_get_path('module', 'taxonomy') . '/taxonomy.js'),
     'css' => array(drupal_get_path('module', 'taxonomy') . '/taxonomy.css'),
  );

This will allow us to automatically add any attached data to a render() structure. A function named drupal_add_ should exist, which is the function that actually add the attached data to the page. For example: drupal_add_js(), drupal_add_css(), etc...

When a renderable element is cached, all the attached data is persisted in the cache, and processed when the cache is used.

Status: Needs review » Needs work

The last submitted patch failed testing.

dropcube’s picture

Status: Needs work » Needs review
FileSize
29.34 KB

- Fixed exceptions reported by tests.

pwolanin’s picture

It would be nice to use this to be able to add http headers - currently that function has the patterns 'drupal_SET_header' not 'drupal_ADD_header'

Just talked to moshe - let's make the array key the full callback name.

pwolanin’s picture

FileSize
27.16 KB

Here's the smallest delta form the last patch - jsut doesnt' assume anything abotu the function name, but uses the array key as the callback.

pwolanin’s picture

FileSize
27.16 KB

oops - that patch has a syntax error.

pwolanin’s picture

FileSize
27.26 KB

Needs another foreach to be consistent with 'js', etc.

dropcube’s picture

We also need to attach<link> tags, HTTP headers, Feeds, etc. (#567482: Allow <link> tags to be added as attached structures). Using the array key as the callback, seems that we will have to write too much.

It's

$build['#attached'] ['drupal_add_dialog'] = array(..);
$build['#attached'] ['drupal_add_feed'] = array(..);
$build['#attached'] ['drupal_set_header'] = array(..);
$build['#attached'] ['drupal_add_tabledrag'] = array(..);

vs

$build['#attached'] ['dialog'] = array(..);
$build['#attached'] ['feed'] = array(..);
$build['#attached'] ['http_header'] = array(..);
$build['#attached'] ['tabledrag'] = array(..);

Can we consider normalizing the names of functions that can be attached to render() structures ?

pwolanin’s picture

catch’s picture

Status: Needs review » Reviewed & tested by the community

@dropcube - if we do that, then we're potentially preventing patches getting in the next couple of months which take advantage of this functionality, but which don't happen to have drupal_add_$foo() syntax. So I'd rather commit this more verbose version that doesn't require a function naming convention, then if there's time, do the convention later.

pwolanin explained that the current function signatures don't allow us to remove the special cased code for js/css/library just yet, so it makes sense to add this here, then unify those callbacks in the followup patch later - since those will actually break APIs whereas this just extends an existing one.

pwolanin’s picture

@dropbcube - I really, really, don't think we should limit to 'drupal_add' - that's silly. How can any contrib module add a new function then?

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

dropcube’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation

@pwolanin: contribs might implement drupal_add_mymodule_function. Ok, that's ugly and it's not in the module namespace, but they can.

Other approach might be:

foreach ($elements['#attached'] as $key => $options) {
  if (function_exists('drupal_add_' . $key)) {
     $callback = 'drupal_add_' . $key;
  }
  else {
    $callback = $key;
  }

  foreach ($elements['#attached'][$key] as $args) {
      call_user_func_array($callback, $args);
  }
}

First check if drupal_add_$key exists, if not, then use the $key as the callback. This will allow us to use either drupal_add_$key or just the key if drupal_add_key doesn't exists.

catch’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation

Needs upgrade docs.

dropcube’s picture

Status: Fixed » Needs review
Issue tags: +Needs documentation
FileSize
1.05 KB

Attached patch implements approach commented in #21. It will allow us to write:

$build['#attached'] ['dialog'] = array(..);
$build['#attached'] ['feed'] = array(..);
$build['#attached'] ['link'] = array(..);
$build['#attached'] ['tabledrag'] = array(..);

Instead of:

$build['#attached'] ['drupal_add_dialog'] = array(..);
$build['#attached'] ['drupal_add_feed'] = array(..);
$build['#attached'] ['drupal_add_link'] = array(..);
$build['#attached'] ['drupal_add_tabledrag'] = array(..);

Without changing any function signature for now.

RobLoach’s picture

Status: Needs review » Needs work

Although I like this, this patch seems to support both the use cases you provided above. Do we really want to support both at the same time? Would be great to have some code documentation on this.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up

Since we can't and don't want to force contributed modules to implement the 'drupal_add' namespace, this seems to be the only way out.

Tagging.

pwolanin’s picture

Status: Needs review » Needs work

#20 is committed and just needs docs - (hence needs work) is there any other change suggested?

catch’s picture

Priority: Critical » Normal
sun’s picture

.

jhodgdon’s picture

changing tag scheme

jhodgdon’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation updates

It looks like the patch in #15 is what was committed, and what needs update documentation.

What I think it does [issue summary]:

a) When you're adding JS, CSS, or libraries to a render element, the syntax has changed:

-      $build['#attached_css'][] = drupal_get_path('module', 'comment') . '/comment.css';
+      $build['#attached']['css'][] = drupal_get_path('module', 'comment') . '/comment.css';

-    $element['#attached_js'] = array('misc/ajax.js');
+    $element['#attached']['js'][] = 'misc/ajax.js';

-    $element['#attached_library' => array(
-        array('system', 'farbtastic'),
-      )
-    );
+    $element['#attached']['library'][] = array('system', 'farbtastic');

b) The function that does the attach processing changed:

// Signature:
-function drupal_process_attached(array $libraries = array(), array $js = array(), array $css = array(), $weight = JS_DEFAULT, $dependency_check = FALSE) {
+function drupal_process_attached($elements, $weight = JS_DEFAULT, $dependency_check = FALSE) {

// Calling it:
-  drupal_process_attached(
-    isset($elements['#attached_library']) ? $elements['#attached_library'] : array(),
-    isset($elements['#attached_js']) ? $elements['#attached_js'] : array(),
-    isset($elements['#attached_css']) ? $elements['#attached_css'] : array()
-  );
+  // Add additional libraries, CSS, JavaScript an other custom
+  // attached data associated with this element.
+  drupal_process_attached($elements);

However, it looks like this attached_js functionality wasn't present in Drupal 6 anyway, according to:
http://drupal.org/update/modules/6/7#attached_js
This already has the updated syntax.

It also appears that the drupal_process_attached() function didn't exist in Drupal 6.

So I think there's nothing that actually needs update doc here.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.