Load library dependencies [#962214]

I think this is one of the last missing pieces in our API feature-wise.

Libraries can depend on other libraries. The most obvious example is the load of jQuery libraries that of course depend on jQuery. Or JQuery UI where each component (is that what they call it?) depends on different subsets of the core library.

This patch introduces a new $library['dependencies'] property. There are two ways to specify this property:

// This doesn't care which variant is used.
$library['dependencies'] = array('jquery');
// This loads the 'minified' variant of jquery.
$library['dependencies'] = array('jquery' => 'minified');

Comes with tests.
I also added a 'Dependencies' column to the output of drush libraries-list.

Comment	File	Size	Author
#8	962214-5-libraries-dependencies.patch	11.54 KB	tstoeckler
#8
#5	962214-5-libraries-dependencies.patch	11.54 KB	tstoeckler
#5
#4	962214-4-libraries-dependencies.patch	5.02 KB	tstoeckler
#4
	libraries-dependencies.patch	10.88 KB	tstoeckler

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 15 January 2011 at 14:33

Status:

Needs review

» Postponed

In case we actually go with the proposed compatible declaration which supports both
'dependencies' => array($name)
and
'dependencies' => array($name => $variant)
we will want to make that consistent just like in #1023258: Make 'files' (and 'integration files') declarations consistent.
Hence, this is postponed on #958162: Allow groups of callbacks to be applied to libraries.

Comment #2

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 21 June 2011 at 05:20

Status:

Postponed

» Active

This is no longer postponed.

For reference: The above doesn't quite make sense. A library should function regardless of its variant, i.e. if you depend on a specific variant you're doing something wrong. What is relevant, though, is the version of the library, so we should use that.

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 21 June 2011 at 15:10

Some libraries do not break backwards-compatibility, so not all dependencies may require a minimum version dependency.
Speaking of minimum version dependency, it's quite possible that an earlier version of a library is only compatible with some maximum version of a dependency.
Variants may be registered with different names though, so it should be possible to at least state "hey, ideally this $variant, if it's there..."
Core's hook_library() does not support versions at all. So we cannot make our definitions similar to what core is doing.
However, core's module system supports to specify version dependencies in a way that might be fully compatible with what we need: Yuck, no idea where this is documented... http://api.drupal.org/api/drupal/includes--module.inc/function/_module_b...
Basically, it's:
```
dependencies[] = libraries (>=2.3 <3.0)
```

So, considering the PHP declaration first:

  'dependencies' => array(
    'superlibrary' => array('version' => '>=2.3 <3.0', 'variant' => 'minified'),
  ),

Ouch. Now I realize that dependencies are likely defined globally per library, so specifying a particular variant is more or less impossible.

Well, in that case, we could simply go ahead and take over what the module system does:

  'dependencies' => array('baselibrary', 'superlibrary (>=2.3 <3.0)'),
  ),

Comment #4

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 29 June 2011 at 23:14

Status:

Active

» Needs work

File	Size
962214-4-libraries-dependencies.patch	5.02 KB

Here's a patch to get things rolling. No tests yet, and I also didn't test it manually, so statistically, it's probably broken. But could probably use some thoughts on the general idea.

Comment #5

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 July 2011 at 05:42

Status:

Needs work

» Needs review

File	Size
962214-5-libraries-dependencies.patch	11.54 KB

Not true, the code above actually worked :).
Also, I'm beginning to really like our testing method (although LibrariesUnitTestCase appears in my dreams ever so often).

In short: Here's a patch with extensive tests. It (unit) tests the dependecy detection (mostly the version parsing) and the loading of dependencies separately. That's in some way duplicate, but they are different code paths (libraries_detect_dependencies() (better name?), and libraries_load()), so...
The actual code is almost (!!) identical to #4.

This is now actually "needs review".

Comment #6

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 July 2011 at 05:43

Oh, and a quick disclaimer:
It's so late here, that it's early, which probably means I went really short on code comments.

Comment #7

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 July 2011 at 06:34

Version:

7.x-1.x-dev

» 7.x-2.x-dev

Wrong branch for testing.

Comment #8

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 July 2011 at 06:35

File	Size
962214-5-libraries-dependencies.patch	11.54 KB

Re-uploading #5

Comment #9

sun

German

Karlsruhe

CreditAttribution: sun commented 1 July 2011 at 14:59

This looks very good. A test with a real world module would be lovely, but I'm not sure which module actively depends on multiple libraries that have a dependency between themselves on their own. (wow, that's mind-boggling ;)

Comment #10

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 July 2011 at 15:02

I guess we'll find out if this works for real when jquery.module finally gets done. But that still depends on at least #964986: Support version- and variant-dependent filenames, if not others.

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 27 July 2011 at 14:24

Issue tags:

+D7 stable release blocker

Comment #12

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 24 August 2011 at 21:16

Status:

Needs review

» Reviewed & tested by the community

+++ b/tests/libraries.test
@@ -50,6 +50,78 @@ class LibrariesTestCase extends DrupalWebTestCase {
+    $this->verbose("Actual:<br>" . $library['error message']);

Minor (!!!) nit: This should use single quotes.

I just looked through this patch and couldn't find anything (else) to complain about. Since it's been a while since I rolled this, I'm going to mark this RTBC. I'll give it a few days and then commit (with the small change above).

Comment #13

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 2 September 2011 at 22:59

Status:

Reviewed & tested by the community

» Needs work

+++ b/tests/libraries.test
@@ -50,6 +50,78 @@ class LibrariesTestCase extends DrupalWebTestCase {
+      'name' => 'Library',
...
+      'name' => 'Library',

Also super minor: This should probably be "Example" for consistency.

The time has come now. :)

Comment #14

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 2 September 2011 at 23:06

Status:

Needs work

» Fixed

Committed #5 to 7.x-2.x with the two changes mentioned above.
http://drupal.org/commitlog/commit/10030/c60cfc3e3769bc172c1961da2606ba0...

Comment #15

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 2 September 2011 at 23:22

Status:

Fixed

» Needs work

Just a note that I realized that somehow the hunk which added this to the output of "drush libraries-list" got lost. I think that's actual a valuable addition (could be persuaded, though), so re-opening.