Problem

* It appears that version arguments declared within variants are not taken into account when performing version detection within libraries_detect().
* The documentation indicates that top-level properties (such as 'version arguments' would be overridden.

Details

An example of the the hook_libraries_info definition is:

  $libraries['backbone'] = array(
    'name' => 'Backbone',
    'vendor url' => 'http://documentcloud.github.io/backbone/',
    'download url' => 'http://backbonejs.org/backbone.js',
    'files' => array(
      'js' => array(
        'backbone.js',
      ),
    ),
    'version arguments' => array(
      'file' => 'backbone.js',
      'pattern' => '/Backbone.js\s*([\d\.]+)/',
    ),
    'versions' => array(
      '1.0.0' => array(
        'variants' => array(
          'source' => array(
            'files' => array(
              'js' => array(
                'backbone.js',
              ),
            ),
          ),
          'minified' => array(
            'files' => array(
              'js' => array(
                'backbone-min.js',
              ),
            ),
            'version arguments' => array(
              'file' => 'backbone-min.js',
              'pattern' => '/VERSION="([\d\.]+)"/',
            ),
          ),
        ),
      ),
    ),
    'dependencies' => array('underscore (>=1.4.4)'),
  );

Comments

eshta’s picture

Issue summary: View changes
eshta’s picture

Issue summary: View changes
tstoeckler’s picture

Title: Variant-specific version arguments not tested » Support version arguments differing by variant
Version: 7.x-2.2 » 7.x-3.x-dev

Yeah, so the current API simply does not allow for this currently.

What you could do is provide a custom version callback that checks which file is available and then performs the check on the correct file.

We do eventually want to support this, but it will almost certainly be an API change, so moving to 7.x-3.x ad re-titling a bit.

coredumperror’s picture

+1 for this issue. I just flailed around for a few hours trying to make this work for a library who's main filename changed from one version to the next, so I needed a versioned "version arguments" array. Debugging into the code showed that the version detection is only done before the config overrides are applied from the "versions" setting. This obviously makes sense: how else could it know which of the "versions" to use? But it would be really nice if the version detection could at least take a glance at the "versions" setting if it fails to detect the library using the default settings, and loop through the versions until it finds one that it can detect.

I could write up a patch that would make it do this, if it would make it into a 2.x release for Drupal 7 (I have no interest in Drupal 8, as my code shop won't be migrating our D7 sites to D8).

tstoeckler’s picture

Version: 7.x-3.x-dev » 7.x-2.x-dev

I thought about this and I think it makes sense. I think we can simply add another version callback that implements the behavior described in #4 and then we could even do this in 7.x-2.x so moving back for that.

tstoeckler’s picture

Status: Active » Needs review
StatusFileSize
new2.15 KB

Something like this I think should be enough.

Needs docs and tests, but maybe someone can give this a test run.

coredumperror’s picture

I'm afraid I'm not sure what to do with the two new functions you've added in this patch. Where would one call libraries_get_version_from_versions() in order to make use of it for the purpose discussed in this issue?

Also, as a note for anyone who might also want to test this patch, be sure to update to the latest dev release before you apply it. That'll make the patch actually work, and it will also add the wonderful new "admin/reports/libraries" page, which is very helpful.

tstoeckler’s picture

Yeah, sorry for not providing any docs. Those functions can be used as version callbacks.
Something like this:

function MYMODULE_libraries_info() {
  $libraries['foo'] = array(
    ...
    // Activate version-dependent version detection.
    'version callback' => 'libraries_get_version_from_versions',
    ...
    'versions' => array(
      '1.0' => array(
        // For this version do regular version detection.
        // Note that 'libraries_get_version' has to specified here explicitly.
        'version callback' => 'libraries_get_version',
        'version arguments' => array(
          'pattern' => '/^Version (\d+)$/',
        ),
        ...
      ),
      ...      
    ),
  );

Something like that.
}

coredumperror’s picture

To make this work, I had to throw in some additional data, which should really be done by Libraries itself.

function date_ical_libraries_info() {
  $libraries['iCalcreator'] = array(
    'name' => 'iCalcreator',
    'vendor url' => 'http://github.com/iCalcreator/iCalcreator',
    'download url' => 'http://github.com/iCalcreator/iCalcreator',
    'version callback' => 'libraries_get_version_from_versions',
    'versions' => array(
      '2.20' => array(
        // For versions prior to 2.22, the main file of the library was
        // iCalcreator.class.php, so we check for that first.
        // Note that 'libraries_get_version' has to specified here explicitly.
        'version callback' => 'libraries_get_version',
        'library path' => libraries_get_path('iCalcreator'),
        'version arguments' => array(
          'file' => 'iCalcreator.class.php',
          'pattern' => "/define.*?ICALCREATOR_VERSION.*?([\d\.]+)/",
          'lines' => 100,
        ),
        'files' => array(
          'php' => array('iCalcreator.class.php'),
        ),
      ),
      '2.22+' => array(
        'version callback' => 'libraries_get_version',
        'library path' => libraries_get_path('iCalcreator'),
        'version arguments' => array(
          'file' => 'iCalcreator.php',
          'pattern' => "/define.*?ICALCREATOR_VERSION.*?([\d\.]+)/",
          'lines' => 100,
        ),
        'files' => array(
          'php' => array('iCalcreator.php'),
        ),
      ),
    ),
  );

  return $libraries;
}

The issue here is that "library path" key I needed to include in both version arrays. That normally gets constructed by the Libraries code automatically, and its assumed to be there by libraries_get_version().

So, to handle that problem and also allow for more DRYness, I'd like to propose this alternate patch from the one in #6. It adds $version_properties['library path'] = $library['library path']; to the loops in the new version callback functions, and also defaults $version_properties['version callback'] to 'libraries_get_version' if it isn't already set. This cuts 5 lines from my hook_libraries_info() implementation.

Status: Needs review » Needs work

The last submitted patch, 9: 2194023-9-version-detect-versions-variants.patch, failed testing.

coredumperror’s picture

StatusFileSize
new2.56 KB

Whoops, I'm an idiot. Should have known not to just edit your patch file. This one's created by git directly.

coredumperror’s picture

StatusFileSize
new2.81 KB

Shoot, found a problem. Using the new version detection code, if you have an older version of the library installed, the code will collect that older library's version number. But then it'll overwrite that with the detected version number of any newer library version, including ones that aren't installed. Thus, libraries_get_version_from_versions() ends up failing to detect the version number unless you have the newest version of the library installed.

Here's another updated patch that solves this problem. libraries_get_version_from_versions() and the variants one will no longer overwrite a detected version with a higher, undetected version.

With this patch in place, I can use these two functions:

/**
 * Implements hook_libraries_info().
 */
function date_ical_libraries_info() {
  $libraries['iCalcreator'] = array(
    'name' => 'iCalcreator',
    'vendor url' => 'http://github.com/iCalcreator/iCalcreator',
    'download url' => 'http://github.com/iCalcreator/iCalcreator',
    'version callback' => '_date_ical_detect_icalcreator_version',
    'versions' => array(
      '2.20' => array(
        // For versions prior to 2.22, the main file of the library was
        // iCalcreator.class.php, so we check for that first.
        'version arguments' => array(
          'file' => 'iCalcreator.class.php',
          'pattern' => "/define.*?ICALCREATOR_VERSION.*?([\d\.]+)/",
          'lines' => 100,
        ),
        'files' => array(
          'php' => array('iCalcreator.class.php'),
        ),
      ),
      '2.22+' => array(
        'version arguments' => array(
          'file' => 'iCalcreator.php',
          'pattern' => "/define.*?ICALCREATOR_VERSION.*?([\d\.]+)/",
          'lines' => 100,
        ),
        'files' => array(
          'php' => array('iCalcreator.php'),
        ),
      ),
    ),
  );

  return $libraries;
}

function _date_ical_detect_icalcreator_version($library) {
  // If the user has a version of libraries installed that has the
  // libraries_get_version_from_versions() function, use it to find the
  // actual version number by inspecting the iCalcreator files.
  if (function_exists('libraries_get_version_from_versions')) {
    return libraries_get_version_from_versions($library);
  }

  // Otherwise, provide a  "best guess" version number.
  $path = libraries_get_path('iCalcreator');
  if (file_exists(DRUPAL_ROOT . "/{$library['library path']}/iCalcreator.class.php")) {
    return '2.20';
  }
  else if (file_exists(DRUPAL_ROOT . "/{$library['library path']}/iCalcreator.php")) {
    return '2.22+';
  }
  else {
    return FALSE;
  }
}

...to make Date iCal capable of detecting the version number of iCalcreator regardless of either iCalcreator's version or Library API's version.

hansfn’s picture

Just adding a related/similar issue about versions. That issue contains a version callback function too.

rohit.rawat619’s picture

Assigned: Unassigned » rohit.rawat619
Status: Needs work » Active
rohit.rawat619’s picture

Assigned: rohit.rawat619 » Unassigned
Status: Active » Needs review
StatusFileSize
new24.9 KB