media_include_browser_js() is called by several functions both in media and media gallery module. Looking through it looks like all of those are while building renderable arrays, which means it could likely be changed to media_attach_browser_js(&$element) to add the library and javascript via #attached instead.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 1034838_attached.patch | 4.46 KB | catch |
| #5 | 1034838_attached.patch | 4.18 KB | catch |
| #4 | attached.patch | 4.07 KB | catch |
| #3 | attached.patch | 4.07 KB | catch |
| #1 | attached.patch | 2.01 KB | catch |
Comments
Comment #1
catchAdded the new function and tested with the 'add media' link from media_gallery().
This doesn't yet replace media_include_browser_js() since wyswiyg's hook_INCLUDE_plugin() doesn't appear to allow more than one js file, or libraries. Not really sure what to do about that yet.
Comment #2
catchDouble checked and wysiwyg definitely doesn't allow more than one js file or support libraries at the moment. Will think about a patch to wysiwyg, but don't want to introduce a dependency on that here so this may be the best we can do for now.
Comment #3
catchSlight update, won't be able to test this until tomorrow. Adds a helper for the js definitions, moves the wysiwyg-specific stuff out of there into media.inc. This at least removes the code duplication, all still not very nice.
Comment #4
catchmodule_load_include() arguments always catch me out...
Comment #5
catchUpdated patch, I've now tested this with both the media gallery block and wysiwyg + ckeditor, all appears to be working fine.
Comment #6
David_Rothstein commentedI reviewed this patch and the code looks fine to me. I didn't actually test it out, but since @catch did I think it's fine.
The use of call_user_func_array() looks fragile to me (probably won't work with certain combinations that can theoretically be passed in as #attached?) but that's a reflection of the Drupal APIs for adding JS/attached being kind of a mess, and given that it's just a workaround for the WYSIWYG issue it seems fine for now. Expanded code comments around that point might not be a terrible idea, though.
Comment #7
catchI updated that code comment, and also added an @see for the wysiwyg issue to media_media_plugin(). Since these are only comment changes, leaving at RTBC.
Comment #8
JacobSingh commentedThis looks good to me, I'm going to try and test and commit tomorrow.
Comment #9
JacobSingh commentedI agree w/ David about the call_user_func being brittle, but I also agree that it isn't likely to become an issue ever, so it's mostly academic.
Committed. Nice work :)
Comment #10
KeesMK commentedIt seems after this patch is processed uploading files is not possible anymore.
See also:
http://drupal.org/node/1067298
and
http://drupal.org/node/1067248
Please look into this. Seems to be solved bij changing a little code within the media_gallery.module.
Comment #11
catchThis seems to have been fixed on #1067248: Fatal error after updating Media module.
Comment #12
David_Rothstein commentedComment #13
twodI've created a new issue to deal with this in a way which acually works like '#attached', but it requires a change in Wysiwyg that would greatly benefit from your review. See #1795968: Prepare for wysiwyg_load_includes being cached for the overview.