See attached patch.

The main reason is that this breaks when using jquery_update, which uses a different version because misc/jquery.form.js is not compatible with jquery 1.5+.

Patch is trivial. Not sure if other files could be included as a library as well..

Comments

merlinofchaos’s picture

Oh wow. Yes, yes they absolutely should be included as a library. I must have never updated this code after ksenzee and I got the library patch in.

berdir’s picture

Is there anything that I need to do to get this commited?

Or can I assume that the patch is ok and will be committed once you find the time? :)

robloach’s picture

As a side note, CTools should update to use hook_library() and #attached instead of ctools_add_js() stuff.

merlinofchaos’s picture

The patch is probably okay and will get committed.

Rob: #attached is only appropriate if using drupal_render(). I haven't bought into the render api and I haven't really spent any time converting things to. The area we're looking at isn't being rendered as part of drupal_render(), so I disagree with your assertion.

berdir’s picture

Priority: Normal » Major
StatusFileSize
new2.78 KB

Ok, attaching a newer patch that fixes all other instances as well.

This is IMHO major due to the following problem:

It is possible that a late call to e.g. "drupal_add_js('misc/ajax.js');" overrides the previous definition and adds the file as JS_DEFAULT, at the end of the default list (e.g. if called in a preprocess function as in our case due to a call to ctools_ajax_text_button()). If there are then js files which define ajax commands, this results in Drupal.ajax is not defined for them and similar JS errors in IE which then prevent the JS from being executed.

Yes, I guess most uses of ctools_add_js() could probably be replaced by properly using the library pattern but I'd suggest to do that in a separate issue to get these fixes in asap :)

robloach’s picture

Status: Needs review » Reviewed & tested by the community
merlinofchaos’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed!

Status: Fixed » Closed (fixed)

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

damontgomery’s picture

There are still instances of ctools loading JS libraries directly instead of using the library when using $form['#attached'].

Two examples are ctools/plugins/export_ui/ctools_export_ui.class.php and ctools/page_manager/page_manager.admin.inc. Both use $form['#attached']['js'] = array('misc/jquery.form.js') and should use $form['#attached']['library'][] = array('system', 'jquery.form') as the views does.

I have not looked through everything or tested it, but I think this is important for the same reasons mentioned above. I also am having issues with jquery_update because of this.

Let me see if I can make a patch (I've never done this before).

Set-up:
Drupal 7.12, ctools 7.x-1.x-dev (Jan 23, 2012).

damontgomery’s picture

Status: Needs work » Closed (fixed)
StatusFileSize
new1.91 KB

Here is the patch. Hope this is correct.

Edit: Use patch in Comment #13

damontgomery’s picture

Status: Closed (fixed) » Needs review

Sorry about the multiple comments, learning how to do this. Marking as Needs Review.

berdir’s picture

Status: Needs review » Needs work

Congrats to your first patch ;)

diff --git a/sites/all/modules/ctools/page_manager/page_manager.admin.inc b/sites/all/modules/ctools/page_manager/page_manager.admin.inc
index 1fce8fb..22ecd63 100644
--- a/sites/all/modules/ctools/page_manager/page_manager.admin.inc
+++ b/sites/all/modules/ctools/page_manager/page_manager.admin.inc
@@ -312,7 +312,9 @@ function page_manager_list_pages_form($form, &$form_state) {
     '#attributes' => array('class' => array('use-ajax-submit')),
   );

The patch should be relative to the module directory. (e.g. a/ctools/...) As you seem to be using git, this is easy to fix by using the --relative option to git diff.

git diff --relative=sites/all/modules or something like that.

The patch itself looks good to me.

damontgomery’s picture

Thank you Berdir,

Here is an attached (fixed references) patch. Let's hope it works!

nod_’s picture

Status: Closed (fixed) » Needs review

:)

merlinofchaos’s picture

Status: Needs review » Fixed

Looks good to me! Committed and pushed!

Status: Fixed » Closed (fixed)

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