Based on further discussions at http://groups.drupal.org/node/15388 and related to #315035: Add jQuery UI elements to core, I propose we move the jQ module into core. This would allow administrators to easily and quickly determine what jQuery plugins are installed, and their versions. It also would allow them to turn on or off certain plugins from the same screen.

The API would allow module maintainers to specify supported plugins with a hook_jq function in a MODULE_NAME.jq.inc file. Modules would invoke plugins with jq_add('plugin-name', ...). (We could simplify this to simply jq() if desired).

I'll roll out a patch tomorrow, and add some screen shots and instructions for testing.

CommentFileSizeAuthor
#285 drupal.library-api.patch1.41 KBsun
#273 drupal.library.267.patch27.25 KBRobLoach
#270 library.diff26.44 KBmoshe weitzman
#269 drupal.library.267.patch27.25 KBsun
#267 315100.patch28.52 KBRobLoach
#266 315100.patch29.49 KBRobLoach
#264 315100.patch26.38 KBRobLoach
#262 315100.patch26.47 KBRobLoach
#255 drupal.library.255.patch26.88 KBRobLoach
#253 drupal-315100-253.patch26.98 KBRobLoach
#251 drupal.library.251.final_.patch27.33 KBsun
#247 drupal.library.247-still-cached.patch28.39 KBsun
#245 315100.patch28.03 KBRobLoach
#238 drupal.library.238.patch31.28 KBsun
#233 drupal.library.233.patch30.12 KBsun
#232 drupal.library.232.patch30.03 KBsun
#231 drupal.library.patch29.03 KBsun
#227 315100.patch32.03 KBRobLoach
#221 315100.patch31.2 KBRobLoach
#221 Accordion Screenshot90.64 KBRobLoach
#209 component.php.txt3.58 KBkkaefer
#205 315100.patch24.36 KBRobLoach
#193 315100.patch22.26 KBRobLoach
#187 fail.php.txt4.56 KBsun
#184 315100.patch18.05 KBRobLoach
#182 315100.patch18.91 KBRobLoach
#178 315100.patch18.93 KBRobLoach
#171 315100.patch17.96 KBRobLoach
#169 315100.patch17.54 KBRobLoach
#163 315100.patch16.97 KBRobLoach
#158 315100.patch16.87 KBquicksketch
#157 315100.patch16.87 KBquicksketch
#152 315100.patch17.07 KBquicksketch
#150 315100.patch17.04 KBquicksketch
#149 315100.patch17.21 KBquicksketch
#148 315100.patch17.21 KBquicksketch
#142 315100.patch15.3 KBquicksketch
#127 315100.patch12.63 KBRobLoach
#123 315100.patch11.25 KBRobLoach
#117 315100.patch13.61 KBRobLoach
#113 Pluign Requirements Screenshot59.07 KBRobLoach
#113 drupal-315100.patch13.86 KBRobLoach
#105 drupal-315100.patch13.36 KBRobLoach
#104 drupal-315100.patch14.65 KBRobLoach
#99 javascript_libraries.zip13.04 KBskilip
#87 315100.patch14.24 KBquicksketch
#86 315100.patch14.23 KBRobLoach
#80 libraries_list.png72.55 KBskilip
#80 libraries_configure.png106.86 KBskilip
#79 315100.patch12.14 KBRobLoach
#78 315100.patch11.07 KBRobLoach
#77 315100.patch11.71 KBRobLoach
#34 315100.patch7.51 KBRobLoach
#34 jquery_ui2.tar_.gz447.66 KBRobLoach
#34 jqueryui.png48.2 KBRobLoach
#23 jquery.plugins.315100.patch7.02 KBaaron
#16 315100.patch3.19 KBRobLoach
#16 jquery_ui.tar_.gz446.64 KBRobLoach
#6 jq.patch5.78 KBaaron
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Title: Move jQ into Core » Centralized jQuery plug-in manager

I'd rather not settle on one solution, and rather build general consensus among the folks who are JavaScript-knowledgeable. But if you want to kick this issue off by making the case for jQ, by all means. :)

Fixing title to be more appropriate.

aaron’s picture

There isn't any javascript in the solution, other than including a js file when invoked. It's just a manager.

Thinking about it last night, I realized that jq isn't a good name for something in core. Keeping w/ Drupal standards, it should be 'plugin' or 'jquery_plugin'. I'll work in some changes, and look at Plugin as well, maybe merge the best qualities of both.

RobLoach’s picture

I've never really understood the point of having a visual interface to see what jQuery plugins are available because all the modules just provide the required jQuery plugins anyway. The user shouldn't have to worry about this stuff, the developer who is writing the Drupal module/theme should.

Stating that, a hook_jq would be great for programmatically managing where the jQuery plugins are, and making sure the desired/latest version of it is included when you want to add the plugin. Instead of a jq_add() function, however, I suggest running with drupal_add_js and drupal_add_css and calling it drupal_add_jq().

I set #315035: Add jQuery UI elements to core as postponed here until the jQuery Plugin registry goes in.

redndahead’s picture

maybe drupal_add_js('color_picker', 'plugin');

With this you could also do: drupal_add_js('/path_to_module/color_picker.js', 'plugin');

Here is the logic that I am thinking. Please tell me if I'm way off.

Let's say the first call is drupal_add_js('color_picker', 'plugin');
It would check to see if the $data ends with .js.
If it doesn't it would check the drupal jQuery plugins folder for 'color_picker/color_picker.js';
If it exists then it would add it to a $javascript['plugin']['color_picker'] = array('path' => NULL);

Then if a subsequent call was drupal_add_js('/path_to_module/color_picker.js', 'plugin');
It would then check for the js extension. Since it is there it assumes that it is a path.
It would then get the file name. In this case color_picker.
Then assign to $javascript['plugin']['color_picker'] = array('path' => '/path_to_module/color_picker.js')

This would effectively replace the one that is included.

Thoughts?

RobLoach’s picture

Yup! It would be similar to the jQ module. Every time you have a jQuery Plugin available, you'd implement hook_jquery_plugins (or hook_jq or something). In the case of the jQuery UI's Color Picker being added to the system module for core, it could look something like this:

/**
 * Implementation of hook_jquery_plugins().
 */
function system_jquery_plugins() {
  return array(
    'ui.core' => array(
      'name' => t('jQuery UI'),
      'version' => t('1.6 RC2'), // Maybe 1.62 so that we can do version comparisons to force the latest version to be added everytime?
      'files' => array(
        'js' => array('misc/ui/ui/ui.core.js'), // or misc/ui/ui/packed/ui.core.packed.js if we have JS performance enabled
        'css' => array('misc/ui/themes/default/ui.all.css'), // or a different jQuery UI theme if our Drupal theme asks for it?
      ),
    ),
    'ui.colorpicker' => array(
      'name' => t('Color Picker'),
      'version=> t('1.6 RC2'),
      'files' => array(
        'js' => array('misc/ui/ui/ui.colorpicker.js'),
      ),
      'dependencies' => array('ui.core'),
    ),
  );
}

And then if you wanted to use the Color Picker, you would call drupal_add_jq('ui.colorpicker'); and it would add the required JavaScript files and CSS files, as well as the plugin dependencies. We could add it to drupal_add_js, but I'd rather wait on #251578: More flexible js/css ordering and an alter operation and use drupal_add_jq in the mean time. The good thing about this is that when we are building the registry, we could switch files we're using depending on whether we want to use the packed JavaScript files, or the uncompressed normal ones.

aaron’s picture

Status: Active » Needs review
FileSize
5.78 KB

Here's the first iteration of a patch. Needs testing. I'll post ideas of how to test the patch later. Rob has a good start above.

aaron’s picture

Differences between this and jQ: uses hook_jquery_plugins rather than hook_jq to define new plugins. The administrative screens have been removed. (Note however you can still manually disable plugins still w/ variable_set('jq_allow_'. $plugin, FALSE); which should be handy for future jQ UI modules, such as jQ or Plugin). plugins are invoked on a page with drupal_add_jq, rather than jq_add. Otherwise it's the same. This patch probably needs better documentation. I'll look later.

webchick’s picture

What is the use case for disabling a plug-in that a module needs? Won't that bust everything?

aaron’s picture

might be useful for debugging a bad plugin. also, might want to disable a plugin if a page takes too long to load?

although i'm stretching it really. we can easily remove that ability if it doesn't really make sense. i just added it for completeness. plus jQ allows admins to disable plugins from a screen, but i don't know of anyone who's actually ever used that functionality.

aaron’s picture

as far as busting things, really just depends on how it's being used. it would be the same as javascript being disabled on a page (though just for that plugin). in most cases, drupal degrades gracefully, and jquery does for the most part as well. though it might mess up the layout in some cases.

webchick’s picture

If these are part of the normal $scripts variable it seems like people could use the usual preprocess functions to remove ones that were offensive? If so, I would prefer that to a separate variable-based mechanism for disabling *some* JS but not others. However, I *definitely* don't think we should expose a UI for it.

bensnyder’s picture

Hmmm... Well there are certainly a lot of great ideas here.... But aren't there hundreds of JQuery modules out there??
(( http://plugins.jquery.com/ ))

It would be very difficult to have .inc files for all of them AND keep it up-to-date... Is this idea feasible??:

>> Add this directory: sites/all/scripts <<

The JQuery website is built on Drupal. Can we also have an update manager for the scripts just like we do for modules and themes?

there's my 2 cents :))

aaron’s picture

both of those are great ideas. we'd probably want to talk w/ the jquery folks about the second idea.

aaron’s picture

looking at http://drupal.org/project/jqp -- it might be a better candidate for core

redndahead’s picture

I think what we should maintain is whatever is used in core and maybe all of jquery UI since it is an official jQuery package that has a version we can reference.

RobLoach’s picture

FileSize
446.64 KB
3.19 KB

Whoops, I didn't check the issue before too see if a patch was already around before I hacked away. The patch includes parts of the jQ module (considered a small rewrite) with a rename of the functions to drupal_get_jq() and drupal_add_jq().

The attached archive (jquery_ui.tar_.gz) is a module implementing hook_jquery_plugins() for jQuery UI. You can see it in action if you apply the patch, enable the module, and visit example.com/jquery_ui . Sorry about that one, Aaron! I'll look at your patch and see if I can adopt this module to it or something.....

aaron’s picture

i'm also refactoring to also scan /plugins ala jqp. that way we get the best of both worlds

jjeff’s picture

Hi all! You're all quicker than me!

Yeah, I'd been toying around with a solution that I thought might be core-worthy. The result is http://drupal.org/project/jqp

It's simple and lightweight. But yes, it does need a caching solution. It should be easy enough to stick a cache_get() and cache_set() into the jqp_scan_dir() function. It'll probably also need a hook_flush_caches() as well.

aaron’s picture

jjeff - we decided to have the best of both worlds. because there are many times you want what jq offers (loading multiple files w/ a single call, maybe tying css to theme at some point, version control, etc), and other times a developer will want to just drop a file in a dir ala jqp, we're going to refactor to combine the ideas. i just about have jq patched to do that now, and will then refactor the patch accordingly. thanks for your work on that!

RobLoach’s picture

Thoughts on patch at #6:

  1. Rename jq_plugins() function to drupal_get_jq() to follow the drupal_add_*/drupal_get_* naming convention. _jq_plugins could also become _drupal_get_jq()
  2. return $plugins[$plugin]; causes a code warning if the plugin isn't found in $plugins......... Replace with: return isset($plugins[$plugin]) ? $plugins[$plugin] : NULL;
  3. It doesn't check for dependancies which is absolutely required for jQuery UI......
    +        if (isset($jq['dependencies']) && is_array($jq['dependencies'])) {
    +          foreach ($jq['dependencies'] as $dependency) {
    +            _drupal_add_jq($dependency);
    +          }
    +        }
            if (is_array($jq['files']['js'])) {
              foreach ($jq['files']['js'] as $file) {
                drupal_add_js($file);
              }
            }
            if (is_array($jq['files']['css'])) {
              foreach ($jq['files']['css'] as $file) {
                drupal_add_css($file);
              }
            }
    
  4. When there's a plugin naming conflict, if we just default to using the latest version of the plugin conflicts, it will mean that we could easily have webchick's contrib jQuery UI module provide an easy upgrade path for future versions of jQuery UI.
  5. +        // On conflicting plugin names, use the latest version.
    +        if (isset($plugins[$name]) && isset($plugins[$name]['version'])) {
    +          if (!isset($details['version']) || !is_numeric($details['version']) || $details['version'] < $plugins[$name]['version']) {
    +            // Attempting to add an older plugin version.
    +            continue;
    +          }
    +        }
    
  6. I think that most instances of "jq" should become a full "jquery". jquery.inc, jquery.cache.inc, etc. The function names are fine though.
  7. module_invoke_all might help instead of using module_implements and module_invoke in _jq_plugins
jjeff’s picture

New version of JQP committed which now stores directory listing in the cache table.

http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/jqp/jqp.mod...

Turns out that it won't need hook_flush_caches() because it's not storing cache data in a separate table.

RobLoach’s picture

Need to check isset on $jq['files']['css'] before a is_array call because sometimes the plugin doesn't need CSS.

if (is_array($jq['files']['css'])) {
          foreach ($jq['files']['css'] as $file) {
            drupal_add_css($file);
          }
        }
aaron’s picture

FileSize
7.02 KB

Rolled in suggestions in #20 and #22 to this patch, except: "module_invoke_all might help instead of using module_implements and module_invoke in _jq_plugins". We need individual module names to store in the array, so we can invoke that module's hook_jquery_plugins if they want to do anything fancy on invocation.

Also rolled in new functionality from jq and jqp modules. When building our cache of plugins, it now checks /plugins and /sites/example.com/plugins for .js and .css files. It stores them in a plugin called w/ the file's basename, so that jquery.fancyplugin.js is called 'jquery.fancyplugin'. Additionally, if there is both a .js and .css w/ same basename, they'll both be invoked in that instance.

I'm sure I've missed something. Keep up the great work, folks!

(btw, the latest jQ now also scans for plugins in /plugins and /sites/example.com/plugins too. kudos to jjeff! you rock!)

RobLoach’s picture

We need individual module names to store in the array, so we can invoke that module's hook_jquery_plugins if they want to do anything fancy on invocation.

Hmm, looking at module_invoke_all.......

function module_invoke_all() {
  $args = func_get_args();
  $hook = $args[0];
  unset($args[0]);
  $return = array();
  foreach (module_implements($hook) as $module) {
    $function = $module .'_'. $hook;
    $result = call_user_func_array($function, $args);
    if (isset($result) && is_array($result)) {
      $return = array_merge_recursive($return, $result);
    }
    else if (isset($result)) {
      $return[] = $result;
    }
  }

  return $return;
}

It's strange that it doesn't give us $return[$module] = $result; . It also seems like using array_merge_recursive() could really hit performance if you're getting an array back and you don't want it to merge the results. Ah well. Thanks, Aaron, I'll try this one out and update the jQuery UI module.

webchick’s picture

drupal_get/add_jq -> drupal_get/add_js_plugin() or similar, please. I get you want a short function name but 'jq' is completely meaningless.

Is there a reason we can't build this functionality into drupal_add_js(), so that people who want to add JS to their modules can use the same function regardless if something's a plug-in or whether it's another random JS file? The distinction seems a bit arbitrary to me.

aaron’s picture

@Rob Loach: i've thought both those in the past, but did nothing about it. maybe worth starting an issue on module_invoke_all and see if there are good reasons not to do it. i've needed that functionality before.

bensnyder’s picture

+1 for jqp approach!

aaron’s picture

@webchick:

we pass on anything after the plugin to the controlling module (if that exists). we'd have to allow for similar functionality w/ drupal_add_js, and i'm not sure it could handle overloaded function parameters as easily, considering it already has several other parameters it uses.

the reason is we might want to specify a specific skin, or a whole slew of parameters to pass to a function during the invocation. couldn't do that without completely destroying the drupal_add_js definition, or losing that great functionality that already exists with the jQ module.

webchick’s picture

In other words, I agree with #4.

The rationale in #5 seems to be "because drupal_add_js() currently sucks." If that's the case, I would love to see this ravenous activity around splitting #251578: More flexible js/css ordering and an alter operation into sub-patches so it's committable, and make this one of the sub-issues related to that. It would extend out the life of this issue by a week or two, but I think we'd all be better for it.

/me ducks incoming tomatos. ;)

aaron’s picture

alternatives to drupal_get/add_jq:
_js_plugin
_jquery_plugin
_jquery
_plugin

thoughts? other suggestions?

webchick’s picture

Oh, replies while I was replying. :D

@aaron: This is the part of the other patch that deals with the function signature of drupal_add_js():

-function drupal_add_js($data = NULL, $type = 'module', $scope = 'header', $defer = FALSE, $cache = TRUE, $preprocess = TRUE) {
+function drupal_add_js($data = NULL, $options = array(), $reset = FALSE) {

So as you can see, $options is becoming an array, just as we're doing here. There's no benefit that I can see to adding this separate function for people to learn, unless I'm missing something.

aaron’s picture

@webchick: no tomatoes. but i just don't see how we could overload the drupal_add_js function. plus, this does quite a bit more than just add a file.

aaron’s picture

@webcheck: i see that now. i'll think about it tonight. might be promise there. thanks

RobLoach’s picture

FileSize
48.2 KB
447.66 KB
7.51 KB

Here's a working proof of concept. I fixed the bugs in Aaron's previous patch.........

1) Apply patch
2) Download and install the attached jQuery UI module
3) Visit example.com/jquery_ui to see a dialog box, a color picker, and a date picker.......

// $Id$

function jquery_ui_menu() {
  $items = array();
  $items['jquery_ui'] = array(
    'title' => 'jQuery UI Test',
    'description' => "Provides a way of testing the jQuery UI.",
    'page callback' => 'jquery_ui_test',
    'access arguments' => array('access content'),
  );
  return $items;
}

function jquery_ui_test() {  
  // Add the jQuery UI Color Picker
  drupal_add_jq('ui.colorpicker');
  drupal_add_jq('ui.dialog');
  drupal_add_jq('ui.datepicker');
  
  // Add the inline JavaScript to show the color picker
  $js = '$(document).ready(
      function () {
        $("#colorpicker").colorpicker({flat:true});
        $("#example").dialog();
        $("#datepicker").datepicker();
      }
    );';
  drupal_add_js($js, 'inline');
  
  // Produce the HTML
  return <<<EOT
<div id="example" title="This is my title">This is a Dialog box!</div>

<h3>Color Picker</h3>
<div id="colorpicker"></div>

<h3>Date Picker</h3>
<div id="date"><input type="text" id="datepicker" value="Click inside me to see a datepicker" style="width:300px;"/></div>
EOT;
}

Screenshot: http://drupal.org/files/issues/jqueryui.png

jjeff’s picture

Well technically these files don't NEED to be plugins... nor do they need to be jQuery dependent... (example: a TinyMCE-like WYSIWYG editor)

They ARE javascript libraries though.

How about we call the directory "jslibs"

and the functions drupal_add_jslib() (or variants)

Of course with the JQP approach, we'd end up with functions like drupal_add_jslib_js() and drupal_add_jslib_css()

aaron’s picture

As a hybrid solution to all these threads, we could do something like the following:

drupal_add_jslib/jq/plugin/jquery/jquery_plugin/whatever: This will have the current functionality, of loading all files and dependencies of a particular plugin library, including js and css files. It would also pass any overloaded parameters to the controlling module (if such exists), which could act further on the operation, such as how jQuery Media already does (which will create a custom script based on page context, even going so far as to cache and write a new file for performance optimization).

drupal_add_js and drupal_add_css could have a 'plugin' type $option that would tell it to fetch the file from what we're calling the "plugin" registry. That would allow for the jqp approach. However, it wouldn't load the entire library or pass any overloaded parameters.

Both solutions work from the same registry. The JQP solution, already in place both in this patch and the jQ module, will comb the /plugins and /sites/example.com/plugins directories for js and css files, storing them in the registry. Then hook_jquery_plugins/js_lib/whatever will allow modules to override and define their own registries, and also define the function to be called when invoking drupal_add_jqwhatsit.

Other thoughts: We can use drupal_alter to define a hook_js_lib_alter when the registry is built. We can also have the registry store the function to be called by a module's definition. This allows another way, other than the automatic version checking control in place, for modules to override what's in there. For instance, several modules might call drupal_add_jslib('clueTips'), but a developer decides to override that with 'hoverTips' instead, and they just alter the registry to change the scripts, css, and functions to be called.

RobLoach’s picture

I'd like to add it to drupal_add_js(), but worry about #251578: More flexible js/css ordering and an alter operation. I rerolled the patch, but am not sure where to go from there because I wasn't really that involved with the issue, or the code.

drupal_add_js('ui.progressbar', 'library');
drupal_add_js('ui.progressbar', array('type' => 'library')); // $options

... Would both work due to how the function runs in the patch. We just need to hit that issue hard, cut the patch up into a number of small fixes, and make sure it gets in.

drupal_add_css wouldn't need the "plugin" type, however, because this would all be managed by drupal_add_js. It already adds the required CSS files.

aaron’s picture

the drupal_add_css would be to handle jjeff's requested functionality, and for completeness.

mfer’s picture

@Rob_Loach - I like your suggestion in #37 for using

drupal_add_js('ui.progressbar', array('type' => 'library')); // $options

as the way to add the files. The previous names suggested seem to be getting confusing.

Should we postpone this patch and work on #251578: More flexible js/css ordering and an alter operation because of its implications to this functionality?

redndahead’s picture

@mfer that's what webchick said in #29 and she's the boss lady

markus_petrux’s picture

What would happen when 2 different modules require a particular jQuery plugin but one requires a newer version that breaks the other module? Could that happen that a particular plugin deprecates something that is needed by some modules?

How could we be able to resolve this kind of conflicts?

redndahead’s picture

You can't use both modules then. I don't think we can really do anything about that. It is the same issue as if I had module1 that uses views1 and module2 that uses views2. Well I can't really run both so I have to either help upgrade module1 to use views2 or live without module1.

webchick’s picture

Welll... that's not entirely true, is it?

If module A needs jQuery Something 1.0 and module B needs jQuery Something 2.0, it only *really* creates a conflict if both module A and module B are pulling in their JS files on the same page. This is something I'd expect the centralized registry function to sort out.

Wim Leers’s picture

@webchick: exactly! And for the sake of PLP (page loading performance), it'd be very stupid to serve 2 different versions of the same plugin, too. So IMO, we shouldn't even be supporting this.

@markus_petrux: do you have a specific (existing) use case in mind?

mfer’s picture

If we use the same dependancy model as the modules you can only use one version on a site at a time.

RobLoach’s picture

Status: Needs review » Needs work

Right now, the registry is built dismissing older plugin versions. We could, however, make the version number a key in the array like so:

$plugins = array(
  'ui.progressbar' => array(
    1.62 => array(
      'files' => array(
        'js' => array(
          'misc/ui/ui.progressbar.js',
        ),
      ),
    ),
    2.4 => array(
      'files' => array(
        'js' => array(
          'sites/all/modules/jquery_ui/ui/ui.progressbar.js',
        ),
      ),
    ),
  ),
);

So we have $plugins[$plugin][$version]....

When calling drupal_add_js(), we could add version as an option if it requires a given version:

drupal_add_js('ui.progressbar', array('type' => 'library', 'version' => 2.4));

If you don't state the 'version' number, it could default to the newest version. Even still though, this would result in a JavaScript function naming error if both differently versioned plugins are added to the page at once, which we should avoid anyway. So I guess just using the latest version is the best solution......

mfer’s picture

@Rob Loach if we go this route we should provide an error if more than one version is called on the same page.

aaron’s picture

jQ logs an error when building the registry if there's more than one version provided. however, for this patch, i dropped that behavior in favor of automatically using the newest version. (if no versioning is provided, it simply overrides the plugin with the last found while building). we could add a watchdog message back in if it makes sense. i don't like the idea of creating errors on the actual display, except when we know something's going to totally break.

redndahead’s picture

@webchick excellent point but then markus_petrux situation doesn't apply.

I'm having a hard time with the idea that the newest version replaces the older version. I think since the plan is for js to have a weighting system we use that system to determine what replaces what. If my module needs an older version than what core provides it would be nice to be able to use the older version I provide rather than core's. Obviously conflicts with core functions would have to be taken into account with my module.

webchick’s picture

Agreed, we shouldn't display errors in the UI.

I would say the logic is basically:
"If more than one version of this plug-in was loaded on the same page request, then pick the newest one and watchdog a notice. Else, just load it like you would anything else."

aaron’s picture

if we implement a hook alter when building the registry, then modules can duke it out themselves (and/or claim unclaimed files from the /plugins directory). i think that's entirely sufficient; these edge cases shouldn't determine the core behavior.

markus_petrux’s picture

<< @markus_petrux: do you have a specific (existing) use case in mind? >>

Well, jQuery itself is problematic when new versions are launched, so that's because jquery_update module exists. It's in the middle of Drupal/jQuery, so it hides (or tries to) any possible incompatibility that may arise. Trying to solve that kind of problems with all jQuery plugins out there looks like a quite heavy issue.

I believe webchick's example may happen, ie. there may be some jQuery plugins that do break backwards compatibility with themselves. We cannot control backward compatibility in jQuery plugins, so I think we should find a way to allow running the same jQuery plugin in different versions, as required/instructed by Drupal module that uses them. ie. Drupal's jQuery plugins manager would have to be able to track different versions of the same jQuery plugin, and let them run as long as more than one version is not explicitly required by more than one module for the same page, which is where runtime conflicts could live, which is not acceptable. In that case, modules involved would have an issue. But if those modules use their jQuery plugins in different pages, then they should be able to, I think.

sun’s picture

Status: Needs work » Postponed

Postponing until #315797: JavaScript Patch #1: $options is in shape.

A possible problem with this approach is that libraries could have no (official) version at all. Also, some libraries may undergo customizations or unofficial bug fixes, which might not be present in a later, official version.

Passing an optional (expected) 'version' parameter to drupal_add_js() sounds sane.

However, if a central plugin repository is still considered, that would require to force users to use a consistent file naming schema for JavaScript library versions, i.e. jquery.form.js (for the latest version) and jquery.form.1.0.js (for an earlier version).

aaron’s picture

that would require to force users to use a consistent file naming schema for JavaScript library versions

only for developers who add files manually into the /plugins directory. module defined plugins would include the version in the info array.

aaron’s picture

Status: Postponed » Needs work

#315797: JavaScript Patch #1: $options has been committed, as well as the css version. time to get this one working now too.

mfer’s picture

Should we wait until #315798: JavaScript Patch #2: Weight and #315801: JavaScript Patch #3: JS Alter Operation are in before we add the plugin manager in? Any of the others at http://groups.drupal.org/node/15365?

RobLoach’s picture

Status: Needs work » Postponed

#315798: JavaScript Patch #2: Weight has a patch which will really help with giving JavaScript version information to drupal_add_js. It needs a RTBC review before we can consider working on this because it'll really be that helpful.

RobLoach’s picture

litwol just brought http://plugins.jquery.com/project/jquery-packages to my attention, which might help this issue....

aaron’s picture

An interesting idea. How would that work with aggressive caching, though?

Wim Leers’s picture

aaron: I think you mean JS aggregation? As long as the JS files are *not* added using drupal_add_js(), lazy loading should work. But then of course lazy loading must know where to fetch the JS files, and as we all know, they're not in one single directory in Drupal. So we'd either need to pass through URLs to JS files to let the lazy loading script know the URLs, or we'd need a JS file registry. Option 1 seems more likely to be viable to me.

This is a very interesting technique that we definitely should give some thought. Maybe we can call in kkaefer here, he's the one most likely in having experience with this.

aaron’s picture

yes, js aggregation/optimization is what i meant, thanks.

i thought the point of this patch is to create a js file registry. thus, it seems that would be a viable option, assuming we get this working properly. once we have this patch in, even if we're using drupal_add_js, seems we might be able to use this project for lazy loading, assuming we are able to alter the resulting array somewhere along the way. in fact, for the end developer, that would be the easiest, so they don't have to think about it. just check a box somewhere and have it work automagically.

aaron’s picture

#315797: JavaScript Patch #1: $options, #315798: JavaScript Patch #2: Weight, and #315801: JavaScript Patch #3: JS Alter Operation have all been committed. I don't think there's any thing else holding up development for this issue now? Opening for discussion.

aaron’s picture

Status: Postponed » Active

#315797: JavaScript Patch #1: $options, #315798: JavaScript Patch #2: Weight, and #315801: JavaScript Patch #3: JS Alter Operation have all been committed. I don't think there's any thing else holding up development for this issue now? Opening for discussion.

webchick’s picture

Yep. Have at it! There is one additional drupal_add_js() improvement pending, #91250: JavaScript Patch #4: External Scripts, but I don't think that really affects this issue much. And in any case, I imagine this will be much harder to solve.

If we want something like Future of Form Building in Drupal in core, we're going to need to #315035: Add jQuery UI elements to core (or parts of it, anyway). In order to add parts of jQuery UI in core, we're going to need some means of people dropping in updates since we'll inevitably ship with jQuery UI.buggy.239.2032.23 in Drupal 7 and it will be largely frozen for its entire lifecycle.

So. Let's get this blocking issue solved so we can start to see some neat things develop, and so Drupal 7 remains a flexible solution for years to come.

mfer’s picture

Couldn't jQuery UI or jQuery Update be used to do the updating through hook_js_alter? Does core have to deal with the updates to the scripts or is it ok for contrib modules to pick that up?

If core has to deal with updates I can see issues coming. For example, I was working on a drupal 5 site running jQuery update this week. Some non-essential core things didn't work. It's easier to chalk that up to a contrib module didn't cross every i or dot every t than to say that a core update didn't do complete dependency management.

Hopefully #91250: JavaScript Patch #4: External Scripts will be done and in before the end of the year.

webchick’s picture

That's true. This issue helps more with the situation where Crazy Form Builder needs jQuery UI Draggables v12.303 and Stupidly Crazy Form Builder needs jQuery UI Draggables v12.301 and dealing with how to track which one to farm out on page requests and that.

aaron’s picture

We might want to wait on #345973: Move drupal_add_js $reset parameter as an option for $options['type'] so we can overload function arguments, and then implement drupal_add_js('my_plugin', 'plugin', $extra, $args, $to, $pass);

RobLoach’s picture

Interesting idea for drupal_add_js('my_plugin', 'plugin', $extra, $args, $to, $pass);. Maybe those additional parameters could be merged into $options['arguments'], or something? Also don't forget that the new progress bar rocks....

aaron’s picture

$options['arguments'] is good too. probably a better idea in the end, as it makes it cleaner in the end.

dmitrig01’s picture

What are $extra, $args, $to, and $pass?

RobLoach’s picture

Any additional arguments that you want passed through Drupal Behaviors to the jQuery plugin. MouseWheel asks for the element to apply too and the callback function, jCarousel asks for the carousel element ID, and the arguments needed to create the carousel.

Is it just the external JavaScript support that should go in before this?

skilip’s picture

Since jjeff made me co-maintainer of jqp I'm very committed to this issue. Here are some thoughts:

Adding a complete library using drupal_add_js would be a little weird if the library to be loaded also requires css to be loaded. In that case, you need to call drupal_add_css inside drupal_add_js.

To me, more appropriate would be this:

drupal_add_plugin('ui.progressbar', $options); // Or drupal_add_library

which loads all required files for this library, by calling both drupal_add_css and drupal_add_js.

As jjeff mentioned, it is a good idea to register required 'plugins' (or 'libraries' :)) in .info files. This way admins who download and install module_x can easily see what required plugins must be installed for the module to function. This could look similar to this:

plugins[foo][title] = Foo
plugins[foo][project_page] = http://plugins.jquery.com/project/foo  
plugins[foo][version] = 1.4.2

All plugins (I won't repeat libraries anymore) are cached in an array similar to this:

$plugins = array(
  'foo' => array(
    'title' => 'Foo',
    'project_page' => 'http://plugins.jquery.com/project/foo',
    'lazy_load' => TRUE, // This would be awesome!!!!
    'files' => array(
      'js' => array(
        'sites/all/plugins/jquery.foo.min.js', // latest (index 0)
        '1.4.1' => 'sites/all/plugins/jquery.foo[1.4.1].min.js',
        '1.4.2' => 'sites/all/plugins/jquery.foo[1.4.2].min.js',
      ),
      'css' => array(
        'sites/all/plugins/jquery.foo.css', // latest
        '1.4' => 'sites/all/plugins/jquery.foo[1.4].css', // Version 1.4.x
      ),
    ),
  ),
);

In drupal_add_plugin() the plugins array should be able to be altered by a hook function. Something like hook_library_alter()?

If the module is installed, hook_requirements could be used to interact with privileged users the status of the files. (whether or not the files are installed correctly). Maybe an overview page for all plugins alongside with their css and js files (admin/reports/plugins)? On that page the cache is reset on every page load. Should administrators be informed if a plugin isn't loaded correctly by an (error) message?

[edit1]
I really love the way jqp registers plugins using drupal_system_listing. This allows users to specify where to place the plugins directory themselves.
[/edit1]

[edit2]
How do we force users to rename the files properly, in order to let drupal know that file jquery.foo[1.4.1].min.js corresponds to plugin foo v1.4.1?
[/edit2]

Again, I'm very committed to this while loads of modules I've developed are depending on jQuery plugins. Therefore I'm much willing to spend time on this. I propose to use jqp to further develop this. Or should I post patches here?

skilip’s picture

Wouldn't even be better use directories like we already do with modules and themes? This way we could easily register several versions of a plugin since we could take advantage of .info files. Something like this:

sites/all/plugins/plugin1/plugin1.info
sites/all/plugins/plugin1/jquery.plugin1.min.js
sites/all/plugins/plugin1/plugin1.css

plugins/plugin1-2/plugin1-2.info
plugins/plugin1-2/jquery.plugin1-2.min.js

sites/all/plugins/plugin2/plugin2.info
sites/all/plugins/plugin2/jquery2.min.js
sites/all/plugins/plugin2/plugin2.css
sites/all/plugins/plugin2/plugin2_other.css

In the .info files, we can then store information about the files. I've seen this in D7 info files.

sun’s picture

Please note that we should design this whole thing in a way that it is not limited to jQuery.

For Dynamic Rendering and Wysiwyg API, we badly need a separate (common) storage in the filesystem that holds external (JS) libraries. Currently, users have hard times to update both modules, since they need to backup the external libraries within the module folders, replace the modules, and put back the external libraries afterwards.

See also #320562: Libraries API

skilip’s picture

I'll sure keep that in mind!

RobLoach’s picture

Status: Active » Postponed

If you want to add the plugin information to module.info like the following:

scripts[jcarousel][title] = jCarousel
scripts[jcarousel][project_page] = http://sorgalla.com/jcarousel/
scripts[jcarousel][version] = 0.2.3
scripts[jcarousel][js][] = jquery.jcarousel.js
scripts[jcarousel][js][] = jcarousel.js
scripts[jcarousel][css] = jquery.jcarousel.css

If we want this information in module.info, then we'd want #338002: Allow modules to cache module information.

Lazy loading shouldn't be required because Drupal behaviors allow use to act on Drupal.settings. Here's an example:

  drupal_add_js('jcarousel', 'plugin', array('#jcarousel' => array('vertical' => TRUE)));

As you can see, here we're adding jCarousel in addition to the behavior/settings which would act on #jcarousel. You can see this behavior/settings system in use with the jCarousel module for Drupal 6, and the MouseWheel module.

Either way, we'll definitely want #345973: Move drupal_add_js $reset parameter as an option for $options['type'].

RobLoach’s picture

Status: Postponed » Needs review
FileSize
11.71 KB

This patch introduces three hooks:

hook_js_plugin()
Registers the JavaScript plugin registry
hook_js_plugin_alter(&$plugin)
Alters the JavaScript plugin registry, which makes it very easy to update any plugins
hook_js_plugin_add($plugin, $options = array())
Called whenever a plugin is added to allow the addition of any special use settings or anything.

This works very well in two places in Drupal HEAD in its current state:

  1. Farbtastic: It's registered through system_js_plugin(). When color module uses it, it sends the theme information through the $options array so that color_js_plugin_add() can take advantage of it.
  2. jQuery Form Plugin: Registered through system_js_plugin().

All of this information has been added to system.api.php.

RobLoach’s picture

FileSize
11.07 KB

A bit of code clean up.

RobLoach’s picture

FileSize
12.14 KB

Adds a test.

skilip’s picture

FileSize
106.86 KB
72.55 KB

We have to choose between letting site administrators manually create directories and an UI for linking required files to a library. I guess there's no other way to be sure the namespaces as well as versioning is done consistently. IMO an UI is more user-friendly, so I've created a mockup for this.

The first mockup shows the library details. I forgot to put some kind of notification about which modules are depending on the library.

The other mockup displays the main page for libraries / plugins. It'll list all libraries registered by other modules. If a file is missing, the status will display a warning.

RobLoach’s picture

Status: Needs work » Needs review

That interface will be a perfect UI for the jQ module in Drupal 7! It would use hook_js_plugin_alter() to allow changing of any JavaScript plugin. Very nicely done, skilip, I'm getting really excited about this!

Any thoughts on moving misc/farbtastic to modules/color/farbtastic since that's the only place it's used? Well, I guess not, since having hook_js_plugin() in system and Farbtastic in misc means that other contrib modules can use it.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs review » Postponed

We want the extra parameters after $options, so we need #345973: Move drupal_add_js $reset parameter as an option for $options['type'].

skilip’s picture

@Rob Loach,

As a response on http://drupal.org/node/315100#comment-1191909 :

If hook_js_plugin is actually going into core, I think it would be wise having a plugin directory in core. It is not only more organized, it is also more obvious plugins should be placed in a plugins directory, and not 'in the wild'. So moving farbristic from misc to plugins? Yes we can!

RobLoach’s picture

Sometimes though, you want to do more then just add the plugin to the page. When we add Farbtastic, for example, we want to add:

  1. farbtastic.js: The jQuery plugin
  2. farbtastic.css: The jQuery plugin's associated CSS
  3. color.js: The Drupal behaviors for Farbtastic
  4. color.css: The custom CSS for Drupal to space out Farbtastic
  5. ... and the settings to tell the behavior how to use Farbtastic.

We couldn't manage adding all of those through just putting farbtastic.js into the plugins directory.

The plugin registry could read through through the "plugins" directory, and load in the files (farbtastic.js and farbtastic.css), but then we're not given the behaviors. We want to do more then just add the files to the page, we want to tell Drupal how to handle them, all in one function call.....

drupal_add_js('farbtastic', 'plugin', array('theme' => $theme));

The above function call would add farbtastic.js, farbtastic.css, color.js, color.css and the behavior handling 'theme' with the Farbtastic plugin. I have no objections to adding the plugins directory to the registry, I just don't see how it would help with adding the behavior and the settings.

RobLoach’s picture

FileSize
14.23 KB

Had the test in twice. Also uploaded a patch to the jQuery UI module to test this patch: #357780: Update to Drupal 7

quicksketch’s picture

FileSize
14.24 KB

This all looks simply fantastic. Here's a tiny re-roll that fixes a few comments (yeah I know, really helpful ;).

Changes examples from:
// Farbtasic to // jCarousel.
Since that's what's in the example.

sun’s picture

So far, I am very opposed to this patch. It adds nothing but support requests. Given that we are already at #88 now, I would rather propose to find a solution that works in contrib and move that into core (IF it does).

I'm tired of repeating the issues raised in IRC already. I'd suggest creating a task force, along with a dedicated module namespace, merging jquery_update, jquery_ui, and jq/jqp modules into one "thing" that deals with libraries, find out how the whole system could work out in one central development space, and afterwards, come up with a valid solution for core. I would happily join that effort.

However, the approach that is taken here looks rather wrong to me.

quicksketch’s picture

Given the amount of existing solutions in Drupal 6 and existing modules dependencies on them, I think it may be too late to hope for a unified solution in Drupal 6. There are dozens of modules out there that require the existing solutions. The patch here is already attempting to mix all the solutions from contrib, even though it's not a direct port, it's the conglomeration of the best ideas. In Drupal 6, we have absolutely no way to use the approach here, since drupal_add_js() is so hard-coded there, even an attempt to do the same thing in Drupal 6 would be impossible.

In addition, this patch is absolutely critical for so many other things that we may not have to time to go back and start all over in contrib.

webchick’s picture

sun: Could you give a little bit more substantive feedback about your concerns with the patch, and/or repeating the issues raised on IRC for the benefit of the people who weren't there for it? *waves* :)

starbow’s picture

subscribing

starbow’s picture

Ok, I am probably be a little dense, but it the goal here just to manage jQuery plugins, or is it to manage Drupal behaviors? Or is that still up for debate? The first is absolutely essential (for the reasons webchick says up in #64), but I have been meaning for months to start agitating for management at the behavior level, and maybe this is the right place?
Also, Reading the patch, the first comment is
"Add a JavaScript plugin ('plugin'):
Adds multiple JavaScript files and CSS files for a JavaScript plugin"
Does that mean we are introducing a new concept of a "Drupal JavaScript Plugin", which is distinct from a jQuery Plugin? If so, we really need a different name for it.

RobLoach’s picture

Reading the patch, the first comment is
"Add a JavaScript plugin ('plugin'):
Adds multiple JavaScript files and CSS files for a JavaScript plugin"
Does that mean we are introducing a new concept of a "Drupal JavaScript Plugin", which is distinct from a jQuery Plugin? If so, we really need a different name for it.

Documentation is definitely something that we'd want to get right with this patch, so thanks for taking a look at it. When putting this together, I didn't want it to be only tailored to jQuery plugins. I wanted it to also apply to other things. Alternative JavaScript libraries maybe? Different JavaScript frameworks like Prototype, families of JavaScript and CSS that are completely unrelated to jQuery, etc. With this patch, the only places that seem to be using it are Farbtastic and jQuery Forms.

So, I'd be more then happy with using "jQuery plugins" in the documentation, but want to keep the idea that it does not have to be exclusively used for jQuery plugins.

starbow’s picture

ok, I get it. "Javascript plugin" = a jQuery plugin, or anything else that operates like a jQuery plugin, even if it isn't jQuery. That's cool. It is probably worth having a little more in the comments on what qualifies for inclusion.

mfer’s picture

In reference to the patch in #87...

  • system.api.php seems to have the hook calls defined twice in the file.
  • files[] = system.js.inc is listed twice in the system.info file.
  • farbtastic is a dependancy for the color module. Not the other way around. It may be used in cases that are not color module related. color_js_plugin_add should not just add it's stuff in if farbtastic is called. If anything, it should be looking for where the color module js is being called and add in farbtastic. That's the appropriate dependency chain.

When I look at the function drupal_add_js I see something that's get's getting really large. I don't like mega functions. Could this be broken down? Maybe make drupal_add_js a driver type function that calls other functions to do the work. That would be more testable.

Of the possible ways this could be done I like the way this patch is coming together. Nice job.

This does bring up an issue of support. I don't like the idea of a plugins directory or anything where someone would get the idea they can just drop a plugin in and have it work. That's a support nightmare. Good documentation and examples will be a must for this.

sun’s picture

I'm trying to summarize the discussions in IRC. Note that this might not be complete.

Here are most of my questions:

  • What qualifies a "plugin"? Is jQuery UI a plugin or a library? If it is a plugin, is drupal.js also a plugin? That questioned, is jquery.js a plugin? I don't think so? Are we rather talking about compatibility and dependencies between certain JavaScript libraries then?
  • How does this approach deal with compatibility issues? What happens if a module adds the same "plugin" again? I.e. which module wins? Will the "previous" module be screwed up? Likewise, what happens if a module alters the plugin registry to implement a new version? That questioned, where is the difference to the current situation? Would the "previous" module also have to implement an alter-hook to ensure that the proper (compatible) version is loaded? Wouldn't the current situation even be better in this case, where it is more *unlikely* that multiple modules try to load the basically same (but different) library on the same page?
  • Would this approach only work for jQuery UI, which is known to be provided by one module only? And if Wysiwyg API would use this implementation to load a jQuery-based editor, the user would suddenly get a different editor version that is incompatible to the 312 modules that integrate with it?
  • If modules are able to alter the "plugin" that is loaded, who will answer the flood of support requests and bug reports saying "Help! My ENTIRE JavaScript stopped working!"? How do we put this under the user's (admin's) control?
  • What if one of those plugins requires more than one JavaScript file? Shouldn't we consider a "plugin" that needs to load a separate CSS file and which is potentially able to load multiple JavaScript files as _library_? Where is the difference to other JavaScript libraries that may not integrate with jQuery? And (when considering the fact that we're discussing libraries), what about external libraries, which cannot be hosted on drupal.org? Isn't our general goal to keep external code out of drupal.org's CVS? What about licensing issues?
  • What happened to the dependency handling that was already mentioned in #20?
  • How do themes implement this hook?
  • If the entire "plugin" registry is cached, no module is ever able to implement its own version of a "plugin" again? Even if it is using it on certain, totally awkward pages only?
  • It was mentioned that modules could bail out if there was a wrong/incompatible "plugin" version in use -- so you consider that all modules in contrib are planning ahead on their time, expecting that 1.5 of a "plugin" will break their script, while 1.4 does not? Did we consider that modules have different release cycles? Will all modules be forced to bail out on each subtle difference then? Were you involved in the jQuery Update issue from 1.2.3 to 1.2.6?
  • What if a module requires a *fixed*, i.e. customized, version of a plugin to work flawlessly? Additionally, what if the official (external) project of a "plugin" is poorly maintained? Will my module not ever work again, just because some other popular module also needs the same library and has a higher module weight?
  • After all, where is the experience with multiple contrib modules that (try to) implement the same library? Why can't this overall issue be solved in a contrib module first, like jQuery UI - or a new module that merges jquery_update, jquery_ui, and jq/jqp, and solely deals with those issues?

See #41 / #45.
See #53.

RobLoach’s picture

These are replies to sun's questions in #96.....

What qualifies a "plugin"? Is jQuery UI a plugin or a library? If it is a plugin, is drupal.js also a plugin? That questioned, is jquery.js a plugin? I don't think so? Are we rather talking about compatibility and dependencies between certain JavaScript libraries then?

A described in #93, this isn't exclusive to jQuery plugins. It could work with anything that means to add both JavaScript and CSS to the page. It just tries to manage them appropriately. We could change the documentation to reflect that appropriately. Feel free to call them what you want, "JavaScript plugins" seemed appropriate to me at the time.

How does this approach deal with compatibility issues? What happens if a module adds the same "plugin" again? I.e. which module wins? Will the "previous" module be screwed up? Likewise, what happens if a module alters the plugin registry to implement a new version? That questioned, where is the difference to the current situation? Would the "previous" module also have to implement an alter-hook to ensure that the proper (compatible) version is loaded? Wouldn't the current situation even be better in this case, where it is more *unlikely* that multiple modules try to load the basically same (but different) library on the same page?

The same could be said about using the jQuery Update module to update your Drupal 5 site to jQuery 1.3. It will break other functionality on the site and there's not much you can do about it other then not do it. The people behind jQuery Update are smart enough not to update it to jQuery 1.3 because they know this. This control is given to the module developers, not its users, so if they want to alter things enough to make that kind of thing possible, then go for it.

Would this approach only work for jQuery UI, which is known to be provided by one module only? And if Wysiwyg API would use this implementation to load a jQuery-based editor, the user would suddenly get a different editor version that is incompatible to the 312 modules that integrate with it?

No, as demonstrated in the patch, we can use it with other things like Farbtastic, and the jQuery Forms Plugin. If there is a versioning conflict, you could use hook_js_plugin_alter() to conditionally swap out parts of other plugins. With hook_js_plugin_add(), you're given the arguments that are passed to the plugin when it's being added, so even then, you could essentially check which version is desired.

If modules are able to alter the "plugin" that is loaded, who will answer the flood of support requests and bug reports saying "Help! My ENTIRE JavaScript stopped working!"? How do we put this under the user's (admin's) control?

It's not in the users/administrators control. It's in the module developers control. We could turn the jQ module into a user interface like skilip suggested in #80 (awesome concept images included!), but I think keeping it in the module developers hands is the safest route (with hook_js_plugin()). Users are not smart people, like mfer mentioned in #95, good documentation and examples on the module developer's side will be key.

What if one of those plugins requires more than one JavaScript file? Shouldn't we consider a "plugin" that needs to load a separate CSS file and which is potentially able to load multiple JavaScript files as _library_? Where is the difference to other JavaScript libraries that may not integrate with jQuery? And (when considering the fact that we're discussing libraries), what about external libraries, which cannot be hosted on drupal.org? Isn't our general goal to keep external code out of drupal.org's CVS? What about licensing issues?

Hook_js_plugin() allows you to send in as many JavaScript and CSS files you want when the registry is built. External libraries won't be supported until external scripts goes in. Again, the way the JavaScript files are loaded are completely dependant to how hook_js_plugin() deals with them. If that means getting it out of a "plugins" directory, then that's up to the module developers to provide the functionality for. If that means making the user upload a JavaScript file, then that's up to the module developers. If that means using the Drupal 7 jQ module to upload a package and reference it to the plugin, then jQ will do that. Again, it's completely up to how hook_js_plugins() creates the registry.

What happened to the dependency handling that was already mentioned in #20?

Right now it's non-existant. Definitely would be great to get a "#dependancy" flag or something in there.

How do themes implement this hook?

Hmmm, at the moment, they don't. What if we added a "js_plugin" theme registry function that would allow theme developers to alter the plugin registry after the call to drupal_alter()?

If the entire "plugin" registry is cached, no module is ever able to implement its own version of a "plugin" again? Even if it is using it on certain, totally awkward pages only?

Would a drupal_clear_js_plugin_cache() function be helpful? All it would is clear the "js_plugin" cached variable....

It was mentioned that modules could bail out if there was a wrong/incompatible "plugin" version in use -- so you consider that all modules in contrib are planning ahead on their time, expecting that 1.5 of a "plugin" will break their script, while 1.4 does not? Did we consider that modules have different release cycles? Will all modules be forced to bail out on each subtle difference then? Were you involved in the jQuery Update issue from 1.2.3 to 1.2.6?

Again, completely dependant on how hook_js_plugin() and hook_js_plugin_alter() operate. It would be great to get more granular with the dependancy model and the version control. Considering its handled through the registry, as well as hook_js_plugin_add(), I see it having a lot of control over itself. If you wanted to get even more granular with the control, you could even get into hook_js_alter().....

What if a module requires a *fixed*, i.e. customized, version of a plugin to work flawlessly? Additionally, what if the official (external) project of a "plugin" is poorly maintained? Will my module not ever work again, just because some other popular module also needs the same library and has a higher module weight?

That's up to hook_js_plugin() to decide. If a module you make requires a fixed/customized version of the plugin, in a Utopian open source world, you would work with the plugin maintainer to fix it so that it doesn't need that customization. If that can't happen, hook_js_plugin() could check that the fix is apparent.

After all, where is the experience with multiple contrib modules that (try to) implement the same library? Why can't this overall issue be solved in a contrib module first, like jQuery UI - or a new module that merges jquery_update, jquery_ui, and jq/jqp, and solely deals with those issues?

The modules you mentioned do completely different things. jquery_update brings jQuery to the latest version, jquery_ui makes loading jQuery UI jQuery plugins easy, jQ creates a plugin registry and gives you a simple function to add the files, and jQP is a handler around drupal_add_js() to add files from a user/administrator maintained "plugins" directory.

We've been working on fixing the issues brought up in all of those modules in the JavaScript Improvements Patch Spotlight. We hit Allow altering JavaScript to make the jQuery Update module easy (jquery_update_js_alter), now we're hitting this issue to make the jQ, jQuery UI and jQP modules easy (hook_js_plugin).

starbow’s picture

Following on #95
+files[] = color.js.inc is also added twice.

I like this functionality. I did not find the name of the functions all that helpful, and had to read the code to get what they each did. How about

  • drupal_js_plugin => drupal_js_plugin_define, since this is where they get defined (or declared)
  • drupal_js_plugin_add => drupal_js_plugin_adding. When I first saw this, I thought it was a better named replacement for drupal_add_js('', 'plugin'). 'adding' makes it clear that the hook is responding to the add event, not initiating it.
skilip’s picture

FileSize
13.04 KB

I've locked myself up at the attic for a while to develop my ideas into a module. Basically I think libraries (or plugins) should be treated like modules, or themes, with the difference that libraries should not be ported form d.o. Therefore these libraries should be configurable as flexible as possible.

I guess the module speaks for itself. There's a detailed description in the help section of the module. Hope you all like it.

Owen Barton’s picture

Status: Postponed » Needs review

Since #345973 is fixed we can put this one on the table again.

Status: Needs review » Needs work

The last submitted patch failed testing.

skilip’s picture

Hey guys, anyone tested my module yet?

skilip’s picture

So I had a little chat about my module with Rob at IRC. Here are some remarks about the module he mentioned (correct me if i'm wrong):

  • Try to omit the UI or put it in a contrib module
  • Why not use the caching system instead of the system table?

As for the UI, I know I should avoid this as much as possible, and a contrib module sounds like a great idea. However I think it enhances the administration of plugins / js libraries exponentially. Users/developers can easily see whether or not all plugins are installed correctly. I'm not sure we should separate this functionality into an 'optional feature'. I'd like to hear more opinions about this.

For the registration of plugins, my aim was to take advantage of .info files for registering plugins/libraries. Besides that, they have a lot in common with modules and themes, like versioning, dependencies, one or more files, etc. With this in mind, I thought it would be wise to take advantage of the current D way of registering modules/themes. Why reinvent the wheel twice?

Please share your thoughts about this. Convince me if I've got it completely wrong!

RobLoach’s picture

FileSize
14.65 KB

Again, if we want to take advantage of the .info files, we'd need #338002: Allow modules to cache module information. We're not reinventing the wheel, it's just saving us a database query to the system table if we just use the cache. Registering the plugins via a hook and a hook alter gives us complete control over how the plugins are managed. If we hand that control over to users, it'll just end up being a support mess. In the module developers hands is where we want to keep the control (through hook_js_alter, hook_js_plugin and hook_js_plugin_alter).

This patch updates to HEAD, as well as allows the JavaScript and CSS to provide their own $options arrays when being added:

function hook_js_plugin() {
  // jCarousel.
  $plugins['jcarousel'] = array(
    'files' => array(
      'js' => array(
        // Items that are just strings are interpreted as stright files. 
        drupal_get_path('module', 'jcarousel') . '/jquery.jcarousel.js',
      ),
      'css' => array(
        // By providing an array, it becomes the $options parameter during the
        // drupal_add_js/drupal_add_css call.
        drupal_get_path('module', 'jcarousel') . '/jquery.jcarousel.css' => array(
          'type' => 'file',
          'preprocess' => FALSE,
          'media' => 'screen',
        ),
      ),
    ),
  );
  return $plugins;
}
RobLoach’s picture

Status: Needs work » Needs review
FileSize
13.36 KB

Wrong patch, here it is. Submitting to the testing bot.

We should consider using call_user_func to pass the trailing arguments after $options instead of just $options. Right now, hook_js_plugin_add() just gets the $options array and it would be nice to get arguments specific to what plugin we're adding. Then you could do something like:

  drupal_add_js('farbtastic', 'plugin', array('theme' => 'garland'));
  // instead of:
  drupal_add_js('farbtastic', array('type' => 'plugin', 'theme' => 'garland'));

As to which modules are benefited by this....

jQ
Pretty much going into core by this patch via hook_js_plugin() and hook_js_plugin_alter().
jQuery Plugin
Pretty much going into core with this patch, aside from the plugins it defines
jQP
Not covered by this patch, as it puts control out of the module developer's hands. The module could invoke hook_js_plugin() and register the plugins in the /plugins/ directory though. Again, since the plugins directory is out of module developer's hands, it should not be in core. It would gain huge benefit of hook_js_plugin() though.
Plugins
What the Plugins module has over this patch is the ability to automatically download and install JavaScript plugins (screenshot). I think this is something that shouldn't be covered by core. At the same time, the majority of the Plugins module is covered by hook_js_plugins() and hook_js_plugins_alter() and seems like a place where plugins could benefit. The Plugins module in Drupal 7 would then allow users to automatically download and install plugins defined by contrib modules invoking hook_js_plugins(), and setting the new downloaded JavaScript/CSS files in hook_js_plugins_alter().
Skilip's JavaScript Library module
The plugin registry is covered in this patch. What is missing from this patch that the JavaSCript Library module provides is a user interface and an overview of what JavaScript plugins are available, along with if the JavaScript file is available. I don't think an interface is required, maybe just a hook_requirements check to make sure all JavaScript plugins are available.

From looking at the above list, we see that's what is missing is the implementation of hook_requirements to see if any JavaScript or CSS files are missing. Should this be covered by the modules themselves, or core?

keith.smith’s picture

Status: Needs work » Needs review

+ * Adds a JavaScript pluging to the page and tests for both it and its CSS.

"pluging"

+    $this->assertTrue(strpos($javascript, 'misc/farbtastic/farbtastic.js') > 0, t('JavaScript Plugins are rendered to the page.'));
+    $this->assertTrue(strpos($stylesheets, 'misc/farbtastic/farbtastic.css') > 0, t('JavaScript Plugins render their CSS to the page.'));

Most other instances of "Plugins" are "plugins".

+ // Items that are just strings are interpreted as stright files.

"stright". What is a straight file, btw?

keith.smith’s picture

Status: Needs review » Needs work

Forgot to change status.

skilip’s picture

Status: Needs review » Needs work

Great to see we're back in business! After some thinking I agree the UI should not be something for core but more of something for a contrib module. The patch in #115 looks good and if we can overcome the following notes, I'm convinced.

  • I really wish to use .info files. It forces developers to use a system of storing them files. It actually helps them to keep things arganized.
  • I saw it is possible to use plugin versioning. If you call drupal_add_js and don't specify a version, how is it handled? And what if the plugin is installed but a module demands a different version?
  • We need to create something which informs the developer / user if a plugin isn't installed correctly. Hook_requirements seems ok.
rickvug’s picture

Issue tags: +jQuery, +JavaScript

I'm adding "Javascript" and "jQuery" tags. I stumbled on this patch when looking into the current jQuery Update, UI and plugins situation for D6. As Rob Loach details in #105, there are a myriad of approaches to the plugin issue in D6. That tells me that this is badly needed. Related to this, based on Mark and Lisa's work, I see more javascript coming into D7 soon. Tempted to mark the issue as critical.

webchick’s picture

Priority: Normal » Critical

Good idea. ;)

quicksketch’s picture

Priority: Critical » Normal

Starbow's comments in #98 (regarding renaming the hooks) haven't yet been addressed, so I thought I'd answer them. Note that where he said "drupal_" should probably be "hook_", since these are hook names, not actual functions.

* drupal_js_plugin => drupal_js_plugin_define, since this is where they get defined (or declared)

The convention for defining a list of possible items has become "_info". hook_node_info(), hook_actions_info(), hook_field_widget_info(), hook_field_info(), etc. Though we've got some anomalies like hook_theme(). I'll leave it to Rob to decided if we should change the name of the hook.

* drupal_js_plugin_add => drupal_js_plugin_adding. When I first saw this, I thought it was a better named replacement for drupal_add_js('', 'plugin'). 'adding' makes it clear that the hook is responding to the add event, not initiating it.

I think we should keep this as "add". Our existing hooks aren't called hook_node_saving() or hook_node_inserting(). Using the infinitive verb is what we use throughout all of Drupal.

quicksketch’s picture

Priority: Normal » Critical
RobLoach’s picture

Status: Needs work » Needs review
FileSize
13.86 KB
59.07 KB

This patch takes some previous issues into consideration:

  • Provides the hook_requirements implementation that adds an error when JavaScript or CSS files are missing from a given plugin. See the attached screenshot.
  • Includes Keith's string fixes
  • Moves to drupal_add_plugin() like skilip suggested in #72 because we needed a way of getting all the plugins, or a single plugin (drupal_get_plugin())

Things that are included that weren't considered previously:

  • Since the plugins can add things other then just JavaScript and CSS files, moved $plugin['files']['css/js'] to just $plugin['css/js']. For example, you could add settings through the plugin, so listing that under files is kind of silly.
  • Added a $plugin['download'] link directly to the required plugin version so that the requirements error is a bit more helpful.

Things that are not included in this patch:

  1. #108 by skilip: The ability to add plugins from .info files. This should probably be handled in a future issue. Could easily be added in system_js_plugin()
  2. #108 by skilip: Direct plugin version control. Right now, it just handles one version at a time. If a module updates a plugin to the latest version, and you still want to use the old version, you can't. The module that implements hook_js_plugin_alter() should be smart enough to replace the plugin and files correctly. If you want to add a jQuery plugin to the page, you don't really care what version it is, just that it's added. If you explicitly state the version you want each time you use it, it might get kind of annoying. Since the alter is there, this kind of thing is possible, it just seems out of scope for this patch. See hook_js_plugin_alter()
  3. Better documentation. Needs lots more documentation.
  4. It might still be nice to be able to have drupal_add_js('farbtastic', 'plugin') wrapper around drupal_add_plugin(). Thoughts? Then you could add additional plugins from a plugin! Haha.
  5. Should jQuery itself be held in the plugin registry? $plugin['jquery'] ?

... On a side note, I think Farbtastic is broken in HEAD, with or without this patch.

redndahead’s picture

I can verify the farbtastic issue exists without this patch.

$base = drupal_get_path..

Should probably be

$path = drupal_get_path...

It seems most of core uses $path var in conjunction with drupal_get_path

Other than that it seems understandable and looks sound. I'm not knowledgeable enough to comment on #4 and #5.

sun’s picture

Title: Centralized jQuery plug-in manager » Centralized JavaScript library/plugin registry
Priority: Critical » Normal
Status: Needs review » Needs work

A described in #93, this isn't exclusive to jQuery plugins.

If this patch is not exclusive to jQuery plugins, then this example invocation contained in the latest patch is wrong:

-    drupal_add_js('misc/jquery.form.js', array('weight' => JS_LIBRARY));
+    drupal_add_plugin('form');

As you can see, the original jQuery plugin filename indicated that it's a jQuery plugin, and not a mootools or prototype plugin. It's also no client-side editor. And, it's also no custom plugin authored and provided by a Drupal contrib module.

The approach taken in this patch will lead to issues like this: #443514: Stylesheet override logic prevents loading of stylesheets of third-party libraries

The same could be said about using the jQuery Update module to update your Drupal 5 site to jQuery 1.3. It will break other functionality on the site and there's not much you can do about it other then not do it. The people behind jQuery Update are smart enough not to update it to jQuery 1.3 because they know this. This control is given to the module developers, not its users, so if they want to alter things enough to make that kind of thing possible, then go for it.

You exactly repeated my concerns. The user can't control which file(s) will be loaded. Once you have a module that alters the plugin registry for any reason, you are lost. Neither you, nor the module author/maintainer will be able to solve the registry manipulation in a way that works for the maintainer and the user that does not want the registry manipulation.

"What happened to the dependency handling that was already mentioned in #20?"

Right now it's non-existant. Definitely would be great to get a "#dependancy" flag or something in there.

Properly handling dependencies is the key feature for a plugin registry. Without dependency handling, this patch does nothing but grouping some functions calls in one - cruft we don't need in core. Again, I invite all of you to find a working solution for this in #328252: Handle compatibility of internal plugins via plugin API.

The challenge is to solve minimum version and maximum version compatibilities between libraries and depending plugins/scripts that are not under our own control.

"If the entire "plugin" registry is cached, no module is ever able to implement its own version of a "plugin" again? Even if it is using it on certain, totally awkward pages only?"

Would a drupal_clear_js_plugin_cache() function be helpful? All it would is clear the "js_plugin" cached variable....

Clearing the plugin registry? Teach people to use a nasty workaround to circumvent some nasty magic of a too limited sytem?

Again, completely dependant on how hook_js_plugin() and hook_js_plugin_alter() operate. It would be great to get more granular with the dependancy model and the version control. Considering its handled through the registry, as well as hook_js_plugin_add(), I see it having a lot of control over itself. If you wanted to get even more granular with the control, you could even get into hook_js_alter().....

So. Now, we are already proposing to fix up plugin version issues using hook_js_plugin_alter() in this very issue. Do I have to predict how many follow-up issues will appear due to this? See also above (in this follow-up).

That's up to hook_js_plugin() to decide. If a module you make requires a fixed/customized version of the plugin, in a Utopian open source world, you would work with the plugin maintainer to fix it so that it doesn't need that customization. If that can't happen, hook_js_plugin() could check that the fix is apparent.

Sorry, but I don't see how "hook_js_plugin() could check that the fix is apparent". Btw, Panels is one of those modules that implements a plugin that was customized/fixed to work with Panels' AJAX functionality.

The modules you mentioned do completely different things. jquery_update brings jQuery to the latest version, jquery_ui makes loading jQuery UI jQuery plugins easy, jQ creates a plugin registry and gives you a simple function to add the files, and jQP is a handler around drupal_add_js() to add files from a user/administrator maintained "plugins" directory.

That does not explain why the maintainers of those modules can't join forces to build a working solution in contrib first. Contrib is the place this JS plugin system is for and where it will be primarily used. Nothing of this patch requires special support in core first.

It might still be nice to be able to have drupal_add_js('farbtastic', 'plugin') wrapper around drupal_add_plugin(). Thoughts? Then you could add additional plugins from a plugin! Haha.

I don't see the joke in here. jQuery is a library. jQuery UI is a library that depends on jQuery, and on top of that, on very certain minimum/maximum versions of jQuery. jQuery UI itself is a library consisting of many plugins, each requiring one or more other core plugin files of jQuery UI. This funny joke is the very first real and valid use-case in this entire issue.

RobLoach’s picture

Instead of just bringing up problems with what we're trying to solve here, it would more constructive if you proposed viable solutions. Thanks.

RobLoach’s picture

FileSize
13.61 KB

I was having a look at applying this to jQuery UI with drupal_get_path('plugin', 'jquery-ui'); and it looks like drupal_get_path() only gets the path if a file exists in the directory by the name of $name.$type. We should look into this so that we could use sites/all/plugins: jQuery UI 7.x-1.x-dev.

jdblank’s picture

I am trying to apply this patch using: patch -p0 -i 315100_6.patch and it tells me the following error:

can't find file to patch at input line 8
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: includes/common.inc
|===================================================================
|RCS file: /cvs/drupal/drupal/includes/common.inc,v
|retrieving revision 1.911
|diff -u -r1.911 common.inc
|--- includes/common.inc        24 May 2009 17:39:30 -0000      1.911
|+++ includes/common.inc        25 May 2009 21:29:56 -0000
--------------------------
File to patch:
[root@drupal1 jquery_ui]#
ctmattice1’s picture

I manually applied the patch to 7.x.dev dated 06/05/09 modifying

// $Id: common.inc,v 1.917 2009/06/03 02:50:21 webchick Exp $
// $Id: form.inc,v 1.339 2009/06/02 13:47:25 dries Exp $
// $Id: system.install,v 1.338 2009/06/04 03:33:28 webchick Exp $
// $Id: system.api.php,v 1.39 2009/06/04 03:33:28 webchick Exp $
// $Id: system.module,v 1.708 2009/06/05 05:28:28 dries Exp $
// $Id: simpletest.module,v 1.52 2009/05/30 11:17:32 dries Exp $
// $Id: color.module,v 1.59 2009/05/27 18:33:55 dries Exp $

status page tells me that it can't find jquery-ui in sites/all/plugins. I've downloaded and installed it there, renamed the folder to jquery-ui and have even dump the jquery-ui files from the folder to sites/all/plugins. No matter what I try it can't find it. I did this after running through the INSTALL.TXT file and setting up the jquery-ui folder there.

Then I checked to see if the color module was working, it was. If someone can guide me in where to look to correct the code to call the file I'll give it a go. I understand php to some extent and can help debug with a little guidance.

BTW this is on a vista box, running xampp and using firefox 3 browser. I also have netbeans installed but am not familiar with IDE's that much as I code by hand (I know, why) so if someone is also willing to assist there I'm all game.

Thanks for the help and any assistance. Even if I can't help with the code all that much I can help find bugs.

RobLoach’s picture

sites/all/plugins won't work because drupal_get_path() only looks for the {name}.{type} file instead of the directory name.

dmitrig01’s picture

I disagree somewhat with the approach used in color.module. You're assuming that whenever you use farbtastic you're using color module, which is not true.

RobLoach’s picture

FileSize
11.25 KB

Good call, Dmitri. Removed color_js_plugin_alter() and color_js_plugin_add() so that color module doesn't automatically add the Color related stuff when using Farbtastic. Thanks.

This patch also removes the file dependency checking in system_requirements() because I don't want to bikeshed the issue.

mfer’s picture

I like the basic structure here. Could use some more documentation. The most confusing part was the need for a hook_js_plugin_add. Makes sense but took me a minute.

RobLoach’s picture

Would switching to a "add" function array instead of hook_js_plugin_add() make more sense?

  // Plugin Two.
  $plugins['plugin-2'] = array(
    // When the plugin is added, call the given function array.
    'add' => array(
      // The plugin_2_added function will be called when plugin-2 is added to the page.
      'plugin_2_added',
    ),
  );
mfer’s picture

@Rob Loach there is a definite difference between these 2 approaches. With the current path I can have something like:

function module1_js_plugin() {
  // Plugin One.
  $plugins['plugin-1'] = array(
    'title' => 'Plugin One',
    'project_page' => 'http://example.com/plugin-1',
    'version' => 1.2,
    'download' => 'http://example.com/plugin-1/plugin-1.js',
    'js' => array(
      // Items that are just strings are interpreted as stright files. 
      drupal_get_path('module', 'my_module') . '/plugin-1.js',
    ),
    'css' => array(
      // By providing an array, it becomes the $options parameter during the
      // drupal_add_js/drupal_add_css call.
      drupal_get_path('module', 'my_module') . '/plugin-2.css' => array(
        'type' => 'file',
        'media' => 'screen',
      ),
    ),
  );
}

function module2_js_plugin_add($plugin) {
  // When Plugin One is added, also add some settings to the page.
  if ($plugin == 'plugin-1') {
    $settings = array(
      'plugin-1' => variable_get('plugin-1', 'November 19th, 1978'),
    );
    drupal_add_js($settings, 'setting'); 
  }
}

Notice that 2 different modules can act on this. In the method you propose in #125 the defining module specifies a function to call. Note, with alter call on the plugin arrays a module could add it's custom function to the list of functions to call.

With that said, i like what's in #125 better. It's one less hook in the hook soup we are building.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

Thanks for the review, Matt.

  1. Adds more documentation to hook_js_plugin()
  2. Switches to an add handler instead of hook_js_plugin_add() like Form APIs #submit
quicksketch’s picture

Awesome, I like this change a lot. To clarify, the example that mfer posted in #126 could now be re-implemented as:

/**
 * Implementation of hook_js_plugin().
 *
 * Add the plugin to the registry.
 */
function module1_js_plugin() {
  // Plugin One.
  $plugins['plugin-1'] = array(
    'title' => 'Plugin One',
    'project_page' => 'http://example.com/plugin-1',
    'version' => 1.2,
    'download' => 'http://example.com/plugin-1/plugin-1.js',
    'js' => array(
      // Items that are just strings are interpreted as stright files.
      drupal_get_path('module', 'my_module') . '/plugin-1.js',
    ),
    'css' => array(
      // By providing an array, it becomes the $options parameter during the
      // drupal_add_js/drupal_add_css call.
      drupal_get_path('module', 'my_module') . '/plugin-2.css' => array(
        'type' => 'file',
        'media' => 'screen',
      ),
    ),
  );
}

/**
 * Implementation of hook_js_plugin_alter().
 *
 * Add a callback to be fired when plugin-1 is added to the page.
 */
function module2_js_plugin_alter($plugins) {
  $plugins['plugin-1']['add'][] = 'module2_js_plugin_add';
}

/**
 * When Plugin One is added, also add some settings to the page. Note that this is not
 * a hook any more, it's a callback specified in module2_js_plugin_alter().
 */
function module2_js_plugin_add($plugin) {
  $settings = array(
    'plugin-1' => variable_get('plugin-1', 'November 19th, 1978'),
  );
  drupal_add_js($settings, 'setting');
}
skilip’s picture

As much as I want this issue to move forward, I still have my concerns about this getting into core.

Still not sure about the namespace. Plugins suggests it is only possible to add jquery plugins, while we'd better focus on registering libraries.

Beside that, how can a library be registered without the need of a hook_plugin implementation? With the need of this hook we still won't get rid of all the jquery wrapper modules. We need some way to register a library without the need of a module!

Finally, if a required library is missing, or when an updated library needs an updated version of a required library, how can we inform the user? And what if module A depends on jquery.foo.1-34.js while module B requires jquery.foo.1-38.js?

quicksketch’s picture

We need some way to register a library without the need of a module!

I don't think that's likely to happen. There is absolutely NO convention for jQuery plugins to define their versions, dependencies, or files. And even if there were a way to determine this for jQuery plugins, it wouldn't be standard for thing that aren't jQuery plugins (such as completely unrelated JavaScript libraries). A module MUST declare ownership of a library. If a solution does become available at some point, this problem can be solved by contrib.

Finally, if a required library is missing, or when an updated library needs an updated version of a required library, how can we inform the user? And what if module A depends on jquery.foo.1-34.js while module B requires jquery.foo.1-38.js?

Just like any other module, we can implement hook_requirements and check which libraries are available and report the error to the user at admin/reports/status.

Regarding naming, I don't really care if we call this "plugins" or "libraries" (or anything else). Whatever convention we use, it will be in conflict with some other project's naming conventions. Plugin seems reasonable to me because 90% of the things we'll be adding will be jQuery plugins, so it makes sense to stay consistent with the project that is most important to us.

skilip’s picture

@quicksketch: I know it will be a huge challenge to solve the versions / dependencies issues regarding plugins and libraries. However I'm not the type of guy who gives up when things are getting heavy. And nor should we all when it applies a core patch. Let's all think of a solution to make this the most solid solution possible, instead of thinking "hey this is cool, let's put it into core".

Again, sorry for the bikeshed!

chx’s picture

Status: Needs review » Reviewed & tested by the community

Get this in. Now. " Plugins suggests it is only possible to add jquery plugins, while we'd better focus on registering libraries." well yeah so what? that's the next patch. Better get this in and be able to move forward with great interfaces building on jQuery UI than debating this to hell and not being able to.

chx’s picture

Note the time honored wisdom on http://drupal.org/patch/review

No patch can save the world, not even a Drupal core patch. If it works and does something useful on its own, then it is good to go. One of the worst things you can do is elaborate on other, equivalent approaches and suggest complex extensions to the patch. Additional features can be added later. A scalpel is often better than a Swiss Army Knife.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Don't do that.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I do that because we are not getting any meaningful reviews but bikeshedding, adding complexities that needs to stop.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

I realize it's frustrating. But it's incredibly important that we have the consensus of the larger JS development community about this before it's truly RTBC, so all you are doing right now is shutting down important review that needs to happen.

You, my friend, are no JS developer. So please don't mark this RTBC again.

RobLoach’s picture

Status: Needs review » Needs work

Let's add a SimpleTest for the "add" function array handler, how about...

jjeff’s picture

Status: Needs work » Needs review

It looks like we're not going to come to a clear conclusion on HOW plugins should be registered (interface, module vs autodetect, etc), so I think the best we can do is to provide a place in core for them to live. We'll provide some examples with the core modules (such as color module), and leave it to contrib to implement the hooks they would like.

This is my understanding of what Rob's patch does, and I think it's a good way to go.

+1

skilip’s picture

Is much as I'm concerned about lack of functionality ATM, I see the potential of this path. There still is a lot to do but you all convinced me to move this forward. Next improvements should go into a new issue. 1+

sun’s picture

Status: Needs review » Needs work

- You rename 'js_plugin' to 'jquery_plugin'. Everywhere. And,

- You make the hook names plural.

- You remove all properties but 'js', 'css', 'plugin', and 'add'.

- You pass the plugin name to custom add handlers as first argument.

aaron’s picture

I tend to agree with chx. Let's get this thing committed, then refine it (or let contrib do the job). Lots of good ideas floating around, but some of them are railroading the initial intent.

More tests, however, are always a good idea.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
15.3 KB

Sun, you'll need to clarify the request to rename to "jquery_plugin". Per #74:

Please note that we should design this whole thing in a way that it is not limited to jQuery.

This implementation is not limited to jQuery plugins, why should we rename it to be hook_jquery_plugin()?

Also, you'll need to explain why things like "version" or "project_page" should be removed from the hook. It seems like this would be helpful to point users to where they can download the needed library. With these properties we're able to implement requirements based on a version of a library and possibly upgrade JavaScript in between core cycles (jQuery Update is an obvious candidate for using such functionality).

Here's a reroll with a new test for altering and the add callback. Passing in the plugin name to the add handlers is definitely required even for this test to work. I'm ambivalent about pluralizing and that change is not included in this reroll.

Also, I'm curious why we don't add jQuery itself through hook_js_plugin(), since it seems like it would be a plainly helpful to know what version of jQuery is being run without having to do funny md5() hashes or regexs on the file contents.

sun’s picture

Status: Needs review » Needs work

This implementation is limited to jQuery, because it presumes that there can only be one "form", "farbtastic", "ui", or "whatever" plugin. That is true for http://plugins.jquery.com, but not at all for anything besides jQuery plugins.

The other properties are not used anywhere, they are of no use, and only introduce cruft.

quicksketch’s picture

Maybe it'd make sense then to rename our keys, instead of "form" do "jquery_form". I'm pretty sure there's only one "farbtastic", so it'd seem a little silly to rename it to something like "jquery_farbtastic".

As for other properties, surely "version" is an extremely helpful property to have. That way you can do something like this in your hook_requirements:

function mymodule_requirements($phase) {
  $requirements = array();
  if ($phase == 'install') {
    $plugin = drupal_get_plugin('jquery_ui');
    if ($plugin['version'] < 1.7) {
      $requirements['mymodule']['error'] = t('jQuery UI 1.7 or higher is needed before My Module can be installed. A module that provides an upgraded version of jQuery UI must be installed.');
    }
  }
}

In the case of jQuery Update (or jQuery UI or jQP), knowing the version of the plugin installed is extremely important. I think keeping the project page is a good idea too so the link to download the file can be referenced in other modules. Not so sure about the "download page", since that seems likely to change frequently (at least in jQuery's history).

markus_petrux’s picture

If this API includes the version, then does that mean when a site needs a new version which is already available from the original source, is the site forced to wait until the Drupal project that provides that particular library updates?

What if the project that wraps the library for Drupal usage provides a method to extract the version number from the library?

sun’s picture

Anyway, I'm out - just wanted to help chx. If you want to get this in and you don't want to add nonsense to core, then you do #140.

quicksketch’s picture

If this API includes the version, then does that mean when a site needs a new version which is already available from the original source, is the site forced to wait until the Drupal project that provides that particular library updates?

This situation will be the same as it currently is. If a new jQuery version comes out during the Drupal 7 release cycle (say 1.4 comes out), but Drupal 7 ships with 1.3, then you'll need a module to provide an updated version of jQuery. Drupal core itself will update for minor releases of jQuery (such as 1.3.3 when it comes out), but not for major versions since they usually introduce API changes.

Having a central place to ask "What version of jQuery do I have?" is very handy. Core doesn't provide a way to find this currently (you just have to "know") and jQuery Update has jquery_update_get_version(), which runs a regex on the actual jquery.js file. Keeping the version makes it possible for any module to know not only if a library is available, but which version, without having to know what module is providing that library or what the special mechanism is to get the version (if there even is one at all).

quicksketch’s picture

Status: Needs work » Needs review
FileSize
17.21 KB

Here's a reroll that hopefully will address most people's concerns. Changes:

  • Changed the hook names to "drupal_add_js_library", "drupal_get_js_library", "hook_js_libraries", and "hook_js_libraries_alter". Why "library"? Because it makes it clear that this can be used not just for jQuery plugins, jQuery itself isn't really a plugin at all, and because we already have the constant JS_LIBRARY as a weight value. It felt funny adding a "plugin" and giving it a JS_LIBRARY weight.
  • jQuery itself is added as a library so we can check what version of jQuery we're running.
  • All libraries are assigned the JS_LIBRARY weight unless specified otherwise (jQuery is still JS_LIBRARY - 2 to make it first).
  • Removed the "download" property from hook_js_libraries ("website" and "version" are still there).
  • Added a few more docs to drupal_add_js_library().
  • Renamed the jquery.form.js plugin to "jquery_form" instead of just "form". When we add jQuery UI, it should be "jquery_ui". Other plugins like Farbtastic, jCarousel, or ThickBox will probably skip the "jquery_" prefix, since they're all jQuery-specific projects.

I double checked update.php, install.php, Upload, and Garland color changing to make sure that jQuery would be okay being defined by system.module. It doesn't seem to have caused any problems and all our tests are still passing.

quicksketch’s picture

FileSize
17.21 KB

I found an inconsistency in the docs that had the $libraries variable returned in hook_js_libraries_alter(), which isn't needed because it's passed by reference.

quicksketch’s picture

FileSize
17.04 KB

Well fiddlesticks, more doc fixes. Needed to remove "download" from the PHPdoc and rename "project_page" to "website".

sun’s picture

Sure, you can just ignore.

(function($) {

$.farbtastic = function (callback) {
  $.farbtastic(this, callback);
  return this;
};

oh? Farbtastic is not a jQuery plugin? What's a jQuery plugin then?

Now you opened exactly that can of worms. If jQuery itself is a plugin/library, because jQuery UI is a plugin/library, then every other JavaScript in Drupal is a plugin/library, too.

If you manipulate one part in the dependency chain, many dependencies won't work any longer. I thought you would know that jquery_update ships with replacement scripts in misc/*.js ?

So why don't you ditch this approach and simply replace drupal_add_js() ?

quicksketch’s picture

FileSize
17.07 KB

Fixing a test that broke after renaming "form" to "jquery_form".

quicksketch’s picture

If you manipulate one part in the dependency chain, many dependencies won't work any longer. I thought you would know that jquery_update ships with replacement scripts in misc/*.js ?

Yes, that's true that when updating jQuery, other scripts also need to be updated. However, those scripts in misc/*.js don't really have official versions in any capacity (other than maybe their CVS ID). If Drupal does what it always does and doesn't break APIs within a release cycle, then those files don't have versions at all that we need to worry about, it's just the "Drupal 7" ahah.js or collapse.js file. So there's no point in providing version information on them.

Regarding Farbtastic, I'm just saying there's no point in referring to it as "jquery_farbtastic", since Farbtastic is a unique name that is a jQuery plugin only, there's no way we'll run into a name conflict there. Same as "thickbox" (which was the jQuery version of Lightbox) or "jcarousel" (where "j" stands for jQuery). The name implies its library.

Tons of modules will still use drupal_add_js(), as will themes. For example, Fivestar's fivestar.jquery.js isn't really a library (or at least it's not intended to be used that way), and specifying it as a library isn't necessary because it's "version" is whatever version of the fivestar.module you're running.

Basically the definition of JavaScript Library is "a project that's separate from Drupal". Is jQuery separate from Drupal? Or jQuery UI, jCarousel, or tinyMCE? Of course they are. They all have their own version numbers and releases. What about Fivestar, Panels, Views? These modules all add JavaScript, but they're not JavaScript libraries. drupal_add_js() and drupal_add_js_library() have very different uses and there's a huge amount of merit in being able to modify them separate from the loose JavaScript files used by modules and themes.

moshe weitzman’s picture

Looks good. I would ditch the cache_set('js_libraries', $libraries);. We can build this on every request. It is trivial - no DB queries. Lets avoid cache proliferation.

quicksketch’s picture

I'm not sure if we want to drop it or not. It's not saving us much (or anything) if using a database query to get it. A site with memcache might get a slight benefit. However if there is a module that dynamically adds libraries/plugins (like some people have mentioned), through a UI and database settings, then cache_get() might offer a more significant gain, as it would prevent those modules from needing to query the database separately.

moshe weitzman’s picture

IMO, the burden is on that module to do manage its cache. I think we should drop it from this patch.

quicksketch’s picture

FileSize
16.87 KB

Well, fair enough I suppose. Less caching always makes things easier to program for and less prone to error. Here's a reroll minus the cache_get() and cache_set() calls (the static cache is obviously still left in place).

quicksketch’s picture

FileSize
16.87 KB

Tiny change removing a double period at the end of a comment.

RobLoach’s picture

The reason I stuck in the caching was that if a contributed module, like jQP did some file checking to see where the plugins were, and then some regex to see which version it was, you'd end up with a pretty slow registry builder. jQuery UI's HEAD might be a good indication of how it performs without the caching. Check out jquery_ui.module.

quicksketch’s picture

Yeah I was thinking the same thing. I thought of this scenario after rolling the last patch. Moshe makes the point that individual modules could do their own caching, but at the same time that means that we might have multiple modules all doing their own caching, causing what could have been a single query into multiple queries. Unless modules start being allowed to bundle specific versions in the Drupal CVS repository, we won't have any way of setting the version number without either asking the user for what version they downloaded or doing some kind of automatic parsing of the downloaded files. Considering the significant amount of frustration already caused by having to download correct versions of certain plugins to get modules to work, my guess is that developers will try to take the route that has less user-error and try to solve it by regexing the downloaded code (such as in the jQuery UI code).

mfer’s picture

/me ogles over this code. I want this.

The one sticking point is over version numbers, regex, and caching. I would much prefer to just include the js code with a module which would make this less of an issue. But, for modules like jquery ui we aren't going to do that. Do we see the need for very many modules to need to preform a regex to get the version number? The only module I'm aware of that does this is jquery ui. Though, many of the other modules simply break the CVS policy and include the js with the module download.

If someone has 4 modules installed that cache get/set their scripts version numbers that's 3 more cache get/set requests than if drupal core did it for you.

sun’s picture

+ * A JavaScript library could consist of a jQuery plugin, a separate JavaScript
+ * framework (such as Prototype or MooTools), or a library such as a WYSIWYG
+ * editor. Any time that there are multiple versions of a JavaScript package

This implementation cannot be used for WYSIWYG editors.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.97 KB

Fixes some documentation that didn't include the recent update to version strings and version_compare().

Daniel, if the documentation example for a WYSIWYG editor isn't a family of related JavaScript or CSS, then should we change that to "or a library such as jQuery UI" instead? This documentation is just trying to explain to the reader that this functionality is for adding a related set of JS and CSS.

mfer’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested this code. It looks good to me and preforms as I would expect it to.

@sun - Can you please explain why this approach won't work for WYSIWYG editors? Is it the dependancy issue you noted in #151 or something else?

sun’s picture

Title: Centralized JavaScript library/plugin registry » Add some functionality for adding a related set of JS and CSS
Status: Needs review » Reviewed & tested by the community
mfer’s picture

Title: Add some functionality for adding a related set of JS and CSS » Centralized JavaScript library/plugin registry

The patch in #163 add a new method for adding libraries (like farbtastic) to a page that include js, css, settings, callback functions, and dependent libraries.

Using the hook hook_js_libraries() you define a library (including name, version, libraries yours is dependent on, js, and css). hook_js_libraries_alter is provided to alter this. An example library would be farbtastic. See the following definition:

function system_js_libraries() {
  // Farbtastic.
  $libraries['farbtastic'] = array(
    'title' => 'Farbtastic',
    'website' => 'http://code.google.com/p/farbtastic/',
    'version' => '1.2',
    'js' => array(
      'misc/farbtastic/farbtastic.js',
    ),
    'css' => array(
      'misc/farbtastic/farbtastic.css',
    ),
  );

  return $libraries;
}

Then to add farbtastic to a page you call drupal_add_js_library('farbtastic') instead of drupal_add_js/css.

This provides some nice possibilities such as version checking (see #144 for quicksketch example) and it deals with a simple dependancy structure.

Below is a more detailed version of what you can define.

+  $libraries['library-2'] = array(
+    'title' => 'Library Two',
+    'website' => 'http://example.com/library-2',
+    'version' => '1.2',
+    'js' => array(
+      // JavaScript settings can be added by sending in an array.
+      array(
+        'type' => 'setting',
+        'data' => array('library-2' => TRUE),
+      ),
+    ),
+    // Any depending libraries can be added by using the "libraries" key.
+    'libraries' => array(
+      // Sending in a string will just add the depending library to the page.
+      'library-1',
+      // Sending in the library name along with an array of parameters will be
+      // sent in as additional arguments to drupal_add_js_library().
+      'library-3' => array(
+        'additional arguments',
+      )
+    ),
+    // The library add handlers are all called when the library is added to the
+    // page.
+    'add' => array(
+      // The function named "library_2_added" will be called when library-2 is
+      // added to the page. The arguments passed to this function are the
+      // additional arguments that were passed during the call to
+      // drupal_add_js_library().
+      'library_2_added',
+    ),
+  );
mfer’s picture

An attempt at a better explanation.

This patch takes JavaScript libraries (things like jQuery, jQuery plugins, dojo js library) and provides a way to manage these packages of js files, css files, callback functions, and dependancies on other libraries.

If you wanted to call a library in drupal 6 you would do something like:

  drupal_add_js('library.js');
  drupal_add_js($settings, 'setting');
  drupal_add_css('library.css');

If you had a library with dependancies on another library this part could be twice as long.

With this patch the code would look like:

  drupal_add_js_library('library');

This same code would work even if there were dependancies on multiple other libraries.

The key is in the library registry. To add a JavaScript Library to drupal (to be used anywhere in drupal) you define it with hook hook_js_libraries(). There is a corresponding hook hook_js_libraries_alter() as well.

A simple example can be seen in the definition for farbtastic which looks like:

function system_js_libraries() {
  // Farbtastic.
  $libraries['farbtastic'] = array(
    'title' => 'Farbtastic',
    'website' => 'http://code.google.com/p/farbtastic/',
    'version' => '1.2',
    'js' => array(
      'misc/farbtastic/farbtastic.js',
    ),
    'css' => array(
      'misc/farbtastic/farbtastic.css',
    ),
  );

  return $libraries;
}

This defines the name, website, version, js, and css files for this library. When drupal_add_js_library('farbtastic') is called the js and css files defined here will be added to the page.

Let's take a look at a more complicated example:

function example_js_libraries() {
  $libraries['library-2'] = array(
    'title' => 'Library Two',
    'website' => 'http://example.com/library-2',
    'version' => '1.2',
    'js' => array(
      // JavaScript settings can be added by sending in an array.
      array(
        'type' => 'setting',
        'data' => array('library-2' => TRUE),
      ),
      'path/to/file.js',
    ),
    // Any depending libraries can be added by using the "libraries" key.
    'libraries' => array(
      // Sending in a string will just add the depending library to the page.
      'library-1',
      // Sending in the library name along with an array of parameters will be
      // sent in as additional arguments to drupal_add_js_library().
      'library-3' => array(
        'additional arguments',
      )
    ),
    // The library add handlers are all called when the library is added to the
    // page.
    'add' => array(
      // The function named "library_2_added" will be called when library-2 is
      // added to the page. The arguments passed to this function are the
      // additional arguments that were passed during the call to
      // drupal_add_js_library().
      'library_2_added',
    ),
  );
}

Just like the previous example a js file will be added when drupal_add_js_library('library-2') is called. In this example a js setting is added as well. Also, before this library is added the libraries library-1 and library-3 are added (in that order). This provides a simple dependancy map. After all the libraries, css, and js are added the function library_2_added will be called.

This callback can react to the addition of the js library. If drupal_add_js_library('library-2', $param1, $param2) is called the callback will be called as library_2_added($param1, $param2). With hook_js_libraries_alter() multiple modules can add their own callback functions here.

Js libraries defined here cross all modules and core. One module can define a library and another can use it as a dependancy.

While version numbers are defined here they are not used in the dependancy map. They can be combined with hook_js_libraries_alter() or hook_requirements() to provide some level of version number checking but this is not built into the system.

mfer’s picture

Status: Reviewed & tested by the community » Needs work

Found an issue.

So, we have the case where you call drupal_add_js_library('lib-1'). Note lib-1 is dependent on lib-2 and lib-2 has a callback function.

+  foreach (array('libraries', 'js', 'css') as $type) {
+    if (isset($library[$type]) && is_array($library[$type])) {
+      foreach ($library[$type] as $data => $options) {
+        // If the value is not an array, it's a filename or library name and
+        // passed as the first (and only) argument.
+        if (!is_array($options)) {
+          $data = $options;
+          $options = array();
+        }
+        // When drupal_add_js with 'type' => 'setting' is called, the first
+        // parameter ($data) is an array. Arrays can't be keys in PHP, so we
+        // have to get $data from the value array.
+        if (is_numeric($data)) {
+          $data = $options['data'];
+          unset($options['data']);
+        }
+        // All libraries are given a weight of JS_LIBRARY by default.
+        if (!isset($options['weight'])) {
+          $options['weight'] = JS_LIBRARY;
+        }
+        // Change the 'libraries' key to 'js_library' to match the function.
+        $function_type = $type == 'libraries' ? 'js_library' : $type;
+        call_user_func('drupal_add_' . $function_type, $data, $options);
+      }
+    }
+  }

In this case the automagic code wouldn't call drupal_add_js_lib('lib-2'). It would call drupal_add_js_lib('lib-2', $options). In this case $options at least has a weight to it.

Wouldn't it be better to use call_user_func_array so the arguments passed in can be defined by the hook_js_libraries() and remove the weight for libraries?

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.54 KB
  • Switches from "libraries" in hook_js_libraries() to "dependencies".
  • Switches from "add" in hook_js_libraries() to "add callbacks".
  • Addresses mfer's point in #168 by switching to call_user_func_array() for libraries. Works well.

I also updated jQuery UI's HEAD to use this and it turned into this to add an accordion widget on the page:

  drupal_add_js_library('ui.accordion', '#accordion');

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
17.96 KB

Forgot to change one of the "add callbacks". This patch also adds another test to see if dependencies are added.

sun’s picture

Title: Centralized JavaScript library/plugin registry » Add some functionality for adding a related set of JS and CSS
Status: Needs review » Reviewed & tested by the community

Tests passed.

To summarize:

- This patch allows to add a set of JS/CSS files to the page, for which at least one dependent module has to register those files through a hook. For Drupal core, this will be system.module.

- That registry allows all modules to alter or replace the loaded files and settings for a set of JS and/or CSS files (called "JS libraries").

- Most probably, it can be only used for jQuery and perhaps jQuery UI (i.e. jquery_update.module and jquery_ui.module), because other JavaScripts in Drupal modules usually rely on a certain version.

- This registry was primarily designed to work with jQuery and its plugins, so it only works with "libraries" that consist of JS and/or CSS files. It also does not work with external libraries.

- The registry is built once and cached for performance, which is good. Users, especially developers and themers, have to find out and learn that it is cached, which cache it is, and how to flush the cache.

- Registered libraries use some additional properties, which are not displayed or used anywhere. One of them being its version - which is intended to allow for easier manipulations of the JS library registry. How the resulting conflicts of such manipulations are handled is left to the end-user.

- All modules can register libraries, possibly overwriting registered definitions of other modules. A user who encounters strange JavaScript errors on his site has to consult the maintainers of the two or more modules registering the same JS library. At least one module maintainer will have to rename/alter the module's definition to resolve this conflict, because such conflicts are expected and there is no other way to resolve it.

- Contrary to its documentation, none of the array keys in hook_js_libraries() are required.

- Libraries can define dependencies. But neither the registry, nor the processing functions contain any validations to check whether a library having dependencies can actually be loaded. Module authors are expected to ensure that in some other way.

At the very least, hook_js_libraries() should have been named hook_js_libraries_info(). Likewise, hook_js_libraries_alter() should have been named hook_js_libraries_info_alter(). But who cares.

To learn about further reasons to not commit this approach/patch, you read the entire issue. However, I seem to be the only one who thinks that this approach is flawed and predicts that this will cause a wave of never-ending support requests throughout drupal.org, so you can just ignore me like everyone else did. Since you will do that anyway, I'm marking this RTBC, because I'm sick of repeating myself. Please just don't mind when I will add another tag to this issue later on.

webchick’s picture

sun: I've seen you criticize the issue several times. What I have not seen are alternatives that would address the issues you have identified. Unless I missed them in amongst the bitterness. I would very much welcome constructive suggestions here, since supporting WYSIWYG editors is obviously something we'd like this registry to be able to do.

* Renaming the hooks is no big deal. We can do that. Same with fixing documentation.
* Sorting out dependencies is indeed an important issue, but given how long it's taken for us to come to a consensus about even *registering* the information, I believe that this almost has to come in a follow-up issue, or we will never see this committed. See #133. This ties into a broader core-wide issue of dependency chaining (modules, updates, etc.)
* This registry was primarily designed to work with jQuery and its plugins, so it only works with "libraries" that consist of JS and/or CSS files. <= could you clarify? What other data needs to be included to satisfy your requirement for flexibility?
* It also does not work with external libraries. <= could you clarify? meaning I can't reference javascript.google.com/blargdeeblarg/something.js?
* The registry is built once and cached for performance, which is good. Users, especially developers and themers, have to find out and learn that it is cached, which cache it is, and how to flush the cache. <= is this a big deal? They already have to learn that the theme registry is cached, and that impacts them vastly more on a day-to-day basis. Why does one more front-end cache make a big difference?

quicksketch’s picture

Note: all the patches after #156 do NOT have caching between requests. However it's always easier to justify *more* caching, so I don't have any problem with leaving this to later patches.

mfer’s picture

Status: Reviewed & tested by the community » Needs work

@sun - can you please clarify why this approach won't work with external libraries and what needs to change for that to happen. The one problem I'm aware of it that drupal_add_css doesn't support external libraries. Is there anything beyond that? Also, why would this approach stop it from working for WYSIWYG?

* From a debugging standpoint where it says:

+  if ($library == FALSE) {
+    return FALSE;
+  }

can we get a watchdog call (and maybe a message)? We should know when an undefined library is being called to help track down the problem.

* Yes, all modules can define libraries and possibly overwrite previously defined ones. This same problem exists in other places like the menu system (see hook_menu). Any suggestions how we handle this?

mfer’s picture

Title: Add some functionality for adding a related set of JS and CSS » Centralized JavaScript library/plugin registry

@sun - Why do you not consider this 'Centralized JavaScript library/plugin registry'? You keep changing the name. I understand you disagree with the implementation. But, what about it isn't a JavaScript library/plugin registry? What's the difference between this and what you call 'related set of JS and CSS' in your opinion?

skilip’s picture

If we use the version parameter, it suggests we support using multiple versions per library. Currently tis is impossible while registering a new version of library 'foo' would overwrite the earlier registered library 'foo'.

So we'd need a different structure.....

  // First key (0), is treated as default version. If no version is specified when calling the library, this one will be loaded.
  $libraries['library-foo'][0] = array(
    'title' => 'Library Foo',
    'website' => 'http://example.com/library-foo',
    'js' => array(
      array(
        'type' => 'setting',
        'data' => array('library-2' => TRUE),
      ),
      'path/to/file.js',
    ),
  );

  // Register version 1.2.4
  $libraries['library-foo']['1.2.4'] = array(
    'title' => 'Library Foo',
    'website' => 'http://example.com/library-foo',
    'js' => array(
      array(
        'type' => 'setting',
        'data' => array('library-2' => TRUE),
      ),
      'path/to/file.js',
    ),
  );
RobLoach’s picture

Status: Needs work » Needs review
FileSize
18.93 KB

I think we should follow the same modal that the modules follow when it comes to supporting multiple versions at a time. You can't have CCK 1.0 and 2.0 installed at the same time. The "version" parameter is meant to be used by the alters (hook_js_alter and hook_js_libraries_alter) so that they can swap out JavaScript and CSS files appropriately if you're doing something like updating a script provided by other modules (like jQuery or Farbtastic). Adding multiple versions of the same script to the page at once will result in conflicts and is something anyone in the right mind should avoid. Wim Leers, webchick and mfer addressed this issue at #44. In summary, the alter hooks are meant to take care of this.

But, this does not mean that it's impossible. Using "add callbacks", you could in fact support multiple versions of the same script at the same time, if you really wanted to:

function library_foo_js_libraries() {
  $libraries['foo'] = array(
    'title' => 'Library Foo',
    'website' => 'http://example.com/library-foo',
    'add callbacks' => array(
      'library_foo_added',
    ),
    'version' => '1.0', // The version that's added by default
  );
}

/**
 * Called when library foo is added
 */
function library_foo_added($library, $version = NULL) {
  switch ($version) {
    case '0.8': // Give support for the old ass buggy version 0.8
      drupal_add_js('path/to/file-0-8.js');
    break;
    default: // But use version 1.0 by default!
      drupal_add_js('path/to/file-1-0.js');
  }
}

// Use Foo version 0.8 because we like using old ass versions of stuff
drupal_add_js_library('foo', '0.8');
// After adding version 0.8, lets add the latest version too!
drupal_add_js_library('foo');

This patch adds a watchdog message like mfer recommended in #175 if the library we're trying to add does not exist.

Title: Centralized JavaScript library/plugin registry » Centralized JavaScript library/plugin registry

The last submitted patch failed testing.

skilip’s picture

Status: Needs review » Needs work

@RobLoach:you're right. I haven't thought about conflicts when loading two version at the same page.

mfer’s picture

@RobLoach, do the JS tests load the right logging modules?

@skilip if we have multiple versions of the same script you'll run into JS namespace conflicts in most cases. Some setups like YUI 3 have made intentional ways around that but they are rare.

We don't expect modules written for CCK 1 to work with CCK 2. I wouldn't expect Views 3 based modules to work in View 2.

Checking the version numbers and knowing the ranges of versions that will work for each other creates as least 2 problems. First, if we release drupal 7 with jquery ui we won't know what future versions of jquery ui will work with the version of jquery we release with core. The only way to test that well is to say any future version could be an incompatibility. But, we don't know. They creates another layer of how we warn people and act on that. I would prefer this be in a separate issue.

The second issue is performance of checking versions. We have graph.inc in core which makes this easier to write with reusable code. But, I wouldn't want graph.inc running on every page. If we go that route it should be cached. Because of the size of that issue I'd prefer it be in a follow-up patch.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
18.91 KB

Hmm, strange....

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
18.05 KB

Whoa, for some reason, the tests from #264876: Allow external CSS files through drupal_add_css got in this patch.... Removed those hunks. Testing bot, hit it!

mfer’s picture

Status: Needs review » Needs work

The code looks good and the test pass. My only issue with this is some nit picking on the comment block above hook_js_libraries in system.api.php

+ * - js: An array of JavaScript elements to be processed when the library is
+ *   being added to the page. The array keys represents the $data parameter of
+ *   the drupal_add_js() call, and the value becomes the $options argument. If
+ *   the key is an integer, then the value becomes the $data parameter.

If I didn't know what the code already did the comment "If the key is an integer, then the value becomes the $data parameter." wouldn't make sense to me.

Then where it says

+ * - add callbacks: An array of function callbacks that will be called when this
+ *   library is processed on the page.

If I didn't know that the arguments passed into drupal_add_js_library() were passed onto the add callbacks i wouldn't learn them from this comment block. Something should be added there.

Other than these 2 things this patch looks ready to go.

kkaefer’s picture

  • When your module depends upon a certain JS library, you simply ship the library with your module and add it into the hook_js_libraries. If two modules add the same library it won’t be added twice. Is that correct? Is there a test for that feature?
  • I’m a bit concerned about the complexity of this patch. To me, it looks like we’re trying to do a lot of magic for very little benefit. Why do we treat JS libraries different from regular JavaScript or CSS files? I think we should merge both into one concept. That’d give us dependency checking for regular CSS/JS files as well.
  • Do we need a way to cleanly remove a JS library?
sun’s picture

FileSize
4.56 KB

When your module depends upon a certain JS library, you simply ship the library with your module and add it into the hook_js_libraries. If two modules add the same library it won't be added twice. Is that correct? Is there a test for that feature?

# php fail.php

No Drupal required to execute this script and see the results.

mfer’s picture

@kkaefer some answers to your points...

When your module depends upon a certain JS library, you simply ship the library with your module and add it into the hook_js_libraries. If two modules add the same library it won’t be added twice. Is that correct? Is there a test for that feature?

If a library is added twice via hook_js_libraries only the second one in the order of hooks being called will be used. In the registry it would over write the first ones entry. This could become a problem if each of the 2 cases are using different incompatible versions of a library.

If they used two different names (e.g., mymodule_jcarousel and mymodule2_jcarousel) they would both be loaded in the page and there could be conflicts.

Why do we treat JS libraries different from regular JavaScript or CSS files? I think we should merge both into one concept. That’d give us dependency checking for regular CSS/JS files as well.

JS libraries are different than something like comment.js in the comment module. It is only dependent on jQuery which is already automagically included in the page and there are no corresponding css files. How would a script like this work in the setup? Would you have to register every JS file you're going to call like this patch does? It seems to be a lot of work for cases where it's not a library but a one off script that uses a library.

Do we need a way to cleanly remove a JS library?

If you disable a module that adds a library it will remove it from drupal. An modules that depend on that library which are still enabled would get a warning in the watchdog. The dependencies in the module .info files should be used to make sure you have a module providing an outside library you're using.

mfer’s picture

@sun - thanks. good catch. I forgot about the internals of module_invoke_all. We need to do something like what menu_router_build does and avoid the recursive nature of module_invoke_all/array_merge_recursive.

sun’s picture

Yes, array_merge_recursive() is a minor issue. If you ignore the major issue of modulitis and the results of that script.

mfer’s picture

@sun what proposed solution/change do you have?

sun’s picture

In this order:

1) Rename the hook and implement this exclusively for core. Limited to jQuery UI and optionally also jQuery. Remove the documentation for this hook and keep it internal. jquery_update will be the only module that will implement it to alter those 2 js libraries - so the hook is primarily and only intended to be used by one dedicated contrib module. Most probably, jQuery won't make sense to use in this scheme, because jquery_update needs to additionally alter/replace several other JavaScripts in misc/ and elsewhere when jquery.js is updated. Also note that we are heading into a completely new territory with jQuery UI alone - there is down to zero experience with major/minor version conflicts of jQuery UI thus far - not even with the contributed jquery_ui module. Merge drupal_add_js_library() into drupal_add_js() to ease adding of jQuery UI effects.

2) Work on and resolve further issues for jQuery UI in core. Specifically issues like #454382: Provide jquery_ui_elements() for easier usage of plugins in other modules, for example.

Unrelated:

3) Solve the larger issue of a common registry - properly - in contrib. As already mentioned in #96. This has to primarily work in and for contrib. There is absolutely nothing required in core to find a working solution. If we will ever find a solution - and only if it happens to work reliably throughout contrib - we can start to think about moving that into core. Very obviously, none of the previously mentioned modules solved - and did not even try to solve - the major issues involved. (I'm repeating myself again here.)

If you still prefer to ignore the facts, then I recommend to proceed with #315035: Add jQuery UI elements to core, since several other issues are waiting for jQuery UI in core currently.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
22.26 KB
  • In #186, kkaefer suggested treating the libraries the same as regular JavaScript files. We were trying to do that in the beginning, but in #113 we discovered that we needed a way to get the already existing libraries (drupal_get_js_libraries). Being able to add libraries through drupal_add_js() would be a great addition though, so this patch adds that as well as a test for it.....
    // Add the jQuery library
    drupal_add_js('jquery', 'library');
    
    // Add jQuery with more arguments
    drupal_add_js('jquery', array(
      'type' => 'library',
      'args' => array(
        'Drupal', 'Rocks!' // Translates to drupal_add_js_library('jquery', 'Drupal', 'Rocks!');
      )
    ));
    
  • This patch also hits some of the documentation fixes that mfer suggested in #185.
  • The naming conflict issue can exist anywhere. Module names, CCK field type names, node type names, PHP class names, etc. It would be an easy fix like mfer suggested in 189. Think we should take this route? Thanks.
  • The problem with going straight to #315035: Add jQuery UI elements to core without a way to manage it is that we'd run into a huge issue down the line when we want to use a module to update it.
catch’s picture

I'm not sure why it's more of an issue to update jQuery UI from a module than it is to update jQuery core from a module? Just because there's more files?

webchick’s picture

@sun:

Solve the larger issue of a common registry - properly - in contrib.

As far as I'm concerned, that's what we've been trying to do for 3+ years now, to no avail. This "kick it to contrib to figure out" problem-solving approach has resulted in at least 4 competing solutions, none of which has resulted in wide-spread adoption. What this means in practice is that everyone who wants to expose a JS library of some kind in Drupal needs to either bet on one of the "centralized JS plugin manager" horses (resulting in users possibly having to install 2-3 such modules that do exactly the same thing in slightly different ways), or just make their own "add some js here" wrapper (as jjeff and I ended up doing with jQuery UI module, since we couldn't figure out any of the existing JS library wrappers). I find this completely unacceptable, and completely do not understand why this is being touted as a logical thing to continue doing in the future.

What we've seen since moving the issue to "core space" is that the authors of all of those various competing contrib solutions are working together to come to an agreement about an common, underlying base for defining these plugins in core, which can still be further extended in contrib if need be. I see getting this baseline "central place to find out what JS libraries are there" in core as a very positive thing, and I also am starting to see convergence from most of the other people who I generally identify as the JS experts in Drupal that this is a good thing.

So bearing in mind that the scope of this issue is basically only adding the ability to centrally register plugin data (stuff like dependency checking needs to be solved in a more general space because it affects many, many things aside from JS plugins), what needs to be added to the metadata array to make it useful to contributed modules such as WYSIWYG API? Or, what is your proposed alternative so that we don't continue with the confusing mess of competing JS plugin registry modules in contrib?

sun’s picture

That obviously means we spent 3+ years with modules that merge some arrays into a large array. No wonder none of them was adopted throughout contrib.

Just because there is no working solution yet does not warrant to put something in core that - obviously - will not work out.

It seems like people didn't invoke that script. So here comes the prettified output of the not-so-hypothetical example in there. Even if you solve the array_merge_recursive() issue, you will (hopefully) see that X modules can register Y libraries from which Z libraries can be the same library, but not the same library version, and each integration/implementation may require different options/settings.

By flattening all of this into one possible item, you are potentially breaking one of the modules that implements the library. If you do not unify, you still break the options, settings, callbacks, and dependencies. In all cases, you break the module that's incompatible with the registered version.

No, I don't see an improvement in something that is expected to break functionality.

array(
  'library-1' => array(
    'title' => array(
      0 => 'Library One',
      1 => 'Library One',
      2 => 'Library One',
    ),
    'website' => array(
      0 => 'http://example.com/library-1',
      1 => 'http://example.com/library-1',
      2 => 'http://example.com/library-1',
    ),
    'version' => array(
      0 => '1.2',
      1 => '2.1',
      2 => '0.7-beta1',
    ),
    'js' => array(
      0 => 'sites/all/modules/my_module/library-1.js',
      1 => 'sites/all/modules/cutting_edge_module/library1-2.1.js',
      'sites/all/modules/crappy_module/library-0.7.1.js' => array(
        'type' => 'file',
        'scope' => 'footer',
        'weight' => 20,
      ),
      2 => 'http://googleapis.com/library-1/0.7.js',
    ),
    'css' => array(
      'sites/all/modules/my_module/library-2.css' => array(
        'type' => 'file',
        'media' => 'screen',
      ),
      'sites/all/modules/cutting_edge_module/library1-2.1.css' => array(
        'type' => 'file',
        'media' => 'all',
      ),
      0 => 'sites/all/modules/crappy_module/library-0.7.css',
    ),
    'add callbacks' => array(
      0 => 'cutting_edge_add_expected_library_v2',
      1 => 'crappy_module_add_v0_7',
    ),
  ),
  'library-2' => array(
    'title' => array(
      0 => 'Library Two',
      1 => 'Library Two',
    ),
    'website' => array(
      0 => 'http://example.com/library-2',
      1 => 'http://example.com/library-2',
    ),
    'version' => array(
      0 => '1.2',
      1 => '1.2',
    ),
    'js' => array(
      0 => array(
        'type' => 'setting',
        'data' => array(
          'library-2' => true,
        ),
      ),
      1 => array(
        'type' => 'setting',
        'data' => array(
          'library-2' => false,
          'compatMode' => false,
        ),
      ),
    ),
    'dependencies' => array(
      0 => 'library-1',
      'library-3' => array(
        0 => 'additional arguments',
        1 => 'additional arguments',
      ),
      1 => 'library-1',
    ),
    'add callbacks' => array(
      0 => 'library_2_added',
      1 => 'cutting_edge_add_library',
    ),
  ),
);
mfer’s picture

@sun - I did run your script. That's why I suggested the alternative to array_merge_recursive. You make a good point about the issue of versions and dependencies with different versions. This is something Rob Loach and I spoke about earlier in IRC.

Two things....
1) You have not offered a solution to the problem. Just pointed it out.

2) Other modules within drupal contrib have this same issue. Do we expect modules written for use Views 2 to work with Views 3? How do we handled the modules designed to work with Filefield 2 now that version 3 is out? This problem of versions and dependencies on versions already exists. Though, I think with jQuery plugins and other libraries this problem will become larger.

I'd suggest having one module or a family or modules that deals with all the major jQuery Plugins (maybe the existing jQuery Plugins module) that lives in contrib. It could be a central source for other modules to use.

sun’s picture

Modules are unique. There can only be one Views module on your Drupal site. Therefore (and as long as Drupal core does not support specifying major/minor versions in dependencies) any module that wants to integrate with Views 2.x has to do:

/**
 * Implementation of hook_views_api().
 */
function content_views_api() {
  return array(
    'api' => 2,
    'path' => drupal_get_path('module', 'content') . '/includes/views',
  );
}

Libraries are not unique. They should be, sure. But libraries are not under our control (neither Drupal core's nor jQuery Plugin module's). As long as you do not remove all libraries from contributions in CVS and tell developers that "there can only be one Foo library and its version is 1.01", you will always have different versions with different options and different dependencies. That is, because their origin is external. In reality, we are talking about handling dependencies on third-party software that may exist multiple times in different fashions in the system.

Libraries are passive. An (active) module or theme just wants to use one. Libraries do not define or declare themselves; they are unknown and they do not exist. That means, every single module that wants to ship with and use one has to define it to let the system know. But even when the system knows all libraries, they can still exist multiple times in different variants -- each module implementing a library may require a certain variant of the library. And since every single of our 4,000+ contributed modules and themes can ship with yet another variant of a library, we are running in circles.

RobLoach’s picture

Supporting external libraries seems like its out of scope for this issue. This is just a central registry API that allows:

  • Registration of JavaScript libraries (hook_js_libraries). Other methods like through the .info file can be done in later issues.
  • Alter the registry (hook_js_libraries_alter)
  • Add related JavaScript and CSS when the library is wanted on the page (dependencies, js and css)
  • Take additional actions when the library is added (add callbacks)

Discovery of third party libraries, as well as a user interface to change/update them, can be supported through hook_js_libraries and hook_js_libraries_alter from a contributed module like jQP. All requirements of this from core are supported. The majority of contrib is also satisfied by this as well:

  • jQP's hook_jqp is pretty much what hook_js_libraries() does.
  • jQ's hook_jq is pretty much the same as hook_jqp().
  • Plugins also has a hook_plugins, which is pretty much also the same as hook_js_libraries(). Plugins also gives a user interface around it, which is pretty much a visual hook_js_libraries_alter() to update the plugins.
  • Skilip uploaded a JavaScript Libraries module, which is the same as hook_js_libraries, except for the addition of a user interface, and the ability to add multiple versions of the same plugin at the same time, which I showed was actually possible to do with this patch.
  • jQuery Plugin would gain huge benefit from this because it could provide the discovery of the provided plugins and stick them in the registery through hook_js_libraries.

Please note the scope of this issue. All we are doing here is providing a way to add related JavaScript and CSS files at the same time, while allowing those libraries to be easily updatable through contributed modules.

Should we put the naming conflict issue fix in this patch, or a later patch? Please note webchick's kittens, this is already a 20KB patch.

sun’s picture

If you remove 'js', 'css', and 'add callbacks' from hook_js_libraries(), then your assumptions are true. These assumptions are two-dimensional, while the reality is three-dimensional.

Supporting external libraries seems like its out of scope for this issue. This is just a central registry API

Lessons learned: Stop following up on this issue. It's completely ignored anyway.

Status: Needs review » Needs work

The last submitted patch failed testing.

mfer’s picture

@sun - Do you have a proposed solution?

RobLoach’s picture

Status: Needs work » Needs review

I'm kicking this back at the testing bot.....

catch at #194: I'm not sure why it's more of an issue to update jQuery UI from a module than it is to update jQuery core from a module? Just because there's more files?

Catch made a good point here because we could do the whole thing through hook_js_alter(), but I think it would be beneficial to see why hook_js_libraries would help us by having a look at how jQuery Update module updates the jQuery library across different Drupal versions would :

  • In Drupal 5, jQuery Update asked your to replace jquery.js with its version, and then gave a "compat.js" for compatibility. This is rather messy.
  • In Drupal 6, it got a bit better with hook_preprocess_page(). It would modify the $scripts variable and swap out the .js file in the script itself. This is a bit better then swapping out the file itself, but is still kind of nasty.
  • In Drupal 7, it's really nice because we just use hook_js_alter to replace the referenced jquery.js file.

So, hook_js_alter() is really nice to have. But, the problem with using hook_js_alter() for a full JavaScript library that comes with some CSS, or has another library dependency, is that on every call, you'd have to traverse through hook_js_alter and make sure the updated files are being used. You'd also have to make sure to do that with CSS as well. On top of that, there's not really any way to cache the changes you want to make in hook_js_alter(). If we had hook_js_libraries, that alter caching is possible.

mfer’s picture

@Rob Loach can you roll in the change from #189 so we don't end up with a mixing of namespaces.

RobLoach’s picture

FileSize
24.36 KB
  • Adds duplicate library name conflict resolution based on the version number (drupal_get_js_library)
  • Provides an additional test for the name conflict resolution (common_test_js_libraries)
  • Adds back the caching because I can see building the registry every time someone hits the slow if you have a lot of contrib that implements hook_js_libraries (drupal_get_js_library)

If there are two different modules that define "farbtastic" in hook_js_libraries, it will take the one that has the latest version number. Although this can cause issues if the library's API changes between the different versions, it's probably the best solution we can take with this being in core.

I attempted what sun presented at #196 and drupal_get_js_libraries() returned the following:

Array
(
    [library-1] => Array
        (
            [title] => Library One
            [website] => http://example.com/library-1
            [version] => 2.1
            [js] => Array
                (
                    [0] => /library-1.js
                )

            [css] => Array
                (
                    [/library-2.css] => Array
                        (
                            [type] => file
                            [media] => screen
                        )

                )

        )

    [library-2] => Array
        (
            [title] => Library Two
            [website] => http://example.com/library-2
            [version] => 1.2
            [js] => Array
                (
                    [0] => Array
                        (
                            [type] => setting
                            [data] => Array
                                (
                                    [library-2] => 1
                                )

                        )

                )

            [dependencies] => Array
                (
                    [0] => library-1
                )

            [add callbacks] => Array
                (
                    [0] => library_2_added
                )

        )

    [jquery] => Array
        (
            [title] => jQuery
            [website] => http://jquery.com
            [version] => 1.3.2
            [js] => Array
                (
                    [misc/jquery.js] => Array
                        (
                            [weight] => -102
                        )

                )

        )

    [jquery_form] => Array
        (
            [title] => jQuery Form Plugin
            [website] => http://malsup.com/jquery/form/
            [version] => 2.16
            [js] => Array
                (
                    [0] => misc/jquery.form.js
                )

        )

    [farbtastic] => Array
        (
            [title] => Farbtastic
            [website] => http://code.google.com/p/farbtastic/
            [version] => 1.2
            [js] => Array
                (
                    [0] => misc/farbtastic/farbtastic.js
                )

            [css] => Array
                (
                    [0] => misc/farbtastic/farbtastic.css
                )

        )

)
kkaefer’s picture

To be honest, I agree with sun in that this patch adds a lot of complexity for only little benefit. Yes, the js_alter hook in jQuery UI update module might be smaller (can someone write actual implementations to demonstrate that it is in fact significantly less code); however, it adds approximately 200 lines of code (not counting tests and documentation).

What about unifying js and css adding into one function, e.g. `drupal_add_component`. This function could take an array of files (css AND js files) and just store it. These arrays are treated as groups (they could optionally be named, for example). You could then easily overwrite or remove a "group of related files". When we finally generate the actual HTML code for including the files, we finally iteratively merge the group. (This would replace, not augment drupal_add_css and drupal_add_js)

kkaefer’s picture

Example:

<?php

// Defining a new component and adding it to the page.
drupal_new_component('farbtastic')
  ->require('jquery')
  ->require('jquery_ui', '1.7')
  ->add_js('misc/farbtastic.js')
  ->add_css('misc/farbtastic.css')
  ->add_to_page();

// Retrieve and add a component to the page.
drupal_component('jquery_ui.selectables')->add_to_page();

// hook_ui_component_NAME
function system_ui_component_jquery($local, $version) {
  return drupal_new_component('jquery', 1.3)
    ->add_js('misc/jquery.js');
}
Dries’s picture

I've some more thinking, reading and testing to do, however, right now, I'm leaning towards sun and kkaefer on this. I'm not in any way a JS person, so maybe I've not been exposed enough to the library/dependency horrors. I'd like to see us add some of jQuery UI to core (but not everything), and it is not clear what this registry approach can _actually_ solve in that regard. In fact, it looks to me like it would be able to solve very little.

kkaefer’s picture

FileSize
3.58 KB

This is a small proof of concept. It would give us dependency checking for free. We could just include parts of jquery UI and the dependencies are automatically included as well. Update modules can overwrite the components (by calling ->reset() and adding their own files). There are of course a lot of hooks and functionality missing, but it's only a demo to show where js/css file addition could be headed.

kkaefer’s picture

The output is of course

<script type="text/javascript" src="/sites/all/modules/mymodule/jquery-1.5.js"></script>
<script type="text/javascript" src="/misc/drupal.js"></script>
<script type="text/javascript" src="/misc/farbtastic.js"></script>
webchick’s picture

@kkaefer: Was the correct file uploaded? That contents of that file were drupal_static..?

Frando’s picture

webchick: read on, I think it is the correct file, it just includes drupal_static() because it does not Drupal but it needs drupal_static().

For the record, I quite like kkaefer's idea. Will try to review more closely soon.

webchick’s picture

And that, my friends, is why you should get more than 4 hours of sleep before reviewing issues. ;)

Hm. Not sure about this approach, but I'm not necessarily opposed. I'd like to hear from someone like Rob Loach and mfer. There is an awful amount of work we've done this release to make drupal_add_js() and drupal_add_css() flexible enough to support things like weighting, altering, adding external libaries, etc. (although drupal_add_css() is lagging behind a bit).

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

If we are to take a new direction can we please close down this issue and start another? it's not easy to work with 200+ followups issues.

skilip’s picture

@chx: Yeah, I totally agree on that. That new direction is awesome indeed but just will bring a whole lot of other issues (and work). We just can't wait any longer having a library manager in core. With only 10 weeks before the codefreeze, a lot of UI stuff to b built, we need this yesterday! So let's put this in core and start bikeshedding in new issues.

webchick’s picture

I'd rather not close this issue until we have consensus on the way forward. There's a lot of knowledge captured here, and all of the heavy-hitters from our JS team are also subscribed.

RobLoach’s picture

The class/object approach is a neat idea, but it becomes difficult when we want to cache the data, or perform alters on it. The hook/array approach is the method taken in the rest of core, so why not take that route here? If Handlers were in core already, this would be the method that we would take with this issue, but it's not. Even still, we don't want to completely swap out JavaScript Libaries, we want to alter them. We also have to cache the fully built registry, and doing that through objects rather then an array with an alter hook can be difficult. What you proposed here can all be accomplished with the array/hook structure in this patch, the same array/hook structure that's also used in all the other JavaScript-related contributed modules (jQ, jQP, jQuery Plugins, etc).

Don't get me wrong, I love chaining, but we're building a library registry here, not refactoring drupal_add_js/css. Once drupal_add_css catches up to the functionality that we stuck into drupal_add_js, we might be able to refactor it to drupal_add_component(), but they are too different right now. That's the main reason why we split up the original #251578: More flexible js/css ordering and an alter operation into a bunch of different issues. If you upload a patch that has the "add callbacks", the name conflict resolution, the alter hook, along with all the tests that this patch provides, I'd love to take a look. But seriously, keep track of the scope here.

jQuery UI 7.x-2.x-dev provides hook_js_library which would update the version of jQuery UI in core. Thanks to sun bring up the naming conflict issue, it now would find that a newer version of jQuery UI exists, and use the newer version. See jquery_ui.module. To add a jQuery UI component to the page, you just have to call:

  drupal_add_js_library('ui.accordion', '#accordion');

This adds the required files for ui.accordion (both JavaScript and CSS), as well as uses the "add callbacks" to probably manage the Drupal JavaScript behaviors to add the accordion on the page (see jquery_ui_help).

Doing the same with hook_js_alter() and hook_css_alter() would be possible, but those hooks could not be cached (also not to mention that we'd be missing the dependencies and add callbacks). This means that every time you have JavaScript or CSS on the page, it would go through the alters and have to do an isset() on each CSS file to see if it has to be replaced. This would be a performance hit, which is why we want to take the registry path so that the alters, callbacks and dependencies are managed for us.

webchick’s picture

Ok. There were pretty compelling arguments in #315035: Add jQuery UI elements to core to get jQuery UI in first in advance of this patch. Now we have an actual use case in core, so we can move out of the theoretical zone and into the practical one. Also, now some of the important D7UX work that requires this library can progress.

mfer’s picture

I see the same thing Rob Loach pointed out. That this looks like a re-factoring of drupal_add_js/css.

@kkaefer, what hooks would be included in the code you posted above? What would the hook implementations look like? I'm interested in the registry part of the code.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
90.64 KB
31.2 KB

This patch explicitly defines the jQuery UI libraries that were just committed in #315035: Add jQuery UI elements to core with correct dependency resolution. I didn't implement the add callback or autodiscovery that you see in jquery_ui.modue though. To see it in action, apply the patch and place the the code found here in a node with the PHP filter. You'll end up with what you see in the attached screenshot.

  drupal_add_js('ui.accordion', 'library');

The "add callbacks" implementation would remove the need for the inline JavaScript as the Drupal JavaScript behavior would apply the Accordion widget for you:

  drupal_add_js_library('ui.accordion', '#accordion');

Thoughts?

mfer’s picture

I'm a little more clear after talking to Rob Loach on kkaefers code.

After a little thought I have a few comments:

  • What's a component? People know what JS, CSS, and a JS Library are but what's a component? This seems confusing.
  • So, we would move from drupal_add_js('path/to/comment.js'); to something like drupal_add_component('comment')->require('jquery')->add_js('path/to/comment.js')->add_to_page();. OK, maybe it's short than this but it would be longer than a drupal_add_js call. Do we want to add more complexity to drupal?
  • I do like the OOP style code. Instead of unifying everything I'd prefer something that uses drupal_add_js/css for one off files (not libraries) and drupal_add_js_library() as a OOP setup to add libraries with optional dependancies, js/css that goes along with calling it, etc. This seems like it would be better for DX.
mfer’s picture

oops, cross posted

mfer’s picture

What about something like this:

For one off js files:

  drupal_add_js('path/to/file.js');

For one off css files:

  drupal_add_css('path/to/file.css');

For JS libraries:

  drupal_add_js_library('ui.dialog')
    ->require('ui.resizable')      // Add the optional dependancy for ui.dialog
    ->add_js('path/to/file.js')    // This file uses ui.dialog
    ->add_js(array('a' => 'setting'), 'setting')
    ->add_css('path/to/file.css')  // Add some custom css for this
    ->add_to_page();

Doing this for libraries puts all the library stuff for a particular instance in one package.

webchick’s picture

OKAY!

Rob Loach, mfer, sun, and I spent some time on #drupal tonight trying to come up with a real world use case to demonstrate sun's concerns. We found one. This should help add some clarity to why sun is so against the current state of the registry.

Picture it: Your fancy Drupal website, which has an event calendar provided by Event module, a historical timeline provided by a view of Historical Timeline Items nodes with ISO 8601 CCK Date fields on them, and a "Birthdate" field on user profiles brought to you by Profile module. All of them use a variation of datepicker.js -- Event module comes with datepicker.js version 1.7, Date module comes with datepicker.js 2.5, and your custom module adds datepicker.js 3.0 for profile fields.

There is never a point where all three of these date fields will be visible at the same time; in other words, the node edit form for Events will never be the node edit form for Historical Timeline Items, and will never be the user edit form with the birthdate field on it. All three JS files have to be loaded with different settings so that they can return different output for their respective date fields, since they all store their data natively in a different format.

The question before us is: How do we build a registry that allows these things to co-exist without stomping on each other?

Dries’s picture

I think the example in #225 is valid, yet flawed. In many cases, as a developer, we can't guarantee that all three won't be visible at the same time -- especially now Fields API is in core.

Swapping in/out JS libraries and trying to resolve version dependencies is just _never_ going to work well.

Instead, we should ask module developers to use the jQuery.UI library in core, and in case that is not possible, they should namespace their JS, for example, by prefixing their JavaScript function names with the module name. Hopefully this isn't necessary in the majority of the cases, but in case it is necessary, can we provide a little script that helps them with that? The script could take a JS file as input, and output a namespaced one? There are some Javascript compression scripts that might give us what we need. Or, in those cases, they can still resort to one of the registries in contrib? Thoughts?

RobLoach’s picture

FileSize
32.03 KB

Module names prefixing library names
Dries beat us to it! As webchick mentioned, mfer, tha_sun and I talked about this tonight and came up with the solution of prefixing the library names with the module name the implements it. This respects the the version dependencies, as well as encourages module developers to work together to use the same libraries. Instead of having the module developers manually prefix the library names with the module names, however, this is automatically done through the registry.

This results in the following available libraries in core:

  // System module implements "jquery" and "farbtastic".
  drupal_add_js('system.jquery', 'library');
  drupal_add_js('system.farbtastic', 'library');
  drupal_add_library('system.ui.dialog');

  // MyModule implement "library1".
  drupal_add_js('mymodule.library1', 'library');

This way, if someone wants to use a version of Farbtastic that has a different API, they can reimplement their own version with the special API, using "farbtastic" as the key, and then add it using drupal_add_js('mymodule.farbtastic', 'library');. Then they have the option to either use mymodule's Farbtastic, or Drupal core's Farbtastic. At the same time, if they want to build off of Drupal core's Farbtastic, they can still use the alter hook with "system.farbtastic". I updated the tests and documentation to reflect that.

Not only JavaScript libraries
Sun also brought up that this is not restricted to JavaScript libraries, it could also be used for CSS libraries like the 960 Grid System. Webchick asked to remove the "js_" prefix. They also brought up that Drupal hooks are singular (hook_theme, hook_block, hook_menu, etc). I updated the documentation and the functions to reflect:

  • drupal_add_library()
  • drupal_get_library()
  • hook_library()
  • hook_library_alter()
chx’s picture

Status: Needs review » Needs work

Separate out system. to a separate argument. ui.dialog is a JS name, system is a Drupal module name, meshing them into one string is confusion. Aliases are bloat, scratch that.

Edit: and add a watchdog warning when adding the same library twice -- this is not hte usual we do not babysit broken code stuff.

RobLoach’s picture

Issue tags: +jQuery UI

I talked with chx about this a bit and he brought up three points:

  1. The module name prefixing the library name with a "." was a bit confusing. Instead he suggested adding the module name as an argument in drupal_add_library().
  2. The drupal_add_js('mylibrary', 'library') alias is bloatware and I tend to agree. We can remove that. Since this does not necessarily have to be JavaScript libraries, it seems that then we'd also have to have drupal_add_css('mylibrary', 'library'). Just remove it and use drupal_add_library() instead.
  3. Now that we can add two different libraries of the same name (System's Farbtastic vs CCK's Farbtastic), you can run into a conflict when they are both added at the same time. chx asked to throw out a watchdog message when this happens.

EDIT: Yay for cross posting!

sun’s picture

Assigned: Unassigned » sun
sun’s picture

Title: Centralized JavaScript library/plugin registry » Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries)
Status: Needs work » Needs review
FileSize
29.03 KB

- We discussed drupal_add_library($module, $name). I don't understand why it was not in the patch. Changed.

- I removed the convenience "feature" that one can simply register JS/CSS files without $options, where the array element value would automatically turn into the array element's key. That is only confusing and leads to an inconsistent registry structure.

- Adding jquery.js as library still doesn't make much sense. At the very least, all libraries would have to define it as dependency. But currently, that makes no sense, because we unconditionally add drupal.js resp. the library when any JavaScript is added to the page. That, we can discuss in a separate issue.

- I replaced the hook_library_alter() example with something more common. Swapping out libraries to use a later version is the job of dedicated modules like jquery_update/jquery_ui (to be merged) and should not be done by arbitrary modules, and thereby, we should not teach how that can be done.

Everything else is documented in-code.

sun’s picture

FileSize
30.03 KB

drupal_add_library() may be called multiple times for the same library to pass further settings to any defined 'add callbacks'. RobLoach implemented some nifty code in jquery_ui HEAD to use 1 add callback for all jQuery UI effects/plugins.

sun’s picture

FileSize
30.12 KB

However, of course, we still need $library information to execute any add callbacks multiple times.

moshe weitzman’s picture

I am taking advantage of the fact that all the right folks are watching this issue to voice once of our goals:

drupal_add_css(), drupal_add_js(), and drupal_add_library should usually be avoided. our new drupal_render() page building model has a much better mechanism for adding these elements. namely, the #attached_css and #attached_js properties that drupal_render supports. this mechanism is better because we can safely cache renderable elements. see #495968: Introduce drupal_render() cache pattern. Start using it for blocks. drupal_add_css/js/library are side effects which can't easily be cached.

If we add a library feature here, it has to have a way to use it from a render structure like #attached_css and #attached_js.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dries’s picture

Status: Needs work » Needs review

I like what sun has done in #233. I only spent about 15 minutes reviewing the code so it needs a more in-depth review still. I hope other people, especially some JS people take a closer look at this.

Some first comments:

- When reading the documentation of hook_library(), I did not understand the purpose of the 'add callback'. It might be good to hint at a common use case in the PHPdoc to make it more practical. Second, 'is added to the page' means the library is registered, or when the output is being generated? The word 'add callback' makes me believe the hook is called when the library is added to the registry. If the hook is called when the code it output to the page, I'd call it 'render callback' or something. Is it called before or after the output is generated? I assume it is called before.

- Instead of writing 'contrib modules' let's write 'contributed modules'.

Thanks. It's been a long issue, but I think it was well worth it. Now we have our direction, let's drive it home.

Dries’s picture

I like what sun has done in #233. I only spent about 15 minutes reviewing the code so it needs a more in-depth review still. I hope other people, especially some JS people take a closer look at this.

Some first comments:

- When reading the documentation of hook_library(), I did not understand the purpose of the 'add callback'. It might be good to hint at a common use case in the PHPdoc to make it more practical. Second, 'is added to the page' means the library is registered, or when the output is being generated? The word 'add callback' makes me believe the hook is called when the library is added to the registry. If the hook is called when the code it output to the page, I'd call it 'render callback' or something. Is it called before or after the output is generated? I assume it is called before.

- Instead of writing 'contrib modules' let's write 'contributed modules'.

Thanks. It's been a long issue, but I think it was well worth it. Now we have our direction, let's drive it home.

sun’s picture

FileSize
31.28 KB

1) Making tests pass.

2) Google Analytics makes my browser freeze and go nuts in this issue. Whether 'add callbacks' is a proper term or whether there may be a better one, we can defer to another issue, please.

3) The entire logic in the description of hook_library_alter() was flawed. Now improved.

4) Created #505084: Add #attached_library FAPI property for drupal_add_library(), because we can be safely deal with that separately.

However.

5) I should have better listened to my stomach. We solved the major registry issue to some extent now. But the fundamental concept of 'add callbacks' is totally flawed. "Why" is documented in-code, PHPDoc of hook_library(), because I want all of you to read, understand, and think about the code we add here.

That said, one possible option to move forward would be to ditch the 'add callbacks' property entirely for now, and - perhaps - find a working solution in a separate issue.

mfer’s picture

Someone correct me if I'm wrong but I believe the idea behind the 'add callbacks' was to be able to do something like:

  drupal_add_library('system', 'ui.accordian', '#accordion');

This would add the library and then pass #accordion to a callback function that would add the JS to apply the accordion to #accordion. The flaw I see in this is that it forces you to use this method to apply something like the accordion to an element in a page. It removes flexibility. I would want something like this to be optional or pushed to another issue where we can spend some time discussing it.

In the tests there is:

+function common_test_library() {
+  $libraries['farbtastic'] = array(
+    'title' => 'Farbtastic: Test library',
+    'website' => 'http://code.google.com/p/farbtastic/',
+    'version' => '5.3',
+    'js' => array(
+      'misc/farbtastic/farbtastic.js' => array(),
+    ),
+    'css' => array(
+      'misc/farbtastic/farbtastic.css' => array(),
+    ),
+  );
+  return $libraries;
+}

Drupal core currently has a forked version of 1.2. When farbtastic 1.3 is out I'll make an issue for that. We should set the version to 1.2 for now. (time to head back to my corner and get 1.3 out)

The tests pass and I went through where we use the libraries manually and they work. Yay for that.

If we fix the documentation Dries noted, moved the add callbacks to their own issue (or made them optional/removed their core use), and got a few other JS folks to weigh in this should be ready to go. I'm leaving this as 'needs review' so we can get some more JS folks to review and weigh in.

mfer’s picture

Someone correct me if I'm wrong but I believe the idea behind the 'add callbacks' was to be able to do something like:

  drupal_add_library('system', 'ui.accordian', '#accordion');

This would add the library and then pass #accordion to a callback function that would add the JS to apply the accordion to #accordion. The flaw I see in this is that it forces you to use this method to apply something like the accordion to an element in a page. It removes flexibility. I would want something like this to be optional or pushed to another issue where we can spend some time discussing it.

In the tests there is:

+function common_test_library() {
+  $libraries['farbtastic'] = array(
+    'title' => 'Farbtastic: Test library',
+    'website' => 'http://code.google.com/p/farbtastic/',
+    'version' => '5.3',
+    'js' => array(
+      'misc/farbtastic/farbtastic.js' => array(),
+    ),
+    'css' => array(
+      'misc/farbtastic/farbtastic.css' => array(),
+    ),
+  );
+  return $libraries;
+}

Drupal core currently has a forked version of 1.2. When farbtastic 1.3 is out I'll make an issue for that. We should set the version to 1.2 for now. (time to head back to my corner and get 1.3 out)

The tests pass and I went through where we use the libraries manually and they work. Yay for that.

If we fix the documentation Dries noted, moved the add callbacks to their own issue (or made them optional/removed their core use), and got a few other JS folks to weigh in this should be ready to go. I'm leaving this as 'needs review' so we can get some more JS folks to review and weigh in.

dmitrig01’s picture

I thought the plans were to switch to the jQuery UI color picker in the future but I could be wrong. Let's not lengthen the issue any farther (sorry!)

RobLoach’s picture

dmitri at #241: I thought the plans were to switch to the jQuery UI color picker in the future but I could be wrong. Let's not lengthen the issue any farther (sorry!)

jQuery UI Color Picker was removed from jQuery UI 1.7 lifespan due to the lack of IE6 support. They might add it back in later, but not in 1.7.

mfer at #240: I believe the idea behind the 'add callbacks' was to be able to do something like:

  drupal_add_library('system', 'ui.accordian', '#accordion');

Sun brought up the problem that it's missing context. Matt and I have been talking about this. The problem is that once you set the "add callbacks", there isn't any context. Whenever you call the function, all callback functions are passed through with the same arguments. If you want to have another "add callback" with different arguments, you're out of luck.

What I'm proposing we do here is add another dimension, like we did when we added the module context to the libraries. Instead of just calling drupal_add_library('system', 'ui.accordian', '#accordion'); to add both the Accordion library and the right settings/behaviors to process the accordion on the page, we add in another argument to add context to it...

  drupal_add_library('system', 'ui.accordian', 'process', '#accordion');

Here, you see that we're passing "process" as our context. In the "add callbacks", it would call every function within the "process" callback family....

  $libraries['ui.accordion'] = array(
    'title' => 'jQuery UI: Accordion',
    // ...
    'add callbacks' => array(
      // Provide the "process" callback family.
      'process' => array('mycallback'),
    ),
  );

function mycallback($module, $name, $context = NULL, $selector = NULL, $options = NULL) {
  // Add the settings and behaviors to process the selector and options.
  // This means that when we add an accordion, we can automatically
  // have the JavaScript process the element on the page.
}

The drupal_add_library() function's signature would become:

function drupal_add_library($module, $name, $context = NULL, ... ) {

When $context is not passed, the "add callbacks" wouldn't be called since there isn't any context. The libraries would simply be added to the page.

Dries’s picture

Why would we want to add the right settings/behaviors using the same drupal_add_library() call? Why can't the module that calls drupal_add_library(), add the settings/behaviors separately? Wouldn't that provide the module the flexibility and context it needs?

mfer’s picture

@dmitrig01 - as Rob Loach said, the jQuery UI color picker has been removed. At the same time farbtastic isn't dead. See http://code.google.com/p/farbtastic. There is a version 1.3 nearing (hopefully) release and a v2 that's in good shape (if it weren't for IE6).

RobLoach’s picture

Priority: Normal » Critical
FileSize
28.03 KB

Although the "add callbacks" are eventually required with this, there is no reason why it should stall this issue. I've removed the "add callbacks" and related documentation/tests. We can push the contextual addition of JavaScript/CSS libraries to a future issue.

Since jQuery UI is in core now, we need a way to manage it, so I'm pushing this issue to critical.

Owen Barton’s picture

A agree that the callback needs more thought, if it is even needed at all - my instinct is that if modules want to provide more functionality than the raw JS library they will code their own "native" high level PHP/JS API specific to the use case (that would call drupal_add_library as needed).

+ *   in a different version or variant. This registry does not attempt and
+ *   cannot prevent library conflicts.

I assume this should be something like "This registry cannot (and does not attempt to) prevent library conflicts." or "This registry does not attempt to, and cannot, prevent library conflicts." (grammaristas?).

+            // For JS settings, $data must be an array. Arrays cannot be keys in
+            // PHP, so we transform $options['data'] into $data.
+            if (isset($options['type'], $options['data']) && $options['type'] == 'setting') {
+              $data = $options['data'];
+              unset($options['data']);
+            }

This comment took me a couple of reads to get - could this be explained/coded in a more simpler way?

The documentation uses drupal_get_path('module', 'my_module') . '/library-1.js' - there have been discussions about using sites/library/ or similar to improve code separation and simplify module upgrades. This may be out of scope for this issue, but I thought it was worth surfacing as a future TODO.

sun’s picture

Needs work, but also wants to be tested.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

In drupal_render, you see the work that kkaefer did on #attached_js and #attached_css. Definitely not necessary, but would there be anyway that we could use the same code for the libraries in drupal_add_library() without having to druplicate it?

  // Add additional CSS and JavaScript files associated with this element.
  foreach (array('css', 'js') as $kind) {
    if (!empty($elements['#attached_' . $kind]) && is_array($elements['#attached_' . $kind])) {
      foreach ($elements['#attached_' . $kind] as $data => $options) {
        // If the value is not an array, it's a filename and passed as first
        // (and only) argument.
        if (!is_array($options)) {
          $data = $options;
          $options = NULL;
        }
        // When drupal_add_js with 'type' => 'setting' is called, the first
        // parameter ($data) is an array. Arrays can't be keys in PHP, so we
        // have to get $data from the value array.
        if (is_numeric($data)) {
          $data = $options['data'];
          unset($options['data']);
        }
        call_user_func('drupal_add_' . $kind, $data, $options);
      }
    }
  }
RobLoach’s picture

The reason the test is breaking is because you removed the following from

  if (isset($libraries['system']['farbtastic'])) {
    // Change the title of Farbtastic to "Farbtastic: Altered Library".
    $libraries['system']['farbtastic']['title'] = t('Farbtastic: Altered Library');
    // Make Farbtastic depend on jQuery Form to test library dependencies.

The test checked for 'Farbtastic: Altered Library', but since that alter isn't there anymore, the test fails.

sun’s picture

Status: Needs work » Needs review
FileSize
27.33 KB

Final pass.

- Removed the DB caching, because that is superfluous without any "real", worth-to-cache registry information. We can discuss to re-introduce it in a separate issue, but for now, there is no point in keeping it. For this sake, I kept drupal_get_library() as separate information getter function.

- One minor @todo is remaining, but that can be deferred to another issue as well.

Looks ready to go for me.

@Owen Barton: The idea and concept of using sites/all/libraries as a separate "repository" for external libraries (not shipped with a module) is outside of the scope and outside of the possibilities of this implementation. External libraries need to be treated differently and perhaps even all libraries need to be treated differently. That is what we want to figure out in http://drupal.org/project/libraries. Time will tell.

RobLoach’s picture

RobLoach’s picture

FileSize
26.98 KB
  • Includes jquery.cookie.js that was just added at #507072: Add jquery cookie library.
  • Removes the @todo note as we could just create an issue for it if we want to rename drupal_get_library.

Could we get a final review for this? It's RTBC in my eyes.

tic2000’s picture

My review is not final, but still:

1. Droppable requires Draggable.
2. If "add callback" is discussed in another issue, the mention about it should be added in that issue and removed from hook_library documentation in this one.

RobLoach’s picture

FileSize
26.88 KB
  • Makes "ui.droppable" depend on "ui.draggable" (good find).
  • Removes the reference of "add callback" from the "js" settings in hook_library() (another good find)
mfer’s picture

I just realized the drupal_get_library now cherry picks the library hook. So, if I call it for system it loads if for system only and then does an alter on that. If I load it for contrib1 it loads just that and does alter on just that. What are the implications of that? Is there any way we'd want to use the alter where we need the full registry?

RobLoach’s picture

Status: Needs review » Needs work

Sun removed the ability to retrieve the full registry in #251 when he removed the caching. Back in #113, I was attempting to write up a hook_requirements() that would check the registry for missing files, and discovered that we needed a way of getting the full registry. Retrieving the full registry is an absolute requirement of this, is there a reason why you removed it?

tic2000’s picture

Is moshe's point in #234 answered by this patch or is this something to do in a follow-up issue?

sun’s picture

Status: Needs work » Needs review

@Rob Loach: As mentioned before, we can discuss to re-introduce a dedicated (db) cache for this "registry" in a separate issue. As of now, this patch is no longer a real registry - rather a layer of abstraction using a module callback to load a set of JS/CSS files and dependent sets of JS/CSS files (if required) to ease the DX for people who want to integrate with such libraries. A db request is a db request, and considering that many pages might only need to add jquery.js, but no library, we add a lot of overhead. Furthermore, I don't see a need for requirements -- these are files that are shipped with Drupal or a module. I don't see the point in displaying requirements for all libraries. We may display the version of jQuery and jQuery UI, but these two don't require a full registry. But anyway, we can (and I'm open to) discuss this in more detail in a separate issue.

@tic2000: That will be dealt with in a follow-up issue: #505084: Add #attached_library FAPI property for drupal_add_library()

@kkaefer: I'm no longer without bias regarding this issue. The patch had a turnaround and I believe the new implementation makes sense. It does not solve and does not try to solve any major issues like version conflicts and related nightmares. It is a pure starting point to simplify loading of JS/CSS libraries for developers. Contrib might be able to advance on it, while it allows all of us to simply ask for System module's jQuery UI Tabs (or whatever) library or Foo module's Bar library to add that to a page, without having to care about the actual filepath or any other possible dependencies a certain library might need. It is certainly a starting point only, a plain wrapper around multiple drupal_add_*() calls. Since you had objections to the former implementation (just like me), I'd like to ask you for feedback and potentially also RTBC status.

RobLoach’s picture

Status: Needs work » Needs review

That sounds reasonable to me. Once this one goes in, we'll be able to have a post issue to cache it and manage the registry like an actual registry. This would mean the alter() could alter the entire registry, drupal_get_library() would retrieve all the libraries.

In summary, this patch gives the ability to add multiple JavaScript and CSS files at a time. This is particularly important now that jQuery UI is part of core. We need a way to manage it, as well as allow upgrading it through contributed modules later on. It does this by introducing two new functions and two new hooks:

hook_library()
Register JavaScript/CSS libraries from modules
hook_library_alter(&$library, $module, $name)
Gives the ability to alter a JavaScript/CSS library before it is added to the page. $module is the name of the module that hosts it, $name is the name of the library.
drupal_add_library($module, $name)
Adds a given library's JavaScript and CSS to the page. $module is the name of the module that hosts the library, and $name is the library name.
drupal_get_library($module, $name)
Retrieves information about the given library, from its $module host and the library $name.

Future issues to this will include:

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

FileSize
26.47 KB

See #260 for details about this patch.

Damien Tournoud’s picture

Status: Needs review » Needs work
+  if (!$library) {
+    watchdog('system', 'Missing JavaScript/CSS library %library (%module module).', array('%library' => $name, '%module' => $module), WATCHDOG_WARNING);
+    return FALSE;
+  }

^ We really don't want to clutter the log with this.

+    // Add dependent libraries (first), JavaScript, and stylesheets.
+    foreach (array('dependencies', 'js', 'css') as $type) {

^ I would separate 'dependencies' from 'js' and 'css', because this is a special case.

+          if (!is_array($options)) {
+            continue;
+          }

^ There is a lot of unneeded type checking like this in this patch. That sort of type checking only hides weird errors under the carpet. This is a bad idea, and those should be removed.

+            // All JavaScript contained within the libraries are given a default
+            // weight of JS_LIBRARY.
+            elseif (!isset($options['weight'])) {
+              $options['weight'] = JS_LIBRARY;
+            }

It's fun that this also applies to CSS files ;)

+function drupal_add_library($module, $name) {

Why $name *and* $module? This makes most of the processing more complex, and doesn't seem to have any added value.

kkaefer suggested earlier that we implement a hook_library_<library>() hook, essentially querying for all modules that provide a version of a given library. We could then take the answer of the module that have the most weight or the higher version available (or a mix of the two). I think this would make more sense then the current flat hook_library().

Moved to CNW, at least for cleaning up the code.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
26.38 KB

Rerolled with a bunch of changes, clean up and suggestions from DamZ and tha_sun.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
29.49 KB

Had to change hook_library_alter($libraries) to hook_library_alter($module_libraries, $module); as we're not caching it anymore.

RobLoach’s picture

FileSize
28.52 KB

Uhh, whoops.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.25 KB

Caching is still deferred to another issue.

Based on #266, slightly reworked comments & stuff.

I'm feeling bold.

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
26.44 KB

webchick was on me recently about how the front end developers were unfamiliar with #attached_js and $attached_js and that they are poorly documented. It turns out that these properties were added in the high profile (and very long) vertical tabs issue. the only way i know to start familiarizing developers is to offer code and explanation within issues that they care about. so, here is an updated patch which uses these new properties. please review it and hopefully we can get back to RTBC very quickly. If my code starts delaying this issue badly, then lets toss my code and proceed with the prior patch. Let's at least try to support all of HEAD's features ...

#attached_js and #attached_css are properties which you may add to any renderable element like node content or a block. when you do that, drupal_render() passes the contents of the property and its options to drupal_add_css or drupal_add_js. The benefit to using the properties versus calling drupal_add_js/css directly is improved caching. At #495968: Introduce drupal_render() cache pattern. Start using it for blocks, we are working on a cache api for any drupal_render() element. That's to say, any part of the page can be cached, even for authenticated users. It is a lofty goal, but doable IMO. One key to making that work is that render elements should have no "side effects" because those effects are not cached. You might recall that standard blocks lost the ability in D6 to add css or js because they are cached and thus not executed on every request. Thats an example of a bad side effect. These properties give us the ability to cleanly avoid the side effect. Hope this all makes sense.

The only substantive change I had to make is to ask add a param to drupal_add_library() where an $element can be passed by reference. We then append #attached properties to that element as needed ... The only slightly awkward bit in the patch is my drupal_render($element) trick when adding jquery.js and in the tests. Thats easily overcome by refactoring drupal_render() a little. I will do that after I get a little feedback.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Moshe and I discussed splitting off his concern in #270. Moshe would like to include the ability to support #attached_js and #attached_css in this issue. RobLoach made a separate issue for it at #505084: Add #attached_library FAPI property for drupal_add_library().

This is a good point to stop and ask the core maintainers, should we continue this thread and get it working with #attached_js/css or commit the patch as-is and finish in the follow-up? We've reached some consensus that the existing patch in #269 accomplishes a strong starting point.

Rob Loach:

In summary, this patch gives the ability to add multiple JavaScript and CSS files at a time.

sun:

It is a pure starting point to simplify loading of JS/CSS libraries for developers.

We're at a point where we could go either way. Split #attached_library into a separate issue or carry on here?

RobLoach’s picture

Status: Needs work » Needs review
FileSize
27.25 KB

If front end developers don't know about #attached_js or #attached_css, then they should read the update logs: Attached JS and CSS for form elements ;-) . Actually, that's a good point, we should document #attached_js and #attached_css in the Converting themes from 6 to 7 too so that they see it..... Added, if you guys have tweaks for it, please make them. I pretty much just copied and pasted. Would be good to have it for a generic drupal_render() $element instead of the Form API though.

Having drupal_add_library() require the use of an $element seems reasonable, but what we're trying to do here is mimic how drupal_add_js() and drupal_add_css() work, but for multiple files. Afterwards, we could then have #attached_library for the Form API. Instead of calling drupal_add_library() on an element, we would use:

  $element['#attached_library'] = array(
    array('system', 'ui.dialog'),
  );

This #attached_library property also has the ability to be cachable, unlike having a call to:

  drupal_add_library('system', 'ui.dialog', $element);

.....Which would just add all the CSS and JS to the $element array itself instead of having to cache just the library name. In drupal_add_library(), I think we should just add the CSS/JS to the page, like how drupal_add_js/css operate, and then make it interact with the Form API through both #454382: Provide jquery_ui_elements() for easier usage of plugins in other modules and #505084: Add #attached_library FAPI property for drupal_add_library(). This issue has had enough kittens in it that were already pushed to future issues (caching, add callbacks, etc).

Also, having the following to add jQuery just looks kind of silly:

  $element = array();
  drupal_add_library('system', 'jquery', $element);
  drupal_render($element);

If you REALLY wanted to use an element around drupal_add_library(), it would eventually turn into:

$element = array(
  '#attached_library' => array(
    array('system', 'jquery'),
  ),
);
drupal_render($element);

Don't worry, Moshe, getting this to work with drupal_render is on our minds, and it will be addressed, but just not quite yet ;-) .

tic2000’s picture

To add to what Rob Loach said I think having #attached_library is consistent with attaching single css and js, DX wise.

moshe weitzman’s picture

Yeah, #attached_library is better than what my patch proposes. I agree with Nate that we need a committer decision about whether it is acceptable to defer this aspect of the patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Guys. #505084: Add #attached_library FAPI property for drupal_add_library() was created long ago, is already marked critical, and depends on the base API functions this patch introduces.

This issue suffered from maximum scope creep since it was started -- i.e. the first ~200 follow-ups.

The only reason to hold up this patch longer could be that it might still not be clear to everyone why the implementation was done this way. PHPDoc of drupal_get_library() explains it, but it is possible that the explanation is not yet sufficient. We want to document the knowledge and reasoning from this issue to prevent any future issue that tries to alter the code without reading through 275+ follow-ups on this issue.

So. If you read the patch and you do not (completely) understand why it works that way, then please mark this as needs work and ask your questions. You should NOT read the entire issue and search for answers.

webchick’s picture

Sorry, Moshe. I agree with sun. Getting this functionality compatible with renderable output is extremely important, but the focus of this is only the registry itself. Let's reconvene in #505084: Add #attached_library FAPI property for drupal_add_library() when this issue gets marked fixed.

tic2000’s picture

No need to feel sorry since he agrees so I say RTBC too.

redndahead’s picture

@webchick: This is RTBC is there something else you are waiting for before commit?

webchick’s picture

Yes. Time to review. :) Should be able to do so today.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Checked through this patch once more. Since sun worked on it, it's very boring to review because it has no coding standards issues. ;)

It looks like we have broad acceptance that this is the way forward, and a good idea of what to enhance in follow-up patches. Very impressive work, folks. Thanks for all of your diligence.

With that, I've committed this to HEAD! Marking "needs work" for docs.

webchick’s picture

Issue tags: +Needs documentation

Tagging.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +drupal_add_library

Ability to add multiple JavaScript/CSS files at once
With drupal_add_library() you can add related sets JavaScript/CSS at once. The first argument is the name of the module that registers the library, while the second argument is the name of the library you want to add.

Drupal 6:

  // Add Farbtastic color picker.
  drupal_add_js('misc/farbtastic/farbtastic.js');
  drupal_add_css('misc/farbtastic/farbtastic.css');

Drupal 7:

  // Add Farbtastic color picker.
  drupal_add_library('system', 'farbtastic');

These sets of JavaScript/CSS libraries must be registered through hook_library():

  function system_library() {
    $libraries['farbtastic'] = array(
      'title' => 'Farbtastic',
      'website' => 'http://code.google.com/p/farbtastic/',
      'version' => '1.2',
      'js' => array(
        'misc/farbtastic/farbtastic.js' => array(),
      ),
      'css' => array(
        'misc/farbtastic/farbtastic.css' => array('preprocess' => FALSE),
      ),
    );
    return $libraries;
  }

.............. You guys must be better writers then me. As for follow up issues:

jpetso’s picture

Not sure about the format of "dependencies", see this piece of code that was committed to system.api.php:

'dependencies' => array(
  // Require jQuery UI core by System module.
  array('system' => 'ui'),
  // Require our other library.
  array('my_module', 'library-1'),
  // Require another library.
  array('other_module', 'library-3'),
),

Note the associative arrow "=>" in the "system" example and the "," in the other ones. The documentation also hasn't got the clearest of all wordings on this matter:

/**
 * - 'dependencies': An array of libraries that are required for a library. Each
 *   element is an array containing the module and name of the registered
 *   library. Note that all dependencies for each dependent library will be
 *   added when this library is added.
 */

I think this error in the example might highlight a source of potential errors, other developers could easily make the same mistake. Maybe a format like the following one would be more appropriate?

'dependencies' => array(
  // Require jQuery UI core by System module.
  'system' => array('ui'),
  // Require our other library.
  'my_module' => array('library-1'),
  // Require another library.
  'other_module' => array('library-3'),
),
sun’s picture

Issue tags: +Libraries
FileSize
1.41 KB

'dependencies' should be a list and no associative array.

Your proposal would work as well with what we have currently (since multiple libraries from the same module might be dependencies). However, it is not yet sure whether #507342: Make jQuery calls without needing addional jQuery files will re-introduce further arguments to drupal_add_library(), which may be passed to 'add callbacks'.

First of all, let's fix the obvious bug.

RobLoach’s picture

jpetso is right here, and chx brought up the problem to me yesterday. It would be rather hard to alter this list of dependencies, when compared to an associative array. Check this out:

   $libraries['farbtastic'] = array(
     'dependencies' => array(
       // Require jQuery UI core by System module.
      array('system', 'ui'),
       // Require our other library.
       array('my_module', 'library-1'),
       // ...

function myothermodule_library_alter(&$libraries) {
  // Remove the Farbtastic dependency on library-1.
  foreach ($libraries['system']['farbtastic']['dependencies'] as $index => $dependency) {
    if ($dependency[0] == 'my_module' && $depedendency[1] == 'library-1') {
      unset($libraries['system']['farbtastic']['dependencies'][$index]);
    }
  }
}

This would be the same with an associative array with what jpetso suggested:

'dependencies' => array(
  // Require jQuery UI core by System module.
  'system' => array('ui'),
  // Require our other library.
  'my_module' => array('library-1'),
  // Require another library.
  'other_module' => array('library-3'),
),

function myothermodule_library_alter(&$libraries) {
  foreach ($libraries['system']['farbtastic']['dependencies']['my_module'] as $index => $dependency) {
    if ($dependency == 'library-1') {
      unset($libraries['system']['farbtastic']['dependencies']['my_module'][$index]);
    }
  }
}

I think there still is a better solution out there, we just haven't come to it yet. Any ideas?

Damien Tournoud’s picture

The natural primary key in that case is ($module, $name). Why not using $module . '-' . $name as the key?

Gábor Hojtsy’s picture

I'd kindly request that once this is included in the module update guide, please also update the jQuery UI entry at http://drupal.org/update/modules/6/7#jquery_ui Thank you!

RobLoach’s picture

I created #511532: Incorrect documentation for hook_library() as to not hijack this thread from the need of documentation.....

Ability to add multiple JavaScript/CSS files at once
You can add related sets of JavaScript and CSS at once now with the drupal_add_library() function. The first argument is the name of the module that registers the library (via hook_library), while the second argument is the name of the library you want to add.

Drupal 6:

  // Add Farbtastic color picker.
  drupal_add_js('misc/farbtastic/farbtastic.js');
  drupal_add_css('misc/farbtastic/farbtastic.css');

Drupal 7:

  // Add Farbtastic color picker.
  drupal_add_library('system', 'farbtastic');

These sets of JavaScript/CSS libraries must be registered through hook_library:

  function system_library() {
    $libraries['farbtastic'] = array(
      'title' => 'Farbtastic',
      'website' => 'http://code.google.com/p/farbtastic/',
      'version' => '1.2',
      'js' => array(
        'misc/farbtastic/farbtastic.js' => array(),
      ),
      'css' => array(
        'misc/farbtastic/farbtastic.css' => array('preprocess' => FALSE),
      ),
    );
    return $libraries;
  }

The library registry can also be altered through hook_library_alter. This becomes useful if you are updating the library to a newer version.

Gábor Hojtsy’s picture

Rob Loach: IMHO feel free to add this to the upgrade docs.

RobLoach’s picture

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

Status: Fixed » Closed (fixed)

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

cburschka’s picture

Status: Closed (fixed) » Needs review

This shouldn't have been closed without a commit...

mfer’s picture

Status: Needs review » Closed (fixed)

This is closed. It was committed in #281 and left open until there were upgrade docs.