Ticket #2701 (closed defect: fixed)

Opened 3 years ago

Last modified 2 years ago

Centralize javascript inclusions

Reported by: mdawaffe Assigned to: mdawaffe
Priority: normal Milestone: 2.1
Component: General Version: 2.1
Severity: normal Keywords: javascript bg|has-patch bg|2nd-opinion bg|dev-feedback
Cc:

Description

WP should provide a built in means of including javascript files and their dependencies. This has been discussed on wp-hackers at least twice (most recently: http://comox.textdrive.com/pipermail/wp-hackers/2006-April/005917.html).

The attached patch establishes a method to define and use WP javascript, reworks the admin section to make use of this method, and calls this method both in admin-header.php and in the wp_head hook. The patch keeps track of all scripts and their dependencies, which scripts are slated for inclusion on the page, and which scripts have already been included on the page.

Perhaps a little overkill.

Attachments

2701.diff (12.5 kB) - added by mdawaffe on 05/03/06 21:36:54.
WP_Scripts class
2701b.diff (13.7 kB) - added by mdawaffe on 05/04/06 01:23:27.
Good suggestions from skeltoac
2701c.diff (17.1 kB) - added by mdawaffe on 05/05/06 04:12:56.
absolute links. Better dependencies check
2701d.diff (17.2 kB) - added by mdawaffe on 05/15/06 17:52:48.
Just say no to array_combine(). Fixes bug found by ryanscheuermann.
2701e.diff (17.2 kB) - added by mdawaffe on 05/15/06 17:57:04.
oops
2701f.diff (18.1 kB) - added by mdawaffe on 05/18/06 17:18:15.
script-loader.php
2701g.diff (24.5 kB) - added by mdawaffe on 05/19/06 22:00:42.
Allow parameter passing to scripts
2701h.diff (414 bytes) - added by mdawaffe on 05/30/06 00:39:37.
Fix for call from wp_head
2701i.patch (0.6 kB) - added by arnee on 06/15/06 19:33:13.
Allow plugins to use DBX
2701j.diff (442 bytes) - added by mdawaffe on 07/22/06 22:33:03.
Better fix for wp_head. Replaces 2701h.diff.

Change History

05/03/06 21:36:54 changed by mdawaffe

  • attachment 2701.diff added.

WP_Scripts class

05/03/06 21:40:58 changed by mdawaffe

  • status changed from new to assigned.

Forgot to mention that the patch also adds ?version=$wp_db_version to all the src attributes. A different method of cache busting could easily be swapped in.

05/03/06 23:30:45 changed by skeltoac

Sweet!

On cache busting, I wouldn't count on the db_version bumping the scripts on a bugfix release. How about this: Each script can have its own version number as an optional argument to the add method, then fall back on db_version.

Also, we have TinyMCE accepting ?ver= and passing that on when it loads its scripts and dialogs. So if you're going to change it to version you have to change TinyMCE as well. I'd stick with ver.

05/04/06 01:22:40 changed by mdawaffe

Thanks for the pointer about TinyMCE and ver. I've deprecated tinymce_include() and taken your suggestions for cache busting in the new patch.

2701b.diff

  1. Optional version parameter in the addition functions.
  2. Native scripts have all been given versions. I used real version numbers where provided, real version numbers . _changeset where necessary, and changeset otherwise. I kept the date based version on TinyMCE. Whether we use dates or changesets for anything in the future won't matter. The changesets were easier to find, is all.
  3. src="blah?ver=( $script->ver ? $script->ver : $wp_db_version )"

05/04/06 01:23:27 changed by mdawaffe

  • attachment 2701b.diff added.

Good suggestions from skeltoac

05/04/06 07:39:48 changed by skeltoac

Changesets are better than dates as long as the committer sets them correctly. The patch writer doesn't know what the changeset will be, so should bump to a guessed number and make note in the ticket for the committer.

05/04/06 19:10:03 changed by mdawaffe

As an aside, this offers a nice pluggable way to disable scripts that pose accessibility concerns.

/*
Plugin Name:  AJAX b0rked my screen reader.
*/

add_action( 'wp_print_scripts', 'no_ajax' );

function no_ajax() {
 global $current_user;
 if ( $current_user->data->no_ajax )
  wp_deregister_script( 'sack' );
}

With this in mind, we should rewrite the printer such that it doesn't include JS with missing dependencies. That way, the plugin wouldn't have to try and non-robustly do wp_deregister_script( 'sack', 'ajaxcat', ... );, and the user wouldn't get a lot of JS errors.

05/05/06 04:12:56 changed by mdawaffe

  • attachment 2701c.diff added.

absolute links. Better dependencies check

05/05/06 04:16:13 changed by mdawaffe

2701c.diff

  1. Absolute paths may be needed by some.
  2. Do not load scripts with missing (i.e. unregistered) dependencies.
  3. wp_print_scripts hook should fire first thing since wp_print_scripts() doesn't instantiate $wp_scripts.
  4. Some inline docs for the recursive or otherwise confusing functions.

05/10/06 19:47:38 changed by mdawaffe

  • keywords changed from javascript bg|has-patch bg|2nd-opinion to javascript bg|has-patch bg|2nd-opinion bg|dev-feedback.

05/12/06 22:12:53 changed by ryanscheuermann

I've installed this patch, and I must say, this is some fine work. Definitely digging the dependencies, and the fact that plugins can call wp_print_scripts without worrying about whether or not it's already there.

As far as I can tell, everything seems to be functioning correctly. Still testing it, though.

Maybe one small suggestion: move the WP_Scripts and _WP_Script classes to classes.php?

05/13/06 12:09:18 changed by ryanscheuermann

OK, found a bug: WP_Scripts->all_deps() uses array_combine which is only available in PHP5 :-(

http://us2.php.net/manual/en/function.array-combine.php

05/15/06 17:52:48 changed by mdawaffe

  • attachment 2701d.diff added.

Just say no to array_combine(). Fixes bug found by ryanscheuermann.

05/15/06 17:54:22 changed by mdawaffe

Good call. Thanks.

2701d.diff

  1. Ditch array_combine(). Use something less complicated.

05/15/06 17:57:04 changed by mdawaffe

  • attachment 2701e.diff added.

oops

05/15/06 17:58:17 changed by mdawaffe

As for the best place for the classes, I don't know. classes.php is fine, if that's where people want them.

2701e.diff

  1. Oops: stray var_dump() fixed.

05/16/06 18:12:25 changed by ryan

This only needs to be loaded for the admin, right? I think it only needs to be loaded when admin-header.php is loaded. Maybe add WP_Scripts and friends to wp-admin/script-loader.php and include it from admin-header.php.

05/16/06 18:17:42 changed by mdawaffe

Ideally, people could use it to deal with JS in their themes and plugins as well. It gets called by the wp_head hook.

05/18/06 06:51:49 changed by ryan

Okay. How about putting the class and the wrapper functions into wp-includes/script-loader.php or something like that?

05/18/06 17:18:15 changed by mdawaffe

  • attachment 2701f.diff added.

script-loader.php

05/18/06 17:22:09 changed by mdawaffe

2701f.diff

  1. Move classes and wrappers to wp-includes/script-loader.php
  2. require script-loader.php from wp-settings.php
  3. Move tinymce_include to wp-includes/deprecated.php

05/19/06 21:59:46 changed by mdawaffe

If we allow query string like parameters to be passed through the enqueuers and printers, we can get rid of a lot of the inline JS. Everybody likes browser caching!

2701g.diff

  1. Allow things like wp_enqueue_script( 'script?foo=bar' ) that calls script.js?ver=3000&foo=bar.
  2. Do this for the dbx stuff. Doesn't admin-header.php look pretty now?

This will give plugins a much better means of customizing the admin panels. Currently, a lot of WP's decisions to include or not include various JS bits is not accessible. Also, it's a nice resource for themes and "front-face" plugins.

05/19/06 22:00:42 changed by mdawaffe

  • attachment 2701g.diff added.

Allow parameter passing to scripts

05/20/06 02:58:18 changed by ryanscheuermann

Hey mdawaffe, first test found 1 small bug. dbx-admin-key-js.php needs to add 'link.php' to its test case:

	case 'link-add.php' :
	case 'link.php':
              $man = 'linkmeta';

Otherwise, when I edit a bookmark, I get a DBX error about an invalid session ID.

05/22/06 17:16:42 changed by ryan

05/30/06 00:39:37 changed by mdawaffe

  • attachment 2701h.diff added.

Fix for call from wp_head

05/30/06 00:47:09 changed by mdawaffe

The wp_head hook passes an empty string to wp_print_scripts() which breaks the script loader since various pieces test that value for identicality with false (not equality).

2701h.diff

  1. Fix for call from wp_head.

06/15/06 19:32:00 changed by arnee

Please allow plugins to use DBX. Patch for dbx-admin-key-js.php attached.

06/15/06 19:33:13 changed by arnee

  • attachment 2701i.patch added.

Allow plugins to use DBX

06/15/06 21:01:22 changed by mdawaffe

arnee, I had assumed that plugins would just create their own dbx-key javascript to register and enqueue (de-registering or dequeuing the normal dbx-key if necessary).

I suppose plugins which use the same DBX groupings as the core does (something I had not considered) would benefit from your patch.

07/22/06 22:33:03 changed by mdawaffe

  • attachment 2701j.diff added.

Better fix for wp_head. Replaces 2701h.diff.

07/22/06 22:35:55 changed by mdawaffe

2701j.diff

  1. Fix call from wp_head. A slightly better patch than 2701h.diff.

arnee, I'm not sure of the security implications of allowing arbitrary JS injection (albeit from a restricted set of users).

So -1 to 2701i.diff. Plugins can register their own dbx-key JS as separate files.

08/18/06 05:23:42 changed by ryan

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [4105]) script loader fix from mdawaffe. fixes #2701