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.
Comment | File | Size | Author |
---|---|---|---|
#23 | drupal_add-callback-56549-23.patch | 1.05 KB | dropcube |
#15 | dyn-attach-565496-15.patch | 27.26 KB | pwolanin |
#14 | dyn-attach-565496-14.patch | 27.16 KB | pwolanin |
#13 | dyn-attach-565496-13.patch | 27.16 KB | pwolanin |
#11 | dyn-attach-565496-11.patch | 29.34 KB | dropcube |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #2
dropcube CreditAttribution: dropcube commentedInitial 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.
Comment #3
dropcube CreditAttribution: dropcube commentedUpdated 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.
Comment #4
vangorra CreditAttribution: vangorra commentedThis 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:
Comment #5
dropcube CreditAttribution: dropcube commented@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 ;)
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedYes, 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.
Comment #7
dropcube CreditAttribution: dropcube commentedYes, I agree, as I commented in #5. I will work on a patch for it.
Comment #8
RobLoachNice job, looks pretty good. Couple of things:
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.
I don't think we need this, as those functions are already called previously.
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.
Comment #9
dropcube CreditAttribution: dropcube commented@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:
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.
Comment #11
dropcube CreditAttribution: dropcube commented- Fixed exceptions reported by tests.
Comment #12
pwolanin CreditAttribution: pwolanin commentedIt 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.
Comment #13
pwolanin CreditAttribution: pwolanin commentedHere's the smallest delta form the last patch - jsut doesnt' assume anything abotu the function name, but uses the array key as the callback.
Comment #14
pwolanin CreditAttribution: pwolanin commentedoops - that patch has a syntax error.
Comment #15
pwolanin CreditAttribution: pwolanin commentedNeeds another foreach to be consistent with 'js', etc.
Comment #16
dropcube CreditAttribution: dropcube commentedWe 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
vs
Can we consider normalizing the names of functions that can be attached to render() structures ?
Comment #17
pwolanin CreditAttribution: pwolanin commentedfollow-up issue for further clean-up #569280: Remove special-case code in drupal_process_attached() for js and css
Comment #18
catch@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.
Comment #19
pwolanin CreditAttribution: pwolanin commented@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?
Comment #20
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #21
dropcube CreditAttribution: dropcube commented@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:
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.Comment #22
catchNeeds upgrade docs.
Comment #23
dropcube CreditAttribution: dropcube commentedAttached patch implements approach commented in #21. It will allow us to write:
Instead of:
Without changing any function signature for now.
Comment #24
RobLoachAlthough 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.
Comment #25
sunSince 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.
Comment #26
pwolanin CreditAttribution: pwolanin commented#20 is committed and just needs docs - (hence needs work) is there any other change suggested?
Comment #27
catch#653068: Update documentation is incomplete
Comment #28
sun.
Comment #29
jhodgdonchanging tag scheme
Comment #30
jhodgdonIt 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:
b) The function that does the attach processing changed:
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.