Add a hook_libraries_info()

This is required to do

# Preventing incompatibilities due to having the same library installed more than once in different versions.

(from the project page), right?
I came up with a little patch that checks for versions.
It is an all-in approach, so that it is not possible to place libraries in a directory that is not nested with their version.
I.E. only sites/all/libraries/[library_name]/[version_number] is possible and not sites/all/libraries/[library_name]
I think that is required though, to properly handle versions, otherwise some libraries would have versions attached to them, and some wouldn't.

No UI as requested in the issue description.

P.S.: I moved the directory searching into a helper function _libraries_searchdir() simply for code readability. The nesting of if's and foreach's is deep enough that way already and would have been much worse with all the while's and additional if's. That's of course only a cosmetic decision and can be reverted if needed.

I just realized libraries_get_path needs a little rework as well. Will post updated patch soon.

Here we go. Back to needs review.

oh wow! Thanks for trying to push this forward!

However, I'm not really sure whether we want to take this route. The only way to prevent conflicts caused by multiple versions of the same library is to have only one version of the library installed.

When integrating external libraries, a Drupal module has to implement an observer pattern, because it does not know whether the proper library, and, whether a supported version of the library has been provided. Depending on the library version that actually has been provided, different features may be available.

  library -> module

Since modules need to do this anyway, I don't really see the benefit of supporting multiple versions of the same library. Contrary to a potential benefit, I rather see that it will be prone to errors, because you absolutely have to make sure that all modules requiring the library (in different versions) are NOT trying to load it on the same page or request. In essence, you wouldn't need Libraries API for that, because your goal is not to provide a common denonimator, and modules just try to take back authority.

  page1 -> module1 -> library-v1
  page2 -> module2 -> library-v2
  page3 -> module1 + module2 -> ?

In short: I'm not convinced that supporting multiple versions of the same library makes sense. Instead, I'm rather convinced that every module integrating with a certain library will be able to support multiple library versions and dynamically adjust itself for the one being available. Updating a module to account for changes in a new library version is not hard after all. The Wysiwyg module is a nice show-case and prototype for this behavior. Ideally, I'd rather like to advance on this observer behavior, making it easier for all modules to do what Wysiwyg module started to do. (but that's a different issue)

  module <- library -> module

Lastly, the suggested directory structure here slightly clashes with the solution we still need to find for #465908: Scan: Name clashes

            library -> module
  module <- library

Thoughts?

All right. I get everything up to the WYSIWYG reference. I will browse some WYSWIWYG code to see if I can get what you mean. Hopefully, then
module <- library -> module will also make more sense to me =)
Also

            library -> module
  module <- library

? Is that related to #465908: Scan: Name clashes? I thought I got that issue (I commented over there), but I don't get that.

Basically what this means, that there has to be some way for Libraries API (and via that any module) to retrieve the version information from a library. So I guess there has to be some kind of hook for which would Libraries API would give some defaults (jQuery, jQuery UI, etc.) and modules could implement their own (ideally with Libraries API integrating more and more along the way).

So that would be the forward, right?
(Back to active, the patch doesn't really count anymore :'( .... :)

Yeah, I guess WYSIWYG's hook_plugin (or hook_editor?, they seem to be rather similar) is pretty much what we want, if I understood that correctly.

Thinking about a name, I just looked at Core's new hook_library and drupal_get_library. Now you said somewhere with very much emphasis (:)) that that is not what you want. The API page for drupal_get_library mentions the following:

Library information is statically cached. Libraries are keyed by module for several reasons:

• Libraries are not unique. Multiple modules might ship with the same library in a different version or variant. This registry cannot (and does not attempt to) prevent library conflicts.
• Modules implementing and thereby depending on a library that is registered by another module can only rely on that module's library.
• Two (or more) modules can still register the same library and use it without conflicts in case the libraries are loaded on certain pages only.

Now if I have understood correctly, that is EXACTLY the limitation that you want to overcome with Libraries API. If not, then I just don't get what the difference is. In any case, a name for the hook would be a great start =)

Right, it's mainly hook_editor() in Wysiwyg that's aware of multiple library versions and implements library version detection. I think we can forget about hook_plugin() for now.

If you haven't read the following two issues already, then it would be good to do it, so we have a common base of knowledge:

#320562: Libraries API (the initial idea + discussion that lead to Libraries API)
#315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) (the Drupal core hook_library() feature request, which contains plenty of background info for why we need Libraries API)

Note that the latest patch in the first issue for Wysiwyg was not committed yet. Wysiwyg is already using sites/all/libraries, so those changes would basically add a module dependency only, thus deferred until work on Wysiwyg 3.x actually starts.

As of now, Libraries API does not do much on its own. It's merely a helper function to retrieve directories from sites/all/libraries, so other modules can depend on it.

The rough and to be discussed plan to move forward looks like this:

1. Add hook_libraries(): A mixture of hook_library() in D7 and hook_editor() in Wysiwyg. More like the latter, because Wysiwyg's library detection and version handling is required. Note that I'm also open to discuss OOP possibilities here, if we would benefit from OOP.
2. Consider to abstract the 'version callback' of various hook_editor() implementations in Wysiwyg into a general-purpose helper function, i.e. something along the lines of libraries_get_version($library, $file, $lines, $pattern), whereas those arguments would be specified via 'version arguments' in hook_library(). This will most likely be suitable and sufficient for >90% of all libraries.
3. Move and abstract Wysiwyg's library list + installation instructions into Libraries API, so that any module can embed installation instructions for its supported libraries by calling libraries_install_instructions(array $libraries), and so we can improve usability once for all modules. #487690: Improve installation instructions contains some screenshots, ideas, and discussions around this.
4. Solve the two major design problems of Libraries API: #465908: Scan: Name clashes and #466090: Scan: Cache results
5. Consider whether it would make sense to have Libraries API implement hook_libraries() on its own. When taking the entire Libraries API idea into an extreme, then situations like the following can occur: The CKEditor library is integrated into Drupal by more than one module: Wysiwyg module and CKEditor module. Both modules would have to register the same library information on their own, including version detection. Now let's replace the CKEditor library with a more common library, perhaps Lightbox or some funky jQuery plugin. Each module that wants to use that library has to register the library on its own, unless the module depends on another module that registers the library already. Therefore, it may be worth to consider whether Libraries API should take over registration of "popular" libraries, i.e. which are used by more than one or two modules.

I hope this list contains everything, but I'm not sure. Until now, further progress on Libraries API was mainly blocked by not having people to discuss its design and architecture - as you can see, the todo list is pretty long and every single topic is very complex. It would be wrong to just start to do something in the hope that it "may work" ;)

Thoughts?

how about when there's a new version out and some of the modules are updated to use it while others not yet. People need to test and report issues to the modules that don't support it yet so that they eventually will.

This is the situation we are actually facing in Wysiwyg module already. And there's no problem with having only one library version in place + multiple being supported. If you install a library version that's not supported by all modules that want to integrate with it, then you need to step back to a version that works with all modules.

For example: #695398: Support for TinyMCE 3.3

The remaining points of your follow-up are based on the paradigm/thinking that any module controls the version of a library that will be loaded...

Now multiply that thinking with another dimension: multiple modules.

And now, yet add another dimension: themes.

Which one wins? Which module's functionality will be broken?

That's why we can only support only a single instance of a library. We need to understand that libraries are third-party software, which we want to depend on.

This third-party software is made available by the site administrator, has different release cycles, and eventually even changing APIs in minor versions. All of these are (external) factors that can break your site, or the functionality of a particular module that wants ot use the library.

Therefore, all modules that want to integrate with a library need to act like consumers, implementing an observer pattern. Ultimately, Libraries API's goal is to take away the logic to discover and identify a library. But much like Wysiwyg's hook_editor(), consuming modules need to implement proper support for varying, evolving libraries, with the paradigm in mind that anything that can change, will change.

I believe a lot of people (like myself) are found in a situation when they try different versions of libraries and if it so happens that something in their site breaks and they discover that a few days later

I think that is something, that we will eventually be able to prevent with Libraries API. Each module requiring an external Library will specify the version it requires, so if a different version than the required one is found it would make sense, for instance, for Libraries API to show a warning on the Status report page. Libraries API will not (!) help with actually having two different versions in parallel, but it can help you if you are developing with a module that depends on an outdated version.

That's at least what I imagine Libraries API would do in my perfect world. Don't know if that is on sun's list as well.

Read the comments sun linked to above, why supporting different versions in parallel is not something we want to support. As you can see in my patch at the top this issue and suns comment, this a choice, not a technical limitation. Just, so you don't get your hopes up =)

This is a draft of a hook_libraries which basically covers 1. and 2. from #8
If you give me green light, I'll work on implementing such a hook.

/*
 * Declare information about required external libraries
 *
 * @return
 *   An associative array containing library information. The key is the name of the
 *   library, which  must be the name of the directory the library should be
 *   contained in within a 'libraries' directory. The values are themselves
 *   associative arrays containing following key-value pairs:
 *     'title' => The human-readable name of the block.
 *     'vendor_url' => The homepage of the library.
 *     'download_url' => The website where the current version of the library can be obtained.
 *     'version_callback' => A function that returns the full version string of the library.
 *       Defaults to 'libraries_get_version', a helper function which takes two arguments:
 *         $file The file to use for the version check.
 *         $line_number The line number in the specified file which contains the version information.
 *     'version_arguments' => Arguments to pass to the version callback. Can usually
 *       be omitted when using a custom version callback.
 *     'files' => (Optional) The files to include as part of the library. Can be PHP,
 *       JavaScript or CSS files. If ommitted, all files with the file endings .php,
 *       .js and .css in the library's directory will be included.
 *     'dependencies' => The library's dependencies. A string for a single dependency or an array if the library has multiple dependencies.
 */
function hook_libraries_info() {
  $libraries['cool_lib'] = array( // This makes Libraries API search the 'sites/all/libraries/cool_lib' directory (among others).
    'title' => 'Cool library',
    'vendor_url' => 'http://cool_lib.com',
    'download_url' => 'http://cool_lib.com/download'
    'version_arguments' => array('CHANGELOG.txt', '5'),
    'files' => array('cool_lib.php', 'cool_lib.js', 'cool_lib.php') 
    'dependencies' => 'jqueryui'
  );

  return $libraries;
}

doh! Dreditor needs a .patch file ;)

What a chance to finally figure out that fakeadd thingy...

Just a note, that I will probably start coding this this week. (Also while re-titling I don't know, why I called the function hook_libraries_info()...)
So if it's totally off, now is the time to yell at me.

That was actually correct :)

Note that we can probably copy the code from Wysiwyg API - actually I thought there even was a patch already... (?) - perhaps in the "Libraries API" issue, which I already linked above

Essentially, that code is a fork of Panels' plugin system, which has been abstracted and generalized into CTools in the meantime. We can surely go ahead with the current code from Wysiwyg HEAD, but ultimately, this will most probably mark the start of adding a dependency on CTools to not duplicate this effort. We can happily defer that to a follow-up issue though (and likely better than trying to revamp all of this in one shot).

Yes, I had taken a big chunk from WYSIWYG API for the hook description in #16/#18.
Just to summarize (if only for my own reference) #16/#18 vs. hook_editor in WYSIWYG API vs. hook_library in core. (CTools is not in there because I didn't look at it originally. I just looked at it briefly and it seems to me, that utilizing CTools Plugin system is a non-trivial decision, so I am more than happy to defer that to a separate issue.)

• 'versions' (hook_editor) / 'version' (hook_library): There is no version parameter in #16/#18. That's because I wanted to make the declaring of the library information independent from the actual version that is required by the module. That way it is totally irrelevant whether the library information is declared by an external module or by Libraries API itself. I also find that semantically cleaner, since we are already calling it hook_libraries_info (hehe...) and a specific version is not info about the library itself. That would also allow to declare the required versions somewhere else, for instance in the module's info file, á la
  libraries[] = mootools (4.7.2)
• 'libraries' (hook_editor) / 'js' and 'css' (hook_library): hook_editor has the ability to load multiple libraries inside one library editor. That way it can give specific files 'title's and other meta-data. We don't need that (I think/hope!), so I omitted that and just went with a 'files' parameter to declare all relevant files. hook_library divides that into 'js' and 'css', but I think it would be just as easy to simply use an associative array with the keys being the filenames and the values being the types ('js', 'css', 'php') (NOTE: I just thought of that now, in #16/#18 'files' is a non-associative array, which is inflexible because we would have to hard-code 'file-ending' -> 'file-type' associations).

Furthermore, in comparison to hook_editor, #16/#18 has

• 'dependencies': Taken from hook_library. Needed for e.g. jQuery libraries.
• 'version arguments': Needed for the easy use of (the yet to code) libraries_get_version() for people who simply want to specify a file and a line number to look for the version.

while it omits

• 'themes callback'
• 'settings callback'
• 'plugin callback'
• 'plugin settings callback'
• 'proxy plugin'
• 'proxy plugin settings callback'

because again I think/hope(!) that this is not needed.
Furthermore, in comparison to hook_library, #16/#18 has

• 'download_url': Taken from hook_editor. Very minor, but I think it makes sense. :)

One last note: I just realized that a bunch of parameters in #16/#18 have underscores where it is probably preferable to use simple spaces.

Ugh, no - caution: we've lost one dimension here. :)

But I can understand that it's not very easy to grok the big picture. Let me try to clarify:

- based on the example of tinymce.inc -

1. 'versions': This is required, and actually the essential core for handling third-party libraries. If you look at wysiwyg_get_all_editors(), the actual function that parses library information into the registry, then you'll notice that - depending on the identified version - the data in 'versions' overrides (replaces) the static/global library information. This is required to account for API changes in library versions.
2. 'libraries': This is sort of required, as various libraries can appear in multiple variants. However, in general, the idea is to support arbitrary properties in the registered library information, so whatever makes sense for editor libraries, it can be added (which also relates to those other '* callback' properties you mentioned). I'd consider proper handling of library variants as a core feature of handling of libraries, so the 'libraries' property (perhaps renamed to 'variants' or similar) rather belongs to (possibly required) standard properties of Libraries API.
3. Sub-keys 'js', 'css', 'php' on 'libraries': Ideally, we should follow hook_library() here, as that will potentially allow us to re-use existing core functions (at least in D7).
4. 'dependencies': This, we need to defer to a separate issue and just skip it for now. Too complex.
5. Sub-keys in 'libraries' and on the top-level: As mentioned above, every registered library has different requirements. For example, the 'proxy plugin' key for editor libraries, which is specific to Wysiwyg API, needs to be able to be registered. Technically, either Wysiwyg module should register the library with those additional properties in the first place, or it would have to alter in those properties for each editor.

   This directly leads to the next topic: The properties 'js files' and 'css files' defined in 'versions' are also specific to Wysiwyg API, and technically, those only apply to Wysiwyg API. Those files are the actual integration/support files for the library, which Wysiwyg API loads when the library is loaded.

   Most modules need to load integration code for the library that is being loaded. So this information should be registered with the library. Compared to 'libraries' ('variants'), the difference is that we talk about own files here, not third-party files. Ideally, this should be standardized as well, so a 'integration' (or similar) property, keyed by module, defines JS/CSS/PHP files to be loaded when the library is loaded.

   Alternatively, or additionally, those integration files could be added by invoking registered callback functions. But I'd rather prefer to keep the "integration files" information in the registry.

   In a follow-up issue, we'd have to think about ways to invoke/load a library - passing the module name, so the integration files would be automatically loaded (or not).

6. In general, hook_library() is irrelevant for us - at least for now. We can certainly compare our patterns with the patterns of hook_library() to make them as similar as possible. But effectively, we need to account for much more: 4 dimensions instead of 2.
7. 'download url': Note that words should be separated with spaces, not underscores. (you already figured that)

Effectively, something along the lines of:

function hook_libraries_info() {
  $libraries['example'] = array(
    'title' => 'Official library name',
    'vendor url' => 'http://example.com',
    'download url' => 'http://example.com/download.php',
    // The actual path to the library should be figured out automatically by Libraries API.
    'path' => 'jscripts/tiny_mce',
    // Optional. Defaults to libraries_get_version().
    'version callback' => 'mymodule_example_version',
    'version arguments' => array(
      'file' => 'example.js',
      'lines' => 40,
    ),

    // Global default properties applying to all versions.
    'variants' => array(
      '' => array(
        'title' => 'Minified',
	      'files' => array(
	        'js' => array('example_min.js' => array()),
	        'css' => array('example.css' => array()),
	      ),
      ),
      'src' => array(
        'title' => 'Source',
	      'files' => array(
	        'js' => array('example.js' => array()),
	        'css' => array('example.css' => array()),
	      ),
      ),
    ),

    // Version-specific overloading.
    'versions' => array(
      '2.1' => array(
        'download url' => 'http://sourceforge.net/project/showfiles.php?group_id=103281&package_id=111430&release_id=557383',
	      'integration' => array(
		      'wysiwyg' => array(
  		      'files' => array(
  		        'js' => array('wysiwyg-example-2.js' => array()),
  		        'css' => array('wysiwyg-example-2.css' => array()),
  		      ),
		      ),
	      ),
      ),
      '3.1' => array(
	      'integration' => array(
		      'wysiwyg' => array(
  		      'files' => array(
  		        'js' => array('wysiwyg-example-3.js'),
  		        'css' => array('wysiwyg-example-3.css'),
  		      ),
		      ),
	      ),
        'variants' => array(
          '' => array(
            'title' => 'Minified',
  		      'files' => array(
    	        'js' => array('example_min.js' => array('preprocess' => FALSE)),
  		        'css' => array('example.css' => array()),
  		      ),
          ),
          'jquery' => array(
            'title' => 'jQuery',
  		      'files' => array(
    	        'js' => array('jquery.example.js' => array('preprocess' => FALSE)),
  		        'css' => array('example.css' => array()),
  		      ),
          ),
          'src' => array(
            'title' => 'Source',
  		      'files' => array(
    	        'js' => array('example.js' => array('preprocess' => FALSE)),
  		        'css' => array('example.css' => array()),
  		      ),
          ),
        ),
      ),
    ),

    // Non-standard properties; either existent when a module registers a
    // library, or when a module alters registered libraries.
    // @todo Should probably live in a sub-key by module.
    'themes callback' => 'wysiwyg_tinymce_themes',
    'settings callback' => 'wysiwyg_tinymce_settings',
    'plugin callback' => 'wysiwyg_tinymce_plugins',
    'plugin settings callback' => 'wysiwyg_tinymce_plugin_settings',
    'proxy plugin' => array(
      'drupal' => array(
        'load' => TRUE,
        'proxy' => TRUE,
      ),
    ),
    'proxy plugin settings callback' => 'wysiwyg_tinymce_proxy_plugin_settings',
  );
  return $libraries;
}

Note: You can also find me on Skype.

Thanks for taking the time to write up that awesome explanation!
After now re-reading for the 4th time and looking at wysiwyg_get_all_editors() I think I get the point.
I basically agree with everything you said in general, just some comments, which aren't really dealbreakers and which don't hold me up from actually trying to implement that in the coming days.

• What about dumping all non-standard properties into a 'properties' key? Just a cosmetic point but it would keep the top level of the array consistent.
• I'm still pondering about (but haven't found a real solution yet) whether it would be possible to collapse the 'versions' key into the 'variants' key. Basically both are about over-loading the basic files depending on certain conditions (version for 'versions' (duh!) and something I haven't figured out yet for 'variants'). Again, I would find that cleaner, but it's not a priority (and I'm not yet sure, whether that would be possible).
• I would find a top level key 'files' a better DX than stuffed in the various variants. Then the 'variant' key and 'versions' key would be optional and would simply overload the base 'file' key. So for simple libraries, the module developers don't have to think about different variants, they just tell Libraries what files it should load and be done with it. Again (...), a mere cosmetic issue.

Concerning Skype: I don't have a headphone at the moment, I could jump on to IRC if that's requested, though. Personally, I don't really dislike this issue queue discussion, because it gives me time to figure stuff out on my own, before asking again. I could understand, though, (and that's meant in all sincerety) if you're tired of giving me a (free!) intro course on the inner-workings of WYSIWYG soon to become Libraries. So, in short: Whatever you say :)

Good thinking.

1) For now, we should keep the "possible" top-level properties as flexible as possible. If modules need to register more information for libraries (like Wysiwyg), then that information is very likely required information. Yes, property clashes are possible this way, when considering all 4 dimensions. But yeah, as we don't have real world data for a fully fledged multi-dimensional system yet, we can let this evolve and keep things easy (i.e. allow registering non-standard top-level properties).

2) Basically you're right with the "conditions" consideration. If there are variants, then the variant to load will most likely (but not necessarily) depend on external factors. In Wysiwyg's example, the editor library to load could depend on global settings defined system-wide elsewhere: a) JS aggregation b) JS minifying etc. So Wysiwyg would/could auto-load the appropriate variant depending on those conditions. However, that's pretty much up to the integrating module and likely vastly differs from library to library. Truth is that I didn't come around to implement support to configure the variant to load in Wysiwyg yet, and while that heavily hinders debugging, it's not that easy to support in Wysiwyg itself (as additional [own] integration files may need to be loaded or other conditions need to be triggered).

3) A top-level 'files' defining the default files to load, next to an optional 'variants' defining different sets of files sounds awesome! :)

re: Skype - I can't do calls myself either, only chat. IRC distracts too much from my day to day work, so I try to limit that to weekends ;) I offered it, so you don't have to wreck your desk by bashing your head on it, and may get quicker answers for questions around the weird nightmares of libraries. ;) I really appreciate that you want to work on this!

Here's a first step.

Various notes:

• This is pretty much untested. I just created a test module with the example data from the API file to see if that works properly. I didn't test all the version mungling, which is basically the most interesting part.
• I renamed the 'integration' key (your suggestion) to 'integration files'. I don't feel strongly about that, I just thought it was more self-explanatory.
• From libraries.api.php:
  

  The versions array is keyed by the version strings or numbers that need to be differentiated. The values are in turn arrays which contain a replica of the top-level array containing version-specific information they need to override.

  The example implementation begs an obvious question: If I declare a 'files' array in 'versions' do the actual files get added to the list in the top-level 'files' array or do they get replaced.? I'm thinking the former (in which case the documentation and code would have to updated) but I'm not sure.

• While coding this, I thought that maybe we should split the function into two. One which only gets the infos from the modules and doesn't care about the rest and one which does the version checking and overloading. Because we already have libraries_get_libraries(), which contains the info about existing modules. Then we could somehow merge that and not do a file_exists(libraries_get_path()) inside the foreach. I guess, just a matter of style.
• Variants handling. Probably has to be discussed when there is a function to actually load a library, to pass that as a parameter and then do the override there. Don't know.

Not implemented yet (partly already discussed, partly not):

• Handling of duplicate libraries. Kind of related to #465908: Scan: Name clashes but could also cause a problem if two module maintainers try to implement the same library with different information. I haven't quite thought that through. Anyways, not done yet.
• Dependency checking
• CTools integration
• libraries_get_version()

Definitely!

Something that just came to mind:
If this is already a whole module only for this purpose, I think it should do its job right and, when checking for the existence of the library, not just check for the directory but check all the files as well. This would have to be done at the end, though, when the 'files' overloading has already taken place. Then we could even put in some error, like 'Your library seems to be corrupt, not all files were found' (in nice). This, of course, would above also go in the category 'not yet implemented'.

Great!

We need to talk about your coding style at some point though. ;) http://drupal.org/node/1354 and surrounding pages hopefully help with the rough edges, so we can deal with remaining details afterwards. Let me know whether you want to do that upfront (before introducing more code that needs adjusting) or later :)

1) It seems you copied the version override logic from Wysiwyg. If you didn't alter it, then it should work (already tested by thousands of users ;)

2) You renamed the 'integration files' key, but its value is not suitable for anything I know. :) Also note that we absolutely need this data to be keyed by module. Since the subkeys are not files, the rename to 'integration files' probably isn't perfect. However, I agree that 'integration' is a very broad term.

3) The propertes in 'versions' need to replace the "default" properties. A later version of a library may remove files, rename them, or whatnot.

4) I agree that the plain registry needs to be split from the actual library detection and version handling. Most importantly, because we need to prepare (+ alter) + cache that registry info, without even knowing whether we will load or use any of those libraries on a page. Secondly, the squeezed in drupal_alter() mentioned in the last sentence needs a proper location, still not affecting actual usage of libraries. Lastly, I'm not even sure whether two stages are sufficient. I'd rather consider 3 stages: registry - detection - usage. We will need detection without usage for a central administration page that lists all supported/required libraries in a Drupal site (likely in Administer » Reports).

5) I agree on deferring variants handling. Let's just define how it should/could work out for the registry for now.

6) If the same library is registered by more than one module, then we need to account for module weights. We'll deal with all 4 dimensions when we have to (likely to happen when usage statistics reach >10,000 users).

7+8) yep, deferred.

9) The default version callback, we can equally copy from one of Wysiwyg's editors. YUI's version callback looks like a good template to me, just needs some variables replaced. Note that we should use named arguments for 'version arguments', i.e. array('file' => 'sub/dir/file.txt', 'pattern' => '@(\d+)\.(\d+)@', 'lines' => 100).

10) Not sure whether a further validation of files makes sense. What matters is that some primary files are located at the right place. We equally do not support partially uploaded Drupal modules or themes. And no one on this earth will support hacks or customizations in libraries. ;)

Lastly, you mentioned that you wrote yourself a testing module... btw, do you know how to write tests? :)

All right, I hope I don't forget anything :):

0. I'll do coding style in the next round (not necessarily tomorrow, though).

1. Well I altered two things: a) Since we default to a version callback (ie we always have one) I removed an if. b) Since versions is now optional the whole thing is now in a giant if (...['versions']) {}. So if you don't specify versions, it will always succeed. The main parts are untouched, but before this is committed (at the very last) I will to one round of testing every possible scenario just to be sure.

2. Damn, I knew I forgot something. Yes, that totally needs to be keyed by module. I'm pretty sure, we're on the same page, but just to reiterate: I wouldn't actually change the signature of hook_libraries_info() to force modules to key 'integration files' by their own name, that should be done in libraries_get_info() (btw, what about the name, I just chose one that seemed somehow reasonable, but it's not really convincing, IMO).
Also but its value is not suitable for anything I know., yeah, found what you mean.

3. That's good then, no code change. But I guess a comment in the doxygen to warn people about this behavior would be adequate.

4. References to drupal_alter() always make run in fear so I won't comment on that. I'll try a seperation of 'registry' and 'detection' in the next round. Any ideas for function names?

5., 6., 7., 8. Great

9. That's good. Also a candidate for the next round. Thanks for the pointer. I like the named version arguments. I thought about that too, but then didn't go for it, because you don't have that eg in hook_menu(). All the better.

10. Granted. I guess it would be overkill.

11. Yes, I know how to write tests. But you will need to tell me how to make tests find something in sites/all/libraries. Oh wait, I get it, you just want to test the correct hook invokation (ie that the information ends up correctly in the registry?!). Yeah, I didn't think about that. That would also cover 1. then. Also, I always hear people talk about unit testing (as opposed to functional testing. Or something?). But I never got that. So if you want unit tests (better: if unit tests are adequate here) I would need some advice.

So just a little TO-DO list for the next time I'm feeling ambitious:

• Make 'integration files' keyed by module
• Coding style and better comment for 'integration files'
• Simpletests
• Seperate the 'registry' part from the 'detection' part
• Make version arguments named
• Provide a libraries_get_version()
• Better comment to warn people that they override all their files with 'versions' and 'variants'

Feel free to add more if I've missed something (or if something pops into your mind).

1. If if you don't specify versions, it will always succeed means that we still determine the version and record the 'installed' property accordingly, then ok.
2. There will be a hook_libraries_info() and a hook_libraries_info_alter(). Modules registering integration files need to add them in the proper location, so no magic involved here.

   A libraries_get_info() or just simply libraries_info() API function to retrieve registered libraries is something we actually didn't really discuss yet ;) We can go with either one and keep it simple for now, i.e. build the registry, and return the key that has been asked for. Happy to defer this detail, as that talks about retrieval already, and registering itself is complex enough ;)

3. :)
4. On API functions:
   • libraries_info(): Builds a registry of supported third-party libraries. (or returns info about a single given by name)
   • libraries_detect(): Detects which supported libraries are installed. (or returns whether a single given by name is installed)

     Note that Wysiwyg has some library detection error handling mixed into the registry process, which is quite valuable - differs between "not found", "unable to determine version", "not supported", and "cool, installed".

   • libraries_load(): Loads a library, if installed.

re: SimpleTests: Actually, I was rather kidding ;) I mean, it would be awesome, but we probably want to defer writing tests until we have a final patch/implementation RTBC here. Otherwise, we'd have to adjust those tests all over again. (I also have to prepare some initial .test files/directory to make this easier).

Quick follow-up, as I just saw another related issue hip-hopping around:

hook_library_info() currently defines 'path', the relative path to the library, (more or less) starting from sites/all/libraries. (Actually, the actual base path to start from is related to the Scan: Name clash issue, but anyway, different issue.)

Currently, Wysiwyg processes that relative 'path' into an (more or less) absolute 'library path'. (Actually, still relative, but the full filesystem path to the library.)

There can be edge-cases, in which a drupal.org project, downloaded as usual into sites/all/modules, shall be used as library, instead of module (BUeditor). True - Wysiwyg could simply direct users to download that module and extract it in sites/all/libraries instead, but FWIW, it should be possible to (pre-)register the full 'library path' upfront, if that is known already.

btw, stream wrappers and external JS/CSS support in D7 sound like... hahahah, crazy areas we should explore after this issue... ;)

All right, two things I don't quite get:

• 2. in #30: Is what you're saying that, since Modules will have to access the integration files through a 'module' key anyway in hook_libraries_info_alter() they should also define their integration files keyed by their own name in hook_libraries_info() for consistency? It's a trivial change either way, (either you change the Docs to tell people to key their 'integration files' by module name or you just do that in libraries_info for them), but it would be nice to know :)
• About the 'path' / 'library path'. Are you saying that 'path' should be translated into a 'library path' (ie if path is 'jscripts/tinymce' then 'library path' is for instance 'sites/all/libraries/jscripts/tinymce'), so that people could also specify 'library path' upfront (ie 'sites/all/modules/bueditor')? If so, should that be a hidden feature or should that be documented somehow?

• A libraries_get_info() or just simply libraries_info() API function to retrieve registered libraries is something we actually didn't really discuss yet ;) We can go with either one and keep it simple for now, i.e. build the registry, and return the key that has been asked for. Happy to defer this detail, as that talks about retrieval already, and registering itself is complex enough ;)

  I don't get what you're trying to tell me here, sorry.

On the other things in #30:
1. Yes, that's how it currently works.
4. Function names sound good. Not that I currently stole all that error handling from WYSIWYG so we can differentiate exactly the 4 mentioned cases.
SimpleTests: Well, I wrote that test module which registers a page with hook_menu and does a print_r(libraries_info()) there. Now I didn't write any assertions yet (ie I looked at that page manually) but that shouldn't be that hard or time-costly to do. Good to know that it's not that high on the list (for now), though.

Oh, and one other thing to add to the list of crazy things to do in D7: Use the Plugin manager to automatically download the libraries if they're not installed.

1. Yes, let's keep it easy for now - if a module registers a library, then it should register its own integration files in the proper location, just like any other module would do in hook_libraries_info_alter(). Any automatism would pretend that we would know - while invoking all hook_libraries_info implmentations - which module we're currently invoking, and if that module defines integration files, we'd munge the registered information afterwards, so that blablablah... :) But we don't gain anything serious from that, so it's rather a lot of code lines with little effect.
2. Correct. 'library path' should be parsed, processed, and generated internally. Docs on the property a probably required, as module authors need to know that this property is the one they want to use when working with the library. But docs on overriding, we can either leave out or keep very short.

   Now that I mention it, note that Wysiwyg even auto-generates yet another 'editor path', which originally was something different, but I'm currently unable to grok my own code. ;)

3. Nevermind - we actually did discuss libraries_info(), so I must have been confused.

Next round. More a cleanup version than anything new.

Not done (I guess in order of importance):

1. libraries_get_version()
2. libraries_load
3. SimpleTests (Note that due to the 'library path' key it will be easily possible to test all kinds of detection and non-detection)

Random notes/questions:

• I tried to comply to all coding standards, but there might be something I've missed. In that case, a pointer would be helpful
• I did not list the 'library path' key explicitly in the API doc but mentioned it in the 'path' description. I don't know if that is what you meant by: Docs on the property a probably required, as module authors need to know that this property is the one they want to use when working with the library. But docs on overriding, we can either leave out or keep very short.
• libraries_detect() passes $libraries by reference, because I thought it makes sense that way. I'm not that firm on that reference thing, though, so I could be wrong.

I will try to provide 1. in the next few days, which would make this patch pretty self-contained, I guess...

Ahh, found one thing you wouldn't have liked.

To move forward, I've taken your patch, revamped the docs and also the implementation a bit, and committed the attached patch to HEAD (7.x).

This does not mean we're done with this yet. ;)

By comparing the patches and/or code, you should notice some coding style issues I fixed along the way.

Also added a couple of @todos, which we need to figure out.

Additionally, I'm not sure whether the current override system of 'variants' + 'versions' + 'integration files' will work out in the long run (i.e. when thinking in full 4D). I fear that this override system worked/works fine for a single module like Wysiwyg, but not necessarily if applied globally. To explain the dimensions again:

• 1D: There is one library. It's just there.
• 2D: One library is registered and used by exactly one module. (Wysiwyg's case)
• 3D: A library is registered and used by many modules. (Which module registers? And which module wins?)
• 4D: A library is available in different versions, and is registered and used by many modules, and the actually available/installed version depends on what the site builder/administrator downloaded and extracted. Whether a module can use the library depends on whether the module supports the installed library version.

We perhaps need to invoke a new hook after detecting a library's version, to allow modules to return their integration files for the specific library version.

Or we need to drop the integration files from the registry and leave the inclusion/loading of those files to the integrating module. Though that would likely mean that we'd be targeting and limiting this system to a 1:1 relationship again (bad). So rather not.

Awesome!!! Love to see this move forward!
Some things I noticed reading your patch:

+++ libraries.api.php	3 Apr 2010 17:11:15 -0000
@@ -0,0 +1,280 @@
+      // It can be easier to parse the first chars of a minified file instead of

'chars' -> 'characters'

+++ libraries.module	3 Apr 2010 18:51:34 -0000
@@ -112,3 +112,141 @@ function libraries_get_libraries() {
+        $properties['module'] = $module;

If multiple modules implement the same library, won't this be overwritten every time? Maybe, though, I don't get this property correctly. Modules that depend on a certain library should be detected someplace else, right?

+++ libraries.module	3 Apr 2010 18:51:34 -0000
@@ -112,3 +112,141 @@ function libraries_get_libraries() {
+    drupal_alter('libraries_info', $libraries);

Wow, I didn't know it was that easy... No reason to run in fear, I guess :)

+++ libraries.module	3 Apr 2010 18:51:34 -0000
@@ -112,3 +112,141 @@ function libraries_get_libraries() {
+    libraries_detect_library(&$libraries[$name]);

I think you're not supposed to call parameters by reference, just declare the parameters by reference in the function declaration.

+++ libraries.module	3 Apr 2010 18:51:34 -0000
@@ -112,3 +112,141 @@ function libraries_get_libraries() {
+  // Special handling for named arguments (single array).

Awesome. I had thought about the need for something like that while writing the previous patch, but forgot to mention it. Great stuff!

We perhaps need to invoke a new hook after detecting a library's version, to allow modules to return their integration files for the specific library version.

Not that such a hook would be bad, but a module could also just specify integration files inside of the 'versions' array. That way it would conditionally be overwritten.

I fear that this override system worked/works fine for a single module like Wysiwyg, but not necessarily if applied globally.

I think a more foolproof approach would be the following in linbraries_info():
1. Key the libraries by $libraries[$library_name][$module_name]
2. Merge the values from $libraries[$library_name][$module_name] into $libraries[$library_name] and check for each value, if the values are equal or not.
- If they are, just use the value
- If they aren't, throw some kind error. Probably a watchdog call would be sufficient.
Also, the 'integration files' key can just be merged as is.

Powered by Dreditor.

Some of the issues you raised are valid and we need to fix those. In general, however, let's try to forget that this code is already committed and treat it as subject to change. I.e. let's not get too detailed about the actual code yet, but rather discuss the big picture.

That said, I'm not sure whether I understood what you meant in detail with your last suggestion of keying library information by module. While we should think of the possibility of multiple modules registering the same library, there at least has to be one thing that registers a library. If there is only one module that registers a certain library, then there is nothing to merge. If multiple modules are registering a certain library, the question is: what do module authors expect to happen?

Right we only need to do the whole thing I described, if there are multiple declarations of the same library. What I described, was a way to handle those multiple declarations from a technical standpoint. As you say, the question, what the result of that 'merge' is to be, is still open.
In my explanation above it was:
- If the infos are identical -> Do nothing and use the information
- If they are not -> put a notice in watchdog. (Once we actually have a UI with a list of Libraries, something should go there too, of course)
I guess the problem/question is not directly for module developers, at least not in every case. When you develop a module you don't necessarily test it with every single other contrib module. But I guess, the behaviour above, would simplify bug reports of 'Hey, this library isn't being loaded'. The maintainer would just have to say: 'Search watchdog for "foo" and paste it here, I'll do the rest.'

I just had to re-learn why Wysiwyg uses a "library path" next to an "editor path": #752516: Version callback failed to open stream

In short: If a registered library specifies a "path", then libraries_detect_library() will compile the actual path to the library + that path into "library path". The version callback always needs to get the unprocessed/unappended/original path to the library as "library path", so as to be able to access a changelog.txt or similar file outside of the actual library directory.

I think I already ensured that before committing this code. However, we likely want to clarify this in docs/inline comments, so as to not unintentionally break it in the future. :) Perhaps we should think about a separate property that holds the vanilla path to the library only, too.

More relevant for this issue: The version callback of YUI editor was optimized due to aforementioned issue.

Since use-cases are more worth than hypothetical stuff, I continue with another issue:

#276465: Add support for TinyMCE gzip compressor tries to add support for TinyMCE's native gzip compressor. Background: TinyMCE ships with 3 variants by default; Minified, jQuery, Source. Users can optionally download a 4th variant that's called "gzip compressor", and effectively changes the way how the library needs to be loaded. It's basically a single JavaScript that invokes a server-side PHP script that takes a list of JS files to aggregate and return in a gzipped BLOB. All of that is irrelevant for us here, but what's relevant: The library variant is optional. But to support it, the module registering the library likely needs/wants to register that variant as well (as it's still the same library, only a new variant).

To actually use the variant, the integrating module needs to expose a select widget or some other mechanism to choose the variant to load. As of now, it cannot know whether the variant actually exists.

This most likely means that we need to detect whether library variants are installed - after detecting whether a library exists, and after merging the version-specific overrides into the global library properties.

  library info -> path exists -> version detection -> version overrides -> variants detection

Patch contains libraries_get_version().
Well, that was a lot easier, than I thought. Thanks for the pointer to YUI.
This also fixes the two code-related issues I noted above.
This is untested, I will leave that to the hopefully soon coming SimpleTests.

Will work on libraries_load() now.

Todos:
- #41 (Variant detection)
- #37-#39 (Duplicate declarations. Maybe related to #465908: Scan: Name clashes?)
- 'library path' vs. 'path' vs. 'editor path': In order to avoid confusion, wouldn't it be easier to keep 'library path' as eg 'sites/all/libraries/tinymce' and then append 'path' to it when needed? That way we wouldn't have three different paths, of which two are a subset of the other.
- Resolving of the call-by-reference thing. Leave that to #466090: Scan: Cache results?

Now with libraries_load().
This one, too, is completely untested, so I'm pretty sure I messed up somewhere.

I hope to get some not so simple tests written for this after dinner.

+++ libraries.module	12 Apr 2010 18:36:49 -0000
@@ -250,3 +250,70 @@ function libraries_detect_library(&$libr
+/*
...
+/*

Docblocks need to start with /**

+++ libraries.module	12 Apr 2010 18:36:49 -0000
@@ -250,3 +250,70 @@ function libraries_detect_library(&$libr
+ * Load a library

The function summary should start with a verb in 2nd person (e.g. "Loads") and needs to end with a period.

+++ libraries.module	12 Apr 2010 18:36:49 -0000
@@ -250,3 +250,70 @@ function libraries_detect_library(&$libr
+  if ($files = $library['files']['css']) {
+    foreach ($files as $file) {
+      drupal_add_css($library['library path'] . $file, 'file');
+    }
+  }

Unfortunately, this is too simple... The array values can also be option arrays.

We likely have to fork most of the code from http://api.drupal.org/api/function/drupal_process_attached/7

+++ libraries.module	12 Apr 2010 18:36:49 -0000
@@ -250,3 +250,70 @@ function libraries_detect_library(&$libr
+function libraries_get_version($file, $pattern, $lines, $cols = NULL) {

Note that version callbacks with named arguments are getting a single argument (array). The PHPDoc needs to be updated, too.

Powered by Dreditor.

Rerolled for the first three things.

I didn't get the other one:

Note that version callbacks with named arguments are getting a single argument (array). The PHPDoc needs to be updated, too.

We're calling user_func_array, so each of the array values gets passed as an own parameter. The docblock for hook_libraries_info() is a bit unprecise about that, if that is what you meant, but I don't get, how the code should be updated.
Maybe I'll find that out, when the tests I'm about to write fail.

Well I started with some tests.
I noticed that I currently don't have a working SimpleTest environment, so ironically, this is untested.
Will flesh this out, hopefully this week, but wanted to put this out there.
I'm not quite sure if I took a good approach in how to test the libraries so improvements are as always welcome.

Subscribing.

Subscribing in case I miss it. Thanks

Subscribing. wrt jQuery Libraries requirements.

It's been quite a while, but I finally have a working development environment now.
New patch attached.
* Fixes a bunch of PHP errors / fatals
* Makes the SimpleTests work

Note:
On part of the test is commented out, because it does not work. I think that is because we currently just die if we don't find a file, instead of some kind of fallback, will investigate that, though.

@sun: I want to get the above mentioned problem working before I tackle the problems that are discussed above (ie variant detection among others, but I'm pretty sure there were others). I will have to read up on those things again, before I can even think about them. Assuming you've got a grand vision on mind, what we could do is that you just write the API documentation on how it should work and I'll see if I can get it to work that way.

I think the SimpleTests (while they most probably can be improved) do provide a more or less robust starting point for further development.

Also, if I'm feeling ambitious I might start an issue for a Libraries UI, because that would not only make sense in general, but would also ease testing, because running SimpleTest tends to be a bit slower than simply reloading a page which already gives you all the information.

(Disclaimer: I've not been around in this issue for the last ~4 weeks (and Drupal development in general), due to lack of time. The current week and the one after that, I should be able to squeeze in some time regularly and I plan to advance on this issue during that time. After that my efforts might be back to a minimum.)

Talk is silver...

Now this one actually works and passes all tests.
Next thing I'll do is re-read the last half of the issue and try to implement missing functionality.
Leaving at needs review, because the patch is now at a pretty self-contained and reviewable state.

Damn.

Sorry for the noise.
Just a small note, that libraries_get_version is not working currently. That's because in hook_libraries_info under 'version arguments' => 'file' you only specify the relative path, which is then passed straight on. But since libraries_get_version, does fopen, etc. we really need the absolute path.
Don't have a solution right not off the top of my head, but I'll think about it.

I'll believe the TODO-list I wrote in #43.

This patch implements variant detection (and loading).
I also noticed that we didn't load integration files yet, so I implemented that.
Both of this is untested, but there are no fatals and the existing tests still pass.

Still left to do:

1. Duplicate declarations. One possibility would be to introduce a $force parameter in hook_libraries_info(), as a quick fix for module developers. Basically needs a decision on what we want.
2. Cleaner implementation of libraries_detect() vs. libraries_detect_library(). Possibly leave that to a follow-up?

I think that's all, but I could be missing something.

Especially needs review on the implementation for:

• The separation of 'library path' and 'path'. More descriptive name for 'path' very welcome. Should we provide a 'full path' as well for convenience? I personally think that makes it more confusing, but I don't really care.
• Variant detection. For quick review, I introduced following text in the API doc.
  

  Additionally, each variant can contain a 'variant callback' and a 'variant arguments' key, which should return TRUE or FALSE, depending on whether the variant is available or not. If ommitted, the variant is expected to be always available.

• Variant loading
• SimpleTests

I think those are the things that are new since your last review.

Also comes with a proper patch name now.

Will roll a new version soon which implements the tests for variant loading and integration files.
Regarding the latter, I just noticed using my approach we limit people to actually including existing JS and CSS files in their modules as integration files vs. all the other options drupal_add_js/css have. We support those for the actual library files (upon your request) but not for integration files.

I actually think that is a good thing, but it would not really be hard to remove that limitation, that would just mean more code.

Wow, the last one was, again, pretty broken in relation to integration file and variant loading.
This one works, though! (Hurray for test-driven development).

#57 still stands, except that, well, it now works!

I was trying to build a UI for Libraries (will hopefully open a new issue soon), and I found a couple of bugs, which made me reconsider the previous testing method.

This now tests rather atomically library-detection with all possible failure cases, libraries_get_version(), and library-loading including variant-loading.

Pretty sweet!

Something I thought about.
Imagine we support external JS (which isn't really hard (for D7) since we use drupal_add_js anyway) and Openlayers wants to use Libraries API. They have an external variant where the library is accessed online and one where you can download the library and access it directly. I guess that will be a fairly common pattern, when libraries implement external files. Then they will want to specify some 'order' in which the variants are assumed to be default if they are available. That way, if the internal library is available it would use that and if not it would use the external one by default.
We would get that feature (almost) for free, with following logic in libraries_load_files() (untested):

  if (empty($library['files']) {
    $variant = key($library['variants']);
    $library['files'] = $library['variants'][$variant]['files'];
  }

Also, I don't know if we want to support external JS in this patch. As noted, it shouldn't be too hard, we would just need to specify a way to declare that. I think it would be enough to define a constant such as LIBRARIES_EXTERNAL and then external libraries should use that as 'path', and then we check for that in libraries_detect_library().

Both of those basically need a decision on what we want (again...).

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -229,13 +231,13 @@ function libraries_detect_library(&$libr
     foreach ($library['versions'] as $supported_version => $version_properties) {
-      if (version_compare($library['installed version'], $supported_version, '>=')) {
+      if (version_compare($library['version'], $supported_version, '>=')) {
         $version = $supported_version;
       }
     }

1) Are we sure that the "version" key isn't used already? Not sure whether "installed version" isn't more precise?

2) Alternatively, would it make sense to have "installed" not be a boolean in case of TRUE, but the installed version number? Not sure whether that makes sense. Perhaps not really clean.

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+ * @param $variant
+ *   The name of the variant to load.

(and elsewhere) Missing "(optional)" and info about the default value if omitted. See http://drupal.org/node/1354 for examples.

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+function libraries_load_files($library, $variant = NULL) {
+
+  if (!empty($variant)) {
+    if (!empty($library['variants'][$variant]['installed'])) {
+      $library = array_merge($library, $library['variants'][$variant]);
+    }

1) Why the extra newline?

2) What's happening here? Can we add a comment?

3) Why two separate if conditions?

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+  // Load both the JavaScript and the CSS files.
+  // The parameters for drupal_add_js() and drupal_add_css() require special
+  // handling.
+  foreach (array('js', 'css') as $type) {

Let's append a @see to this comment, stating from where we forked this code, for future comparison.

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+        require_once($file_path);

require[_once] and include[_once] are statements, not functions. We write them without parentheses.

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+ * @param $lines
+ *   The maximum number of lines to search the pattern in. For example: 20.

Like below for $cols, I wonder whether we should also have a default value for $lines, e.g., 20.

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+ * @param $cols
+ *   (optional) The maximum number of characters per line to take into account.
+ *   For example: 40. Defaults to unlimited. To be used if the file containing
+ *   the library version is minified/compressed, i.e. reading a single line
+ *   would read the entire library into memory.

Actually, should we default to a reasonable maximum length per line? For example, 100-200? fgets() will stop at whichever comes first, line end or maximum. That way, we'd prevent people from mistakenly reading entire compressed script files into memory...?

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+  while ($lines && $line = call_user_func_array('fgets', $args)) {

Why do we need call_user_func_array() here? That makes things even slower... ;)

+++ libraries.module	30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+    } ¶

Trailing white-space.

+++ tests/libraries_test.inc	30 May 2010 22:01:30 -0000
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ tests/libraries_test.info	30 May 2010 22:01:30 -0000

wow! I'm going to commit some empty files here.

Powered by Dreditor.

wow, I've to say this is some quite impressive and well work, tstoeckler! Well done!

I've just committed all files below /tests, including smaller quick fixes and changes not worth to mention (you may want to compare your working copy).

Not committed yet are the main API changes and additions. I hope to be able properly get back to this issue very soon, to re-read and re-confirm the proposed changes. But all in all, they look very promising!

Lastly, I'm feeling comfortable with granting CVS access to you now. Welcome on board! 8-) (Not sure whether we already talked about maintenance, but we have some best practices[, which actually state that you're free to go ahead if I won't come back to review this patch in a reasonable time-frame ;) ])

---

Also, you don't seem to have a twitter account... anyway, http://twitter.com/tha_sun/statuses/18677478499

err, since I already have it applied, I could have re-attached this sucker. :)

Wow, Thanks! That's great!!!

I read http://drupal.org/node/363367 and it seems to very coherent with what I would consider best practices, so that's good :)

I think now that we're on the verge of solving the very base problem of Libraries (for now!!!) once we've fleshed out this issue we can actually have contrib implement this and then see how all this works out with different libraries. Also this paves the way for the other major issues in the queue right now.

So I'm glad to be on board and look forward to (external) libraries support in Drupal 8 :) Hehe...

Now onto some comments. I'll leave things out that are uncontroversial/I agree with:

1) Are we sure that the "version" key isn't used already? Not sure whether "installed version" isn't more precise?

It isn't used, but it might be confused with "versions". On the other hand there is only one version at a time and that is of course the installed version so "installed version", which is what WYSIWYG uses, could be seen as redundant. I don't really care, but we wen't with "version" in the patch that was your previous commit, and the current patch just makes is consistent (== working).

2) Alternatively, would it make sense to have "installed" not be a boolean in case of TRUE, but the installed version number? Not sure whether that makes sense. Perhaps not really clean.

I don't find that to be very clean either, but again, not very attached to that emotionally :)

+++ libraries.module 30 May 2010 22:01:30 -0000
@@ -243,10 +245,137 @@ function libraries_detect_library(&$libr
+function libraries_load_files($library, $variant = NULL) {
+
+  if (!empty($variant)) {
+    if (!empty($library['variants'][$variant]['installed'])) {
+      $library = array_merge($library, $library['variants'][$variant]);
+    }

2) What's happening here? Can we add a comment?

That's just the variant override. Comment is definitely useful.

3) Why two separate if conditions?

Just checking for the second one gives you a PHP5.3 notice, but from (re)reading it now, it seems like they could be one line?! Will try that out...

Like below for $cols, I wonder whether we should also have a default value for $lines, e.g., 20.

I think that's a great idea. Since everyone implementing a library has to have a version callback, we should try hard to make libraries_get_version() as simple as possible. Then most people would only have to enter a file and a PCRE expression. We could even document 3-4 common regular expressions for people to copy&paste.

Why do we need call_user_func_array() here? That makes things even slower... ;)

There war a reason, ie something not working. But I don't remember that off the top of my head. Will look into that.

Concerning the @todos you added in the test files concerning the "missing" files:
1. I don't know if a top-level 'files' property makes sense, I just blindly assumed yes.
2. I originally had all of these "missing" files there and tested that they were NOT loaded, but at some point in time that seemed like overkill to me, so I removed it. Don't really know what makes sense here.

we went with "version" in the patch that was your previous commit, and the current patch just makes is consistent (== working)

oopsie. heh, seems we're good to go then ;)

could be one line?

aight.

Then most people would only have to enter a file and a PCRE expression.

ok, would be good to find the common, maximum values from existing version callbacks in Wysiwyg then (they're quite different, IIRC).

something not working. But I don't remember

you know what that means, right? Always document shit! :) Better one line of documentation too much than missing one.

Basically, whenever you're talking to yourself, or asking "WTF?", or starting to experiment, or whatnot -- all key signs for documentation being required.

But in the end, I believe we're able to remix this line without cufa().

I don't know if a top-level 'files' property makes sense, I just blindly assumed yes.

IIRC, we discussed here that a top-level 'files' property makes sense for libraries, which don't implement variants; i.e., if there can only be one set of files, that's it. Otherwise, files need to be defined per variant.

Actually not sure whether we discussed this here, but it would make sense. :)

I originally had all of these "missing" files there and tested that they were NOT loaded, but at some point in time that seemed like overkill to me, so I removed it. Don't really know what makes sense here.

I'd consider this an essential part of the API that needs to be tested... if we are supposed to load certain files only, including support for version-specific overloading, then the API better doesn't load files it shouldn't load.

All right. Just one thing left:

I'd consider this an essential part of the API that needs to be tested... if we are supposed to load certain files only, including support for version-specific overloading, then the API better doesn't load files it shouldn't load.

That actually means we won't have to add any more files though. We just remove the top-level files property, but leave the files for version '1' (the wrong one) as 'missing', because for version overrides we can just test what ends up in the $library array.

Sure, but also not so sure. I think that we'll have to rebuild the tests at some point anyway, to make them more atomic unit tests. For example, the first libraries named after their special testing purpose will likely lead to more precise testing results than the other, big library that we're using to test variants, file loading, overloading, etc.

We likely can register many more libraries, from which some are referencing the same testing files, so as to being able to separately test a top-level files property, variant overloading, version overloading, variant + version overloading, loading of integration files, aso. - i.e. all the details that we are presuming and expecting right now.

All right. New version attached.
This fixes all mentioned problems. Well, the easy ones...
A couple of "API changes":

• Every version callback and variant callback now gets the library path as the first argument. For libraries_get_version() to be of any use, we already needed to special-case that particular function and generalizing that, IMO makes sense.
• I made libraries_get_version's parameters default to: $cols = 200, $lines = 20; I didn't yet add more examples of PCRE expressions to the API doc. I tried but was a bit unsure and too lazy to go stealing from WYSIWYG again.
• I documented hook_libraries_info_alter().
• I revamped the tests to achieve the mentioned atomicity (?!). I didn't add any new files to check for, but I guess we should decide on what we want.
  1. We can either just have two sets of files example_correct.xxx and example_incorrect.xxx and then adapt the library definitions so that we always check for the correct file (literally). This is basically what we have now, except the two files are called example_installed_2 and exapmle_installed_variant, which is kind of weird.
  2. Or we can actually add files for each of the versions and variants that would lead us to 4 or 5 sets of files:
     example.xxx (could be omitted, potentially)
     example_version_1.xxx
     example_version_2.xxx
     example_variant_1.xxx
     example_variant_2.xxx

I tried splitting this up into smaller chunks to make it more reviewable, but I always got stuck somewhere. It's just too interconnected. On the other hand, once this sucker is in, it paves the way for a lot of other cool stuff...

Less whitespace and a small optimization, where libraries_load_files was calculating the library path twice.

The whitespace is more persistent than I thought.

Ahhhh....

I swear this is the last one. I just saw I forgot to update the parameter descriptions for libraries_get_version in the function declaration.

Still 35 assertions (yeah!).

Thanks!

Every version callback and variant callback now gets the library path as the first argument. For libraries_get_version() to be of any use, we already needed to special-case that particular function and generalizing that, IMO makes sense.

I'd pass the entire $library info, not just the path.

+++ libraries.api.php	20 Jul 2010 00:41:43 -0000
@@ -48,7 +48,12 @@
- *     the variant, most often just 'files'. Variants can be version specific.
+ *     the variant, most often just 'files'. Additionally, each variant can
+ *     contain a 'variant callback' and a 'variant arguments' key, which should
+ *     return TRUE or FALSE, depending on whether the variant is available or
+ *     not. The first argument of the variant callback is always $library_path,
+ *     a string containing the library path. If ommitted, the variant is
+ *     expected to always be available. Variants can be version specific.

Would it make sense to start a new sub-property-list for 'variant callback' and 'variant arguments'? Of course, only containing special keys. It won't be easy to clearly state that the properties belong to each variant, i.e., onto a sub-key of a sub-key of 'variants' in reality.

Same as below with $library[_path] here.

+++ libraries.module	20 Jul 2010 00:41:44 -0000
@@ -210,18 +210,17 @@ function libraries_detect_library(&$libr
-  // Special handling for named arguments (single array).
-  if (!isset($library['version arguments'][0])) {
-    $library['version arguments'] = array($library['version arguments']);
-  }

d'oh! uhm, here you're removing the one-argument feature I'm talking about below... :-|

+++ libraries.module	20 Jul 2010 00:41:44 -0000
@@ -243,10 +245,146 @@ function libraries_detect_library(&$libr
+        $info['variant arguments'] = array_merge(array('library_path' => $library['library path']), $info['variant arguments']);

(and elsewhere) Attention: array_merge() potentially alters array keys. It's better to use

$info['arguments'] = array('library' => $library) + $info['arguments'];

Also, we should be consistent with those callback arguments. The version callback supports an indexed list, which are passed as separate arguments.
In general, I'm more in love with the named parameters (which result in a single keyed array as argument to the version callback, also skipping cufa()), so I'd have no problem with standardizing on this type, perhaps. We could then pass $library as first, and $options as second argument. Also, AFAICS, the current code does not implement/ensure default values for the named parameters yet.

+++ libraries.module	20 Jul 2010 00:41:44 -0000
@@ -243,10 +245,146 @@ function libraries_detect_library(&$libr
+  // Load PHP files.
...
+      $file_path = "$path/$file";
+      if (file_exists($file_path)) {
+        require_once $file_path;

Should we prefix $file_path with DRUPAL_ROOT ?

+++ libraries.module	20 Jul 2010 00:41:44 -0000
@@ -243,10 +245,146 @@ function libraries_detect_library(&$libr
+ * @param $file
...
+function libraries_get_version($library_path, $file, $pattern, $lines = 20, $cols = 200) {

First @param is missing in PHPDoc.

Powered by Dreditor.

Well, I did pass the whole $library first, but then I had some smaller errors, and doing a debug($library) is just horrible when you see everything twice. Technically not a problem, though.

In general, I'm more in love with the named parameters (which result in a single keyed array as argument to the version callback, also skipping cufa())

Ahhh, now for the first time I understand what you're trying to tell me... Right I will adapt the code for that. Then of course the lines I removed for the single parameter handling are indeed needed. (They weren't needed because we were calling cufa() anyway...).
I'll roll the path accordingly, but if only for reference I must say, that I actually don't like it that way for 2 reasons:
1. It's inconsistent with what we do for example in hook_menu()
2. It makes the function declaration less readable (Instead of just seeing $param1, $param2, $param3, I actually have to look in the PHPDoc to see what's going on).
Also the version detection shouldn't be happening very often, so the cufa() shouldn't be that much of a problem.

First @param is missing in PHPDoc.

I thought it'd be like form callbacks where we don't document the automatically prepended $form and $form_state either. No big deal, though.

Hopefully the next patch will be tonight.

It'd be cool, if we could commit some version of this soon and then maybe refine in follow-ups as this really holds up all other issues currently.

You're right about the documentation issue. But if you also consider variable arguments, optional arguments, but also arguments that may be changing over time, then it quickly becomes apparent that arguments like array(20, 20, 'foo', 1) are also not very self-explanatory, and the corresponding function signature of $rows, $cols, $prefix, $someboolean is poorly driven by expectations.

That said, we can keep the current behavior and just support both. We just have to ensure that both callbacks/arguments behave the same, and named arguments are always getting certain default values.

You're also right that version detection shouldn't happen very often. However, as of now, Libraries API is not using any kind of caching, so it can literally happen on every single request, for every single library ;) [different issue]

To document $library, we should (already?) have a standard description text, which anything else can just copy and paste. Similar to $form_state being documented in form.inc (HEAD, that is).

Happy to see commits and de-block other issues, as long as we keep the remaining changes in the loop.

So if I understand you correctly we would do the following logic:

if $library['version arguments'] is a string, just pass it as a second parameter after $library.

if $library['version arguments'] has numeric keys pass them as individual parameters after $library.

if $library['version arguments'] has string keys pass them as a single $options parameter after $library.

I would favor that approach. I mean we are trying to cater for all possible use-cases architecturally, so why not in this API decision also. We can always restrict the options later.

mmmmh, actually only the 2nd and 3rd, as the existing code for 'version arguments' should already cover. If people want to pass a simple string, they can pass array('anystring') :)

Here's where I got tonight.

1 Test is still failing. Will look into that.
The whole "double-API" is making things a lot more complicated internally, I have to say.

One thing that bothered me though is that during debugging I temporarily constructed the file path wrong, but only the PHP file tests failed, the CSS and JS tests didn't. So I'm guessing our tests are flawed =(.

Still to do:
- The renaming of the test files (will go with the 2. approach i #69 ?!)

Concerning a commit. I guess since we're in the middle of a rather important outward facing API change, I'll let this pass for another round of reviews.
I plan to provide a patch for #466090: Scan: Cache results soon and will open new issues for 1. handling of external JS and 2. automatic variant loading (see #61)

It's late...

Oh, just noticing while looking at the patch file:
I tried some doc changes, but, well... ...it's late :), so I might not have nailed it.

I need a Dreditor plugin for gedit...

+++ libraries.api.php	22 Jul 2010 00:44:09 -0000
@@ -22,20 +22,26 @@
+ *     - options: An associative array of additional information to pass to the

@@ -48,7 +54,21 @@
+ *       - options: An associative array of additional information to pass to

By documenting the key "options", readers are getting confused about the key. Reading this, I would have assumed that I would have to specify an array key 'options', which holds the named arguments.

Actually not sure how to document this properly. Perhaps a list isn't the best solution.

+++ libraries.api.php	22 Jul 2010 00:44:09 -0000
@@ -122,6 +144,10 @@ function hook_libraries_info() {
+        'variant callback' => 'mymodule_check_variant',
+        'variant arguments' => array(
+          'variant' => 'minified',
+        ),

Interestingly, all variant callbacks will have to pass arguments like that in order to identify, for which variant they are being called?

+++ libraries.module	22 Jul 2010 00:44:10 -0000
@@ -210,18 +210,25 @@ function libraries_detect_library(&$libr
 
+
+

Stray newlines added here?

+++ libraries.module	22 Jul 2010 00:44:10 -0000
@@ -243,10 +253,164 @@ function libraries_detect_library(&$libr
+  $options += array(
+    'file' => '',
...
+  $file = DRUPAL_ROOT . '/' . $library['library path'] . '/' . $options['file'];
+  if (!file_exists($file)) {
+    return;
+  }

I'm pretty sure that file_exists() will return TRUE if 'file' is (mistakenly) an empty string, since the directory exists.

+++ tests/libraries_test.module	22 Jul 2010 00:44:12 -0000
@@ -10,38 +10,41 @@
+    'version callback' => '_libraries_test_get_version',

@@ -137,62 +268,126 @@ function libraries_test_libraries_info()
+function _libraries_test_get_version($library, $version) {

Could we rename this to libraries_test_return_version(), so as to clarify most of the testing code?

I.e.

libraries_test_return_version('2')

...does what? yes, return '2' :)

+++ tests/libraries_test.module	22 Jul 2010 00:44:12 -0000
@@ -137,62 +268,126 @@ function libraries_test_libraries_info()
  * Returns exactly the version string entered as the $version parameter, unless
  * you specify 'undetected', in which case it returns nothing.
...
   if ($version !== 'undetected') {

I think we're actually able to pass TRUE/FALSE/NULL as arguments... no? Why the special string value?

+++ tests/libraries_test.module	22 Jul 2010 00:44:12 -0000
@@ -137,62 +268,126 @@ function libraries_test_libraries_info()
+ * This is an exact copy of libraries_get_version() except for the fact that it
+ * does not take a single associative array as a parameter but multiple
+ * parameters. Since we support both type of version callbacks this might be
+ * a useful reference for a custom version callback that uses multiple
+ * parameters

It would be lovely to learn why we need to fork the function to use a list of arguments instead -- the most important "why?" is missing in the PHPDoc :)

If it turns out that named arguments do not work as expected, then we might have to consider to drop them.

Powered by Dreditor.

Just wanted to make sure that #763378: Meta Issue: jQuery Module is on your radar; most likely the very first consumer of this new API functionality.

Updated version now. This passes the tests now. And adds one test for the alternative version callback.
You were right regarding the variant callback. We now pass the $name in addition to $library.

TODO:
- Fix the docs on the "double API"
- Debug the miraculously not-failing CSS/JS tests + check the file_exists()
- General in-depth doc-review
- General in-depth test-review

If we can agree, that this is the API we want (regarding the version callback) I guess those items would make good follow-ups.

+++ libraries.module	22 Jul 2010 21:39:16 -0000
@@ -243,10 +250,162 @@ function libraries_detect_library(&$libr
+  // Construct the full path to the library for later use.
+  $path = isset($library['path']) ? $library['library path'] . '/' . $library['path'] : $library['library path'];
+  $path = DRUPAL_ROOT . '/' . $path;

DRUPAL_ROOT only applies for PHP files. Totally #fails for JS + CSS files ;-)

After fixing that, i.e., aside from that, let's turn this around: If _you_ feel comfortable with this patch, please go ahead and commit. Documentation and stuff can be finalized and fixed in follow-up patches; though I'd rather keep those follow-up patches in this issue.

Committed the attached version. YEAH!!! That feels good. :)

I went through the tests in detail (there was quite a bit of stale code) and made sure everything is how it is supposed to be.
That uncovered a couple minor bugs (eg I had forgotten some '/' when constructing the file paths, etc.) which are now fixed.
The CSS / JS loading and testing also works. Before I had added debug($library) somewhere while testing, so that the file names of the wrong files showed up inside of the debugging info, which caused the tests to always pass.
I added some CSS and JS into the test files for testing / debugging purposes, as we currently have no way to actually test a specific portion of JS or CSS was loaded. Just look at one of the "Debug messages" when you run the tests to see how that looks like.
Fixed DRUPAL_ROOT in drupal_add_css/js (...had I mentioned it was late?...)

What is left now is to finish up the docs. I read through the sections changed in this patch again, to make sure nothing is actually incorrect as of now, but there are some parts which could be better. Especially the fact that there is two ways to do version/variant callback (which is de-facto a good thing), can be quite confusing, since we currently don't do a very good job of explaining the situation (as you already pointed out above).

Also:
In #465908: Scan: Name clashes you mentioned an .info file approach. IMO since this is highly related to hook_libraries_info() (which is after all, the title of this issue :)...) this might also be a good follow-up. I won't insist, though, if you want to stay two-digit in here.

The whitespace never dies...

Just a note that I signed us up for SimpleTesting over at:
#689990-55: Contrib projects to be included in beta stage of automated testing for modules

Lovely. :)

So, in addition to the documentation issues already mentioned earlier:

+++ tests/libraries_test.css	23 Jul 2010 12:44:01 -0000
@@ -1 +1,15 @@
+ * Because we cannot test CSS programatically with SimpleTest, the CSS below can
+ * be useful for debugging with SimpleTest's verbose mode. Note that since the
+ * DOM cannot be manipulated via CSS, JavaScript loading needs to be functional
+ * for this to have any visible effect.

I don't grok this comment. (...but see below...)

+++ tests/libraries_test.module	23 Jul 2010 12:44:01 -0000
@@ -10,38 +10,41 @@
  * Note: DO NOT use drupal_get_path() in your implementations! Used for testing
- * purposes only.
+ * purposes only. It is strongly discouraged to declare the 'library path'
+ * property as that will be detected by Libraries API automatically.

We can combine the two warnings into a single one.

+++ tests/libraries_test.module	23 Jul 2010 12:44:01 -0000
@@ -137,62 +284,125 @@ function libraries_test_libraries_info()
+function _libraries_get_version($library, $file, $pattern, $lines = 20, $cols = 200) {

Should be in the libraries_test namespace, not libraries.

+++ tests/libraries_test.module	23 Jul 2010 12:44:01 -0000
@@ -137,62 +284,125 @@ function libraries_test_libraries_info()
+function _libraries_test_detect_variant($library, $name, $status) {
+  return $status;

Same like version here -- ..._return_status() ?

+++ tests/example/example_installed_2.css	23 Jul 2010 12:44:01 -0000
@@ -1 +1,15 @@
+div#libraries-test {
+  color: green;

ah, now I get it! ;)

I think we can shorten those @file docs and more importantly tell something about multiple CSS and JS files applying different colors.

Powered by Dreditor.

I think this fixes all mentioned items.

Hmm. Maybe some notes on the non-trivial changes:
1. The @file declarations in all the test JS/CSS files. I shortened those to a minimum and added an extensive paragraph in example_installed.txt explaining the testing method.

2. On discouraging people to use 'library path' and drupal_get_path. I tried hard to get the latter in there, but it didn't really make sense. 'Libraries API will automatically detect that' is kind of obvious, IMO. Maybe I was just not creative enough, though.

Just a note that I have no clue why the patch doesn't apply. It's probably something with my system as I'm getting weird conflicts when applying the patch. Should be review-able though, anyway.

What a nice way of pulling me into this issue :)

+++ libraries.api.php	8 Aug 2010 13:54:32 -0000
@@ -21,17 +21,26 @@
  *   - version callback: (optional) The name of a function that detects and
...
+ *     returns the full version string of the library. There are two ways to
+ *     declare the version callback's arguments, which correspond to the two
+ *     ways to specify the argument values (see 'version arguments'). The first
+ *     argument is always $library, an array containing all library information
+ *     as described here. Either the second argument is $options, an associative
+ *     array whose keys are the parameter names. In this case the version
+ *     arguments (see below) must be declared as an associative array. Or the
+ *     following arguments (after the first one) are declared independently as
+ *     real function arguments. In this case the version arguments (see below)
+ *     must be declared as an indexed array. Defaults to
+ *     libraries_get_version().

Somehow, most of this description is documentation about 'version arguments', but not about 'version callback'. Why do we document arguments for the callback property? :)

+++ tests/libraries_test.module	8 Aug 2010 13:54:32 -0000
@@ -9,9 +9,8 @@
+ * Note: DO NOT declare the 'library path' property in your implementations!
+ * It will be detected automatically by Libraries API.

Hm... re-reading this with fresh eyes, I wonder whether it wouldn't be better to skip this function header description, but put short one-line notes right above the actual code lines that define the 'library path' property...?

Something along the lines of:

function ...
  $libraries = array(
    '...' => ...
    // Never declare library path manually. It is detected automatically.
    'library path' => ...
                                                                               | 80 chars.

+++ tests/libraries_test.module	8 Aug 2010 13:54:32 -0000
@@ -155,7 +154,7 @@ function libraries_test_libraries_info()
-        'variant callback' => '_libraries_test_detect_variant',
+        'variant callback' => '_libraries_test_return_installed',
         'variant arguments' => array(FALSE),

:) Had to look this up. After looking it up, I wondered why we don't simply call this _libraries_test_return_argument()? 8)

+++ tests/libraries_test.module	8 Aug 2010 13:54:32 -0000
--- tests/example/example_installed.txt	23 Jul 2010 12:57:01 -0000	1.2
+++ tests/example/example_installed.txt	8 Aug 2010 13:54:32 -0000

heh. With those extensive docs -- how about renaming this file to README.txt? :)

Aside from those, nothing left to hold off this commit. Anything else we still need to follow up or fix in this issue?

Of course, we want our tests to pass. However, making PIFR behave is not always easy, probably better to rule out in a separate issue.

Powered by Dreditor.

:) Had to look this up. After looking it up, I wondered why we don't simply call this _libraries_test_return_argument()? 8)

Because different arguments are passed automatically to version callbacks and variant callbacks.
I don't like the function name that well either but I think it is clear in context with the arguments right below it:

$library['example']['variants']['example_variant'] = array(
  'variant callback' => '_libraries_test_return_installed',
  'variant arguments' => array(TRUE),
);

Changed the rest of the comments. The API doc is now less redundant concerning the different ways to declare callback arguments.
Attaching patch for self-review, will commit later.
(Note that for a reason unknown to me, the removal of example_installed.txt is not included in this patch, I will do that manually)

Just like sending an e-Mail without an attachment...

Duh.

#95: 719896-docs-cleanup_2.patch queued for re-testing.

Good thing we have testing now, I can't believe I missed that.
In the meantime, I kind of refactored the tests a bit. We really don't have different versions and variants currently, but simply 4 sets of arbitrary files, that we, in our tests, associate with different versions and variants. It is impossible though, to find a naming that fits all.
Therefore, I renamed the files to example_1.css, ...js, ...php through example_4.php
I also but the asserting of files into a seperate helper function. That makes the code flow of the actual testing code easier to follow. As a side-effect we get a couple more assertions, because we now test that the wrong files are not loaded for all libraries (not just the 'complicated' one as it was before).

I guess I'll leave this for another review, to see if you like this change.

Just to let you know, I've just re-reviewed the code, rather thoroughly and am fine with it the way it is. As I've said above, since this makes a considerable change to the testing system, I'll let you review it once more.

here's my ideas again:
http://drupal.org/node/681034#comment-3437286

i see you guys want to do multiple versioning. that would be more complex and more functional ^^
but a question, won't loading multiple different version jquery.ui in the same page/node/region for example, create some chaos?

like in same function names but different code

Please read the above issue for more information. Short version: We support different versions of a library, but only one at a time.

I reviewed this patch again and could find no issues with it. The (new) tests pass.
I decided to commit this, so we can move forward with other issues.

Marking fixed, because I'm pretty sure that nothing that came up in this issue hasn't been fixed now.

And here's the commit link.

Ehhh... sorry.
http://drupal.org/cvs?commit=426000