Revamp Libraries API integration

Have not yet tried the patch, but two things come to mind (probably follow-ups though):

1. Since a special version of modernizr is needed, it probably doesn't make sense for it to be the site-wide version. Can we just name it modernizr-navbar so other modules can add other special builds? Or would that still clash when multiple versions of modernizr are loaded at the same time? I guess it would, as the docs suggest: https://drupal.org/node/1342220#libraries-version
2. Does Libraries API have a way to disable the minified version without removing it from sites/all/libraries/xyz/ ?

Patch seems to work perfectly.

One thing I noticed: with this patch the minified file should be named modernizr-min.js, while the README says modernizr.min.js

+++ b/navbar.module
@@ -924,38 +916,33 @@ function navbar_libraries_info() {
+    'variants' => array(
+      'minified' => array(
+        'files' => array(
+          'js' => array(
+            'modernizr.min.js',
           ),
         ),
+        'variant callback' => '_navbar_libraries_minified_variant_exists',
+        'variant arguments' => array('modernizr-min.js')
       ),
     ),

Two different file names. I'll change the variant check to modernizr.min.js

+++ b/navbar.module
@@ -924,38 +916,33 @@ function navbar_libraries_info() {
+    'versions' => array(
+      // Means ">=2.6.2": matches 2.6.2, 2.7.1, etc.
+      '2.6.2' => array(),
+    ),

Ok, so by declaring 'versions', we get a minimum version number for Libraries API to check against.

Alright, this is finally the implementation that I'm happy with. Wim, I hope you're cool with it as well.

All files are now declared in a named variant under a version. We can do this because of the variant existence checking that Wim added in the previous patch.

A library is determined to be installed if at least one of its version's variants is installed. Previously, a library was assumed to be installed if it had a version that matched the physical file's version even if eventually nothing got loaded.

The variant loading order is determined by the variant order property on the library (custom to navbar). This property can be overridden by another module using hook_libraries_info_alter. So no where now does the code impose the minified version; the variants and their preferred selection order are determined by configuration of the library info. Another module could conceivably add a variant to the library info of Navbar if needed for the installed library version.

Also, the status report now lists what variant will be loaded.

Navbar: Backbone library      1.0.0 (minified)
Navbar: Modernizr library      2.7.0 (source)
Navbar: Underscore library      1.5.0 (minified)

As @Wim Leers already mentioned the version detection is currently quite fragile. This fixes that which also allows to remove the post-detect callback. (In other words the logic in the post-detect callback is moved into the version callback.)

I also added a @todo to _navbar_convert_libraries_to_library(). That function is really unfortunate and Libraries API should do that for you.

The major conceptual problem with this is that you are trying to enforce that if the 'minified' variant is available that it be used. There is no reason to do that, however. If people want to load the 'source' variant for some reason, they should totally be able to do that. One thing that Libraries API *should* do however, is to let libraries provide some default. To put this into code:

// Both of these are perfectly valid and should work in any case.
// (Although, of course, only one of them per page.)
libraries_load('backbone', 'source');
libraries_load('backbone', 'minified');

// However, Libraries API should be smart and load the minified variant
// (in case it's available) when I do the following:
libraries_load('backbone');

This might mean moving the 'variant order' that you introduced into Libraries API proper and then checking that in libraries_load(). Not sure yet, but I will open an issue to discuss that.

Here we go: #2167447: Allow libraries to declare their preferred variants

Oh, I forgot to say: I did not actually test that #12 works, so statistically it's probably broken.

Also, the one thing I don't understand about the patch is the version specification in hook_libraries_info(). Why is that done?

re #11, I opened an issue with a patch to make the error messages for version detection more explicit: #2167249: Make version/variant callbacks provide error messages themselves for better DX/UX.

tstoeckler, I'm psyched we're collaborating on this. I really want the Spark modules to have THE definitive implementation of Libraries.

Yes, I saw that. Haven't commented there yet, but it's a great idea. Thanks for that! That would also help the version callback I introduced here.

tstoekler, I don't quite understand this comment from your patch:

// If different versions have been found, there is no way to
// determine the correct one. We cannot use the information on the preferred
// variants because we cannot guarantee that a less preferred variant will not
// be loaded.

Does this mean that a variant label such as minified is declared under two numbered version arrays? e.g.

'versions' => array(
  // Means ">=1.0.0": matches 1.0.0, 1.1.0, etc.
  '1.0.0' => array(
    'variants' => array(
      'minified' => array()     
    ),
  ),
  '2.0.0' => array(
    'variants' => array(
      'minified' => array()
    ),
  ),
),

Would this result in no file being loaded?

As @Wim Leers already mentioned the version detection is currently quite fragile. This fixes that which also allows to remove the post-detect callback. (In other words the logic in the post-detect callback is moved into the version callback.)

Given the current code in libraries_detect, I don't think we can move the variant callback logic to the version callback.

For one, the callback in invoked around line 527 of libraries.module, but the variants are not moved to a top-level property in the $libraries variable until around line 558, when the versions property is processed, thus the version checking fails here:

function _navbar_libraries_get_version(array $library, array $options) {
  $versions = array();
  foreach ($library['variant order'] as $variant_name) {
    debug($library['variants'], '', TRUE);  <-- THIS IS UNDEFINED.
    $variant = $library['variants'][$variant_name];

    // @see libraries_detect()
    if (!isset($variant['variant callback'])) {
      $variant['installed'] = TRUE;
    }

The version callback is meant to return a version for the library, a string.

So, I thought about this some more and made the attached changes. Now, version callback compares the variant versions. If the versions are the same or NULL (meaning the version couldn't be determined), but not all NULL (meaning no version could be determined), then we return the unique single version string.

We unfortunately lose some error information here because we get one error message (maybe return an array instead?). So I've tried to return at least some useful info with additional error messages. I tested all the scenarios.

re: #19, to your first point, I'm not seeing this behavior with the latest Navbar code. If I change the name of the minified Modernizr file from modernizr-min.js to zmodernizr-min.js (note the prepended 'z' character), then the source file is loaded. Note that the name of the minified file should be modernizr-min.js, not modernizr.min.js so that the minified file name matches the Backbone and Underscore minified file name pattern.

And the opposite case where the modernizr.js file is renamed and made unfindable.

I think it's working about as well as we can get it to for the moment. Can you retest and see if you replicate the behavior I'm seeing? If you can, I vote we go with the code in Navbar as the pattern for the Spark module Libraries implementation.

Sorry for not following up earlier! #18 is certainly an improvement over the status quo so I won't un-RTBC, but I still think we can do better.

I at first didn't understand what you meant in #18 because I skipped over the code snippet. I just re-read that and now I get where you're coming from. Thanks for providing that snippet, that was helpful.

The problem is that the actual library (variant) file information is currently "hidden away" in the 'versions' property. Per definition this property can only be accessed once the version is known. However, since #12 it contains the version detection information for the variants. This is paradox, in short: You need to know the version to figure out the version. Instead the variant version detection information should be moved into the top-level 'variants' property at which point #12 starts working.

As I already mentioned above, but maybe did not explain well enough: I don't understand the reason why the 'versions' property is used at all. It is used when e.g. libraries move around files in newer versions and thus the library definition needs to differ depending on the version. In the code comment, it says something about the 'min' version.

What I think is happening, now that I think about it, is that the 'navbar' code depends on a certain minimum library version in order to work. Is that correct? In that case, what should really happen is that the navbar declares a *versioned* dependency on the navbar library. But since modules can't currently declare dependencies on libraries at all, I think it would be fine to leave in the 'versions' property for now.

The paragraph from above is still correct, though: The version-detection information should move up. I will provide an updated patch soon, but it might be tomorrow or Wednesday. (Obviously) Feel free to commit this, for now, though.

Commited to 7.x-1.x e2e2a44d

This change is incorporated in 7.x-1.3

Thanks Wim! Pushed to 7.x-1.x (503dae83). I'm about to cut 7.x-1.4.