Problem

While individual JS and CSS files can already be external (@see #1023274: Document how to use 'inline' and 'settings' types for CSS/JS), for Libraries API to consider a library installed/existant/available there must be a corresponding subdirectory in the sites/all/libraries directory for the library.
This directory is not only completely worthless for libraries that are hosted externally, but it is also cumbersome (and a bit strange) to ask the user to create that directory each time and it sort of defeats the purpose of an external library in the first place.

Realizations

  1. Whether a library is externally or locally loaded does not constitute a variant. The file, for instance jquery.js, is exactly the same, only its location is different. The minified version, jquery.min.js, is a different variant, but just the same you can load that same file locally or externally.
  2. Especially for one-file JS libraries, it is often a matter of personal taste (network latency, vs. bandwidth, effort, etc.) whether a library is downloaded to the server or used externally. If it is available locally, that constitutes the administrator's decision, so we should prefer local availability, but also provide an easy fallback mechanism for the external location.

Proposed solution

Instead of hardcoding the libraries_get_path() call in libraries_detect(), provide a 'path callback' key for libraries, which defaults to libraries_get_path() and which populates the 'path' key. That way libraries can provide their own business logic for where they are located under which circumstances.
Simple external libraries simply populate the 'path' key upfront and no detection takes place. The specified path is assumed to exist.
To accomodate the use-case in 2. above, libraries_get_path() now takes an optional $fallback parameter. If that is specified, libraries_get_path() returns that instead of FALSE if the library isn't found locally.

API changes

  • function libraries_get_path($name, $base_path = NULL) => libraries_get_path($library, $options)
  • function libraries_get_libraries() => function libraries_get_directories()
  • $library['library path'] => $library['path']
  • New: $library['path callback'] = 'libraries_get_path'
  • New: $library['path arguments']
  • $library['path'] => $library['directory']
CommentFileSizeAuthor
#91 864376-91-libraries-external.patch23.41 KBSweetchuck
#81 864376-81-libraries-external.patch27.38 KBtstoeckler
#79 864376-78-libraries-external.patch28.72 KBtstoeckler
#77 864376-77-libraries-external.patch27.22 KBtstoeckler
#75 864376-75-libraries-external.patch19.95 KBtstoeckler
#71 864376-71-libraries-external.patch17.29 KBtstoeckler
#69 864376-69-libraries-external.patch17.29 KBtstoeckler
#67 864376-libraries-external.patch21.22 KBtstoeckler
#61 libraries-external-61.patch24.07 KBtstoeckler
#60 libraries-external-60.patch7.94 KBtstoeckler
#57 libraries-external-location-callback.patch11.56 KBtstoeckler
#54 864376-external-54.patch13.35 KBtstoeckler
#53 864376-external-53.patch13.37 KBtstoeckler
#52 864376-external-files-51.patch44.33 KBtstoeckler
#48 864376-external-files-48.patch40.5 KBtstoeckler
#46 864376-external-files-46.patch40.47 KBtstoeckler
#43 libraries-HEAD.external.43.patch15.11 KBsun
#39 864376-libraries-external-slim-32.patch15.8 KBtstoeckler
#36 864376-libraries-cleanup-36.patch17.71 KBtstoeckler
#33 864376-libraries-cleanup-33.patch17.22 KBtstoeckler
#32 864376-libraries-external-slim-32.patch15.8 KBtstoeckler
#32 864376-libraries-cleanup-32.patch12.61 KBtstoeckler
#29 libraries-cleanup.patch11.89 KBtstoeckler
#28 864376-libraries-external-slim.patch15.67 KBtstoeckler
#26 864376-libraries-external-26.patch24.18 KBtstoeckler
#20 864376-libraries-external-20.patch24.12 KBtstoeckler
#20 review-me.patch2.69 KBtstoeckler
#15 libraries-HEAD.external.15.patch9.96 KBtstoeckler
#14 libraries-HEAD.external.14.patch6.3 KBsun
#13 libraries-external_5.patch2.71 KBtstoeckler
#11 libraries-external_4.patch2.58 KBtstoeckler
#8 libraries-external.patch1.11 KBtstoeckler
#3 libraries-external_2.patch1.35 KBtstoeckler
libraries-external.patch1.35 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs work
sun’s picture

Status: Needs work » Active

I thought we basically forked this from drupal_process_attached(), which supports external files already? Just pass 'external' => TRUE in the options array:

$build['#attached']['js'][] = array(
  'file' => drupal_get_path('module', 'taxonomy') . '/taxonomy.js',
  'external' => TRUE,
);

Since we purposively used the same file definition structure as in D7, I don't think there is anything to do here? Just register a library or variant declaring 'external' => TRUE for every file?

tstoeckler’s picture

FileSize
1.35 KB

I didn't see any external file handling in drupal_process_attached, and I checked your example to prove that it doesn't work.

Uploading a new patch, because
($data == 'external') != ($data === 'external')

tstoeckler’s picture

Status: Active » Needs review

:)

sun’s picture

well, the actual handling of external files is of course contained in http://api.drupal.org/api/function/drupal_add_js/7 resp. ultimately http://api.drupal.org/api/function/drupal_get_js/7

tstoeckler’s picture

Status: Needs review » Active

Dammit. Those double parameters really confuse the hell out of me...
Forget the patches, the structure is the following:

$library['files']['js'] = array(
  'http://www.openlayers.org/api/OpenLayers.js' => array(
    'type' => 'external',
  ),
);

That leaves the question mentioned in the op, about the error detection, etc.

Next time I'll roll a patch to libraries.api.php which documents this notation.

sun’s picture

Next time I'll roll a patch to libraries.api.php which documents this notation.

Please don't :) All we need, if it's missing, is a @see to the corresponding Drupal core documentation for #attached. Instead, it might be a good idea to improve the core docs of http://api.drupal.org/api/function/drupal_process_attached/7, especially regarding examples.

But you're right, next to that @see, we probably want to skip testing external files.

tstoeckler’s picture

Status: Active » Needs work
FileSize
1.11 KB

I opened #880368: Document how to load external files with #attached for the core documentation issue.
I think the attached patch is what we should do. First of all it adds a check in libraries_load if the library was installed. I think we should do that in any case, and if we run into problems with libraries being not installed (like here) we should fix that. I think the check in itself makes sense.

Then I added a check in the libraries_detect_library() to not look for a library path if $library['external'] is TRUE. That of course is a very simple, yet incomplete solution. What if only certain versions are external? Or certain variants (I think this is the most important case)? Or only certain files? That means we need to decide whether the library is external "dynamically" based on a couple of factors. But those factors then depend on a successfully installed library, which depends on a version callback, which depends on a local file (I think)...

I think we should write some tests for this and then see in what way this makes sense and into what concrete problems we run.
Also I haven't (yet) documented the 'external' property, that would obviously have to be done (if we stick with it).

sun’s picture

Hm. Completely open considerations on this:

+++ libraries.module	12 Aug 2010 17:05:51 -0000
@@ -208,7 +208,8 @@ function libraries_detect_library(&$libr
+  // Don't check locally for external libraries.
+  if (!file_exists($library['library path']) && $library['external'] !== TRUE) {

1) The condition needs to be flipped; otherwise, file_exists() is invoked nevertheless. !$library['external'] would be sufficient.

2) Comments should either explain something noteworthy or just don't exist at all. ;)

3) 'external' needs to be documented in libraries.api.php, if it's going to be a public/declarative property. Since we are merging variant/version properties onto to the top-level, it would be possible to declare an entire variant as external. That might be useful, not sure.

4) Should 'external' be declarative or automatically detected? libraries_detect_library() should be able to identify external file paths/URLs via http://api.drupal.org/api/function/url_is_external/7

+++ libraries.module	12 Aug 2010 17:05:51 -0000
@@ -291,7 +292,9 @@ function libraries_detect_library(&$libr
+  if ($library['installed'] == TRUE) {

"== TRUE" can be skipped here, just $library['installed'] is the same.

Thoughts?

Powered by Dreditor.

tstoeckler’s picture

1), 2) sure.
3) Yes, will document 'external'.
4) Do you mean instead of people declaring 'external' => TRUE they would be declaring 'path' => 'http://example.com/foo/bar' and then we run a url_is_external() on that? The question is, whether each library which is available online has one single location or whether the files are scattered on multiple locations. Probably, though, it is reasonable to assume that there is such a single location. But then the question is, in how far 'download url' still makes sense. Maybe just make that an optional property? Also this method would not work with the example below.

Concerning versions and variants, I think it's important to allow certain variants to be external and others to not. With Google's high performance hosting of js libraries for example, I think it makes sense to give people the option to use that external variant (for lazy people) or to download it on their server and use the local one (for people interested in performance). For bonus points I think we should provide a mechanism to automatically use a certain variant if another is not installed (ie use the external one if the local one is not available), but I will open another issue for that, as that is fundamentally unrelated.
Anyway:
Different versions being external or not, is really not a problem. We need a valid version callback anyway so the override happens just like with anything else.
Different variants being external introduces a kind of paradox:
We don't know whether the files are local or remote, but to find out the version, we need to look at one of the files (which, needless to say, requires the knowledge of where the file is).
One solution, which the above patch kind of applies is to declare the 'external' property as soon as one of the variants is external. Then the version callback would/could contain the file_exists() check itself:

function mymodule_examplelibrary_get_version($library) {
  $path = libraries_get_path('examplelibrary');
  if (file_exists($path)) {
    // If there is a local version use that one.
    $options = array(
      'file' => 'README.txt',
      'pattern' => '/@version (\d+)\.(\d+)/',
    );
    return libraries_get_version($library, $options);
  }
  elseif (mymodule_determine_if_there_is_a_public_internet_connection()) {
    // Otherwise get the current available version online.
    return mymodule_examplelibrary_get_current_version();
  }
}

And the variant callbacks would be

// For the local variant.
file_exists(libraries_get_path('examplelibrary'));
// For the external variant.
mymodule_determine_if_there_is_a_public_internet_connection();

I'll roll a proper patch with documentation and examples soon. And maybe we do need some tests, to test that an external library can never be 'not found'.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Maybe this is all we can do for now. If you find some kind of wording for the external property, where the word 'external' doesn't wrap right below each other 3 times, go for it :)
Hmm.

Status: Needs review » Needs work

The last submitted patch, libraries-external_4.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

wow, that's nice!
This one should work.
(Also note that the last hunk is unrelated, but it doesn't make sense to have the same check ("Is it installed") spelled out differently in the same file).

sun’s picture

Just thinking out loudly. Keep on rocking, please :)

tstoeckler’s picture

I challenge to a game of patch-ping-pong :-)
I don't like duplicated code, so I just collapsed the two 'library_get_version's into one. I split the creation of the file path on two lines simply for cosmetic reasons.
Also added some tests, and an example library.
I get one fail, because I can't seem to open the external file (I can access it via a browser and allow_url_fopen is enabled), but wanted to post it anyway.
We'll see, if we end up having to use an 'external' property at all, if we figure out the file_exists() stuff properly (which is also causing the test to fail (I mitigated the other test fail exactly by specifying 'external' => TRUE)), but for now it can't hurt I guess.

Status: Needs review » Needs work

The last submitted patch, libraries-HEAD.external.15.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#15: libraries-HEAD.external.15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, libraries-HEAD.external.15.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
+++ libraries.api.php	26 Aug 2010 13:12:26 -0000
@@ -30,6 +34,9 @@
+ *     In case the library version is static, the 'version' property may also be
+ *     predefined (without corresponding 'version callback'), so as to not try
+ *     to detect the library's version.

I guess we should document the 'version' property separately as "(optional)", above/before the 'version callback', and clarify that no version callback will be invoked if it is set.

+++ libraries.api.php	26 Aug 2010 13:12:26 -0000
@@ -303,6 +310,43 @@ function hook_libraries_info() {
+    // This should determine which variant is available (the local or the
+    // remote one) with 'libraries_get_path', and then use
+    // 'libraries_get_version' conditionally with or without the 'external' flag
+    // to determine the version. Note that in case of the remote variant the
+    // library information that is passed to 'libraries_get_version' must be
+    // padded with the 'library path' property as it is defined below in the
+    // variant information of the local variant.
+    'version callback' => 'openlayers_get_version',

wow, that's quite a use-case ;) I'd like to see that function LOL... not sure whether we shouldn't rather document supported and built-in basics here + play with advanced auto-fallback use-cases afterwards only?

+++ libraries.api.php	26 Aug 2010 13:12:26 -0000
@@ -303,6 +310,43 @@ function hook_libraries_info() {
+      'local' => array(
+        'library path' => 'http://openlayers.org/api',

Variant name doesn't map to its location.

+++ libraries.api.php	26 Aug 2010 13:12:26 -0000
@@ -303,6 +310,43 @@ function hook_libraries_info() {
+            // This syntax makes drupal_process_attached() pass the 'external'
+            // property correctly.
+            'OpenLayers.js' => array(
+              'type' => 'external',
+            ),

That shouldn't be required? I'd assume that filenames starting with http:// should be auto-detected... is that a Drupal core bug?

+++ libraries.module	26 Aug 2010 13:12:26 -0000
@@ -291,6 +296,9 @@ function libraries_detect_library(&$libr
 function libraries_load($library, $variant = NULL) {
   $library = libraries_info($library);
   libraries_detect_library($library);
+  if (!$library['installed']) {
+    return $library['error'];
+  }
   libraries_load_files($library, $variant);
 }

I don't really like that this function basically returns TRUE this way in case of an error, and NULL in case of success.

I like that it returns something.

Do we need to return the error message? Do modules that are trying to load a library have to care for that? Perhaps yes. But I'd expect to test if (!libraries_load(...)) first, or similar...

Why do we detect during loading again? ah, I guess we'll change that in the caching issue.

+++ tests/libraries_test.module	26 Aug 2010 13:12:26 -0000
@@ -276,6 +276,26 @@ function libraries_test_libraries_info()
+  $libraries['example_external_version'] = array(
+    'external' => TRUE,
...
+    'version arguments' => array(
...
+      'external' => TRUE,
+    ),
+  );

Perhaps, it may be possible to automatically set the 'external' key in version arguments, if not already set. But then again, this patch just made my head explode. ;) I've to study our current code (flow) first.

Powered by Dreditor.

tstoeckler’s picture

Alright. Shiny new version. Disclaimer: A bunch of coding style fixes crept in there. I felt bad, so I attached a patch that only contains changes strictly related to loading of external files. It won't apply because I messed with the patch file, though.

  • I dropped the 'external' property entirely.
  • External libraries (=== libraries that are not NECCESSARILY on the server locally === libraries that have at least one external variant) specify 'library path' to opt-out of the detection with libraries_get_path(). As of now, the actual library path has no meaning whatsoever, but servers as a purpose to test allow_url_fopen. See the api.php example or the libraries_test one.
  • To determine whether the given library_path is actually valid, I introduced a new helper function libraries_file_exists(), which is basically like file_exists() except that for external paths it does a fopen() and returns TRUE if that succeeded. There's a little problem that, if fopen() fails, it spits out a warning, which I didn't manage to suppress.
  • In case there no 'version' was declared, the version callback is run for external and local libraries alike. We check with url_is_external() whether the given file is external.
  • libraries_load now returns TRUE on success and FALSE on failure. That way, a developer doesn't get the error message, but doing:
    $library = libraries_info('name');
    libraries_detect_library($library);
    return $library['error'];
    

    isn't that hard...

  • We load the files, again utilizing url_is_external(). drupal_add_js() and drupal_add_css() load the external files automatically. I could not get the PHP file to load even though I had allow_url_include on. The file would get included properly (at least that's what the return value of 'include $file' said), but doing a function_exists() on the function defined in that file, would always give FALSE. The test, which is commented out in the patch, though, fails exactly on that PHP file, with both allow_url_fopen and allow_url_include on.
    I also changed 'require_once' to 'include_once', to be nicer to people who have 'allow_url_include' disabled (...everybody?!).

In general I like this approach a lot, because 1. There's no extra work for people declaring external libraries, 2. there's no real internal seperation between external and local libraries. We only call url_is_external a couple times to get the path handling right, and utilize libraries_file_exists(), because file_exists() only works on local files. In general, though, there's not much code-overhead.

Thoughts? :)

tstoeckler’s picture

+++ libraries.module	7 Oct 2010 01:32:32 -0000
@@ -332,8 +339,8 @@ function libraries_load_files($library, 
+          // Prepend the library path to the file name, if it is not an external file.

Doesn't wrap. Posting here for next reroll.

Powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch, review-me.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
sun’s picture

+++ libraries.module	7 Oct 2010 01:32:32 -0000
@@ -408,3 +415,12 @@ function libraries_get_version($library,
+function libraries_file_exists($file) {
+  return (file_exists($file) || (url_is_external($file) && fopen($file, 'r')));
+}

Hmmm. fopen() will actually open + download the external file. :-|

Apparently, there's nothing in core we could take as an example. I think we blatantly need to *presume* that any external file simply exists.

I.e., changing that code into:

return !url_is_external($file) ? file_exists($file) : TRUE;

Powered by Dreditor.

sun’s picture

Even better:

return url_is_external($file) || file_exists($file);

tstoeckler’s picture

Rerolled for the above.
Got a strange bug locally, where the JS would load, but the CSS wouldn't.
No clue.

sun’s picture

mmm, trying to review, but the other changes are highly distracting -- can we quickly split them into a separate patch in any other issue? Also happy to simply temporarily mix that patch into this issue, and get back to the real changes for this issue afterwards.

tstoeckler’s picture

Here's a slimmed down version of the patch, which only contains changes related to external files.
Will upload a patch containing the other changes in a minute.

tstoeckler’s picture

FileSize
11.89 KB

And here's the clean-up patch. It's basically:
- code style
- use 'testing' profile (which works perfectly, no changes required for that)
- In the tests, test 'error' instead of 'error message'. That's basically the correct way of solving the test breakage, that was the result of the commit in #542940: [meta] Support for downloading libraries via Composer. That's also a pure aesthetical change, but it feels more correct that way.
- return TRUE or FALSE in libraries_load()
- allow hardcoded $library['version']
So basically all changes that are trivial and/or we have agreed upon. Maybe we can just commit this, in order to move ahead with the "real" patch.

Because the bot doesn't seem to like me recently, I checked and the tests pass locally.

Status: Needs review » Needs work

The last submitted patch, libraries-cleanup.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Sorry, had to do a quick commit to fix the testbot; http://drupal.org/cvs?commit=434526 - so the clean-up patch no longer applies.

+++ libraries.module	11 Oct 2010 13:25:53 -0000
@@ -359,11 +365,11 @@ function libraries_load($library, $varia
-  if (!empty($variant) && !empty($library['variants'][$variant]['installed'])) {
+  if (!empty($variant) && $library['variants'][$variant]['installed']) {

PHP notice here, if someone specifies a $variant that does not exist (e.g., 'foo').

+++ tests/libraries.test	11 Oct 2010 13:25:53 -0000
@@ -45,27 +46,17 @@ class LibrariesTestCase extends DrupalWe
-    $error = t('%library could not be found.', array(
-      '%library' => $library['title'],
-    ));
-    $this->assertEqual($library['error message'], $error, 'Non-existing library not found.');
+    $this->assertEqual($library['error'], 'not found', 'Non-existing library not found.');

I wonder whether we shouldn't keep the existing assertions to test both? A valid and proper error message is also our expectation.

The rest of the clean-up patch makes perfectly sense.

Powered by Dreditor.

tstoeckler’s picture

Both rerolled. Takes into account #31 (we now test both 'error' and 'error message').

Let's see if the bot likes me now. :)

tstoeckler’s picture

Actually, if we already have a cleanup patch, I'm going to move the addition of 'title's for the the example libraries here, too.
Coming from #919632: Allow library information to be stored in info file and basically also #542940: [meta] Support for downloading libraries via Composer.

tstoeckler’s picture

The fail is because 'example_external' specifies a hard-coded version, but we only support that in the cleanup patch.
The best way forward here, IMO, is to commit the clean-up patch. I'll let you review it once more, though.

sun’s picture

+++ libraries.module	15 Oct 2010 13:24:51 -0000
@@ -359,11 +365,11 @@ function libraries_load($library, $varia
-  if (!empty($variant) && !empty($library['variants'][$variant]['installed'])) {
+  if (!empty($variant) && !empty($library['variants'][$variant]) && $library['variants'][$variant]['installed']) {

This change looks odd to me -- the old line looks to be sufficient...?

Aside from that, I agree that the clean-up patch is ready to go.

Powered by Dreditor.

tstoeckler’s picture

I thought
!empty($array['not-exists']['foo']);
would throw warnings, but apparently it doesn't.
Now that #939174: Notices on the modules page (due to fake info file) is in, we can add a title to that library as well.
Rerolled.

sun’s picture

Status: Needs review » Reviewed & tested by the community
+++ tests/example/libraries_example.info	15 Oct 2010 14:32:59 -0000
@@ -3,6 +3,7 @@
 name = example_info_file
+title = Example info file

mmm, now I'm confused. All other .info files in Drupal use the "name" property for the human-readable name. I guess it would make sense to leave this particular change to a separate issue.

Aside from that, RTBC.

Powered by Dreditor.

tstoeckler’s picture

Well, there's a little grey area around that.
The above info file, specifies the two properties exactly how they are used later.
If you don't specify title, though, it will automatically be filled with what you put in name. To put a non alphanumeric value in name would probably break something, though.
Anyway, will commit this in a minute.

tstoeckler’s picture

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

http://drupal.org/cvs?commit=436540
Commited the clean-up patch in #36.
Re-uploading the external-files patch to see if it passes now.

sun’s picture

+++ libraries.api.php	15 Oct 2010 13:07:15 -0000
@@ -311,6 +323,34 @@ function hook_libraries_info() {
+    // This makes Libraries API not scan the filesystem for the library.
+    'library path' => 'http://openlayers.org/api/OpenLayers.js',
...
+    'variants' => array(
+      'local' => array(
+        'library path' => libraries_get_path('openlayers'),
+        'files' => array(
+          'js' => array('OpenLayers.js'),
+        ),
+      ),
+      'remote' => array(
+        'library path' => 'http://openlayers.org/api',
+        'files' => array(
+          'js' => array('OpenLayers.js'),
+        ),
+      ),
+    ),

+++ tests/libraries_test.module	15 Oct 2010 13:07:16 -0000
@@ -43,9 +47,34 @@ function libraries_test_libraries_info()
+    // External libraries specify a file as library path, not a directory.
+    'library path' => "$external_path/example/README.txt",

I don't really get this. :)

1) If a library has both a local and remote variant, then it should not declare 'library path' on the top-level, but instead, only for the remote variant.

2) The library path should always be the base path to the library files only, regardless of whether that path is relative, absolute, internal, or external.

Do I get this wrong?

Powered by Dreditor.

tstoeckler’s picture

1) You are correct, but there is no way to circumvent a file_exists(libraries_get_path()) otherwise. Which will fail for external libraries.

2) That is actually a remnant of when we (I) still tried to actually validate the accessibility of a file via fopen(). So that can/should be removed.

tstoeckler’s picture

I thought about 1) a bit, and we basically have a design flaw (I think). While variants like the one in #40 are basically equal to each other, our API currently privileges the top-level library declaration over any variants. Both in (path) detection and loading (in case the variant was not explicitly specified). I think that's a problem, which surfaces in 1).

An idea from the top of my head:
We check right up front in libraries_detect_libraries(), whether there are variants, and if there are, completely ignore top-level properties such as 'files', 'libraries path' etc. And if someone does libraries_load('foo') (e.g. without a variant) of a library that has a variant, we just check each variant in the order of declaration and use the first one.
Haven't thought much about implications, but it seems like it could be a good idea.

sun’s picture

mmm, the current alpha code for .info file support unnecessarily complicates this. Attached patch should get us out of the woods.

tstoeckler’s picture

Sorry, but could you explain, what the advantage of your patch is, and what problem it fixes? I don't seem to get it.

sun’s picture

oh, sorry, I was in a hurry last time I posted.

The primary purpose of the patch in #43 was to get the existing tests back working, and also to make them compatible with the new ones for external libraries. IIRC, I didn't change any actual functionality/behavior for external libraries since your last patch. It's probably a good idea to split some/most of the test changes into a separate issue (or back into the original .info file issue).

But aside from that, I think we really need a clear use-case and code example for the functionality. That is, because right now, I can only _assume_ (but actually not sure) that a library definition like the following will be the most common:

+++ tests/libraries_test.module	15 Oct 2010 17:49:27 -0000
@@ -47,6 +47,43 @@ function libraries_test_libraries_info()
+  $libraries['example_external_variant'] = array(
+    'version' => '2',
+    'files' => array(
+      'js' => array('example_1.js'),
+      'css' => array('example_1.css'),
+    ),
+    'variants' => array(
+      'local' => array(
+        'library path' => $local_path,
+      ),
+      'remote' => array(
+        'library path' => $remote_path,
+      ),
+    ),
+  );

That is, because I assume that:

1) Every library comes from a project somewhere. The primary usage is likely to download and use the library locally.

2) Only certain libraries additionally provide a remote "variant" that can be used without downloading. The library and everything else is still the same though.

However, at this point, we can only assume how this could work. It would probably be a good idea to start to write the bare essentials for http://drupal.org/project/jquery to figure this out. I.e., have an actual library that exists in multiple versions and is normally downloaded as local library, but _can_ also be loaded from an external/remote URL.

So by having an actual use-case and implementation scenario, I think it will get a lot easier to understand how externally hosted libraries are going to be used. As of now, I'd even consider it as possible that we might need to do larger enhancements or changes to the library definition, since the current overloading of 'variants' and 'versions' could turn out to be not usable for externally hosted library "variants". (Not even sure whether adding a "remote" variant into 'variants' is actually the right thing to do.)

btw, happy to grant you CVS access for the jQuery project, if you want to. :)

tstoeckler’s picture

Reroll. Explanation coming up in form of a Dreditor review.

tstoeckler’s picture

+++ libraries.api.php	26 Oct 2010 15:36:30 -0000
@@ -311,6 +323,29 @@ function hook_libraries_info() {
+    'files' => array(
+      'js' => array('OpenLayers.js'),
+    ),
+    'variants' => array(
+      'local' => array(
+        'library path' => libraries_get_path('openlayers'),
+      ),
+      'remote' => array(
+        'library path' => 'http://openlayers.org/api',
+      ),
+    ),

Regarding 'local' vs. 'remote' just differing in 'library path'. I had initially thought, OpenLayers is actually an example of where this doesn't work (and had subsequently removed this assumption from the API), but I guess it makes sense to keep it that way until someone complains. Your reasoning was quite correct.

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -28,9 +28,7 @@ function libraries_get_path($library, $b
   if (!isset($libraries[$library])) {
-    // Most often, external libraries can be shared across multiple sites, so
-    // we return sites/all/libraries as the default path.
-    $path .= 'sites/all/libraries/' . $library;
+    return FALSE;
   }

libraries_get_path() returning sites/all by default, might have been useful in the past, but not in the days of libraries_load(). Keeping the old behavior would have made the path detection a lot more complicated (see below).

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -131,26 +129,22 @@ function libraries_info_files() {
-  foreach ($files as &$file) {
-    $file = $file->uri;

I thought you wanted to return full file objects and was confused by your last patch. This is now retained. I don't really care, though.

<del>
+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -187,9 +181,10 @@ function libraries_info($library = NULL)
+      $libraries[$name] = $properties;      ¶
</del>

Oops. =)
EDIT: Fixed in #48

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -199,6 +194,7 @@ function libraries_info($library = NULL)
+        'library path' => FALSE,

This not only saves an isset(), but is necessary so that not setting 'library path' in the first place and doing:
$library['library path'] = libraries_get_path($name);
in case it isn't found, has the same result.

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -256,10 +252,23 @@ function libraries_detect_library(&$libr
+    foreach ($library['variants'] as $variant => $info) {
+      if (isset($info['library path']) && libraries_file_exists($info['library path'])) {
+        $found = TRUE;
+        break;
+      }
+    }

This is the last remaining quirky part of the API, I think. Later, in libraries_load() we obviously need 'library path'. But you could call libraries_load() with a variant that doesn't have a library path defined and that leads to notices (at best).

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -364,9 +373,6 @@ function libraries_load($library, $varia
-  // Construct the full path to the library for later use.
-  $path = (!empty($library['path']) ? $library['library path'] . '/' . $library['path'] : $library['library path']);
-
   // If a variant was specified, override the top-level properties with the
   // variant properties.
   if (!empty($variant) && !empty($library['variants'][$variant]['installed'])) {
@@ -383,6 +389,9 @@ function libraries_load_files($library, 

@@ -383,6 +389,9 @@ function libraries_load_files($library, 
     }
   }
 
+  // Construct the full path to the library for later use.
+  $path = (!empty($library['path']) ? $library['library path'] . '/' . $library['path'] : $library['library path']);
+

This is not an aesthetical moving of code. In case of example_external_variant the $path was being constructed before 'libraries path' even existed.

+++ libraries.module	26 Oct 2010 15:36:31 -0000
@@ -393,9 +402,15 @@ function libraries_load_files($library, 
+          // This is needed because drupal_add_css() does not detect external
+          // paths automatically. See http://drupal.org/node/953340
+          if (url_is_external("$path/$options")) {
+            $options = array('type' => 'external');
+          }

Yup.

+++ tests/libraries.test	26 Oct 2010 15:36:31 -0000
@@ -77,18 +77,21 @@ class LibrariesTestCase extends DrupalWe
+    $this->verbose(var_export($library, TRUE));

I didn't know $this->verbose before and added it to all the assertions. Don't know if you think that's too much, though.

+++ tests/libraries.test	26 Oct 2010 15:36:31 -0000
@@ -163,22 +200,27 @@ class LibrariesTestCase extends DrupalWe
+   * @param $prefix
+   *   A string to prepend to the assertion messages, to make them less ambiguous.

It was becoming more and more difficult to debug the wall of "$foo (not) found", and now the test results page is quite a bit nicer, IMO.

+++ tests/libraries_test.module	26 Oct 2010 15:36:32 -0000
@@ -38,7 +35,6 @@ function libraries_test_libraries_info()
-    // Never declare library path manually. It is detected automatically.

In the patches before I changed that to "Never declare library path manually (except for external libraries)." That was a bit much, I guess, but just leaving it there is wrong.

+++ tests/libraries_test.module	26 Oct 2010 15:36:32 -0000
@@ -227,6 +252,10 @@ function libraries_test_libraries_info()
+  // This library is used together with libraries_info() to be populated with
+  // the defaults.
+  $libraries['empty'] = array();
+

I found this to be very useful for the info file test. (Which broke when I added the 'libraries path' => FALSE, thing (see above))

+++ tests/libraries_test.module	26 Oct 2010 15:36:32 -0000
@@ -342,31 +371,72 @@ function libraries_test_menu() {
 function _libraries_test_load($library, $variant = NULL) {

I went a bit wild with the test page callback. I like the result a lot, it's a lot clearer than before, and helped me personally in debugging. I can understand, if you find it's a bit much, though.

Powered by Dreditor.

tstoeckler’s picture

Reroll for the whitespace above. No other changes.

tstoeckler’s picture

Just a note from myself:
I think this is pretty good to go at its current state. We have tests for a simple external library and for the local vs. remote thing, so pretty much everything that popped up here as a potential use-case.
There will probably be follow-up improvements, but the patch file is getting pretty big (now mine is of course a lot bigger, because of all the changes to the test files, but I mean the actual libraries.module changes), and we've basically agreed on 80% of the patch already and given all the approaches we tried and the all way this patch has come, I guess we can be pretty sure the approach isn't that bad.

sun’s picture

wow! :)

Reading through your review issues in #47, I actually hoped that all of those details and reasonings would be contained as inline comments in the code, as they pretty much explain important details that shouldn't live in this issue only. Some of them could even be copied 1:1, it seems.

It looks like most changes in the patch are clean-ups now, which are technically unrelated to this issue. I think it would be a good idea to split those into a separate issue + patch, so we can focus on the actual changes for external library files here. Happy to review that separately to get it in first.

tstoeckler’s picture

Reroll. Tried to do more inline comments. Now only loads variants that are actually installed. Also improved the detection of variants. The code flow might seem a bit weird to follow at first (improvements of course (!!!) welcome !), but before you stick your head in it too much, maybe you could just comment on the two assumptions I documented in code:

// We consider the variant not found if one of the following is true:
// 1. The library itself was not found and the variant did not specify a
// library path.
// 2. The variant specified a library path that could not be found.

// We consider the library found if one of the following is true:
// 1. 'library path' was specified in the top-level library or in one of the
// variants and resolves to an existing path.
// 2. libraries_get_path() finds the library in a libraries directory.

I thought about it for a while and came to the conclusion that that solves all possible use-cases. Could be off, though.

Regarding splitting the patch: Sure, but how? Maybe you could just go through it with Dreditor real quick and select the hunks that disturb you while reviewing. I just tried to split it, but saw no good 'line' to draw.

Regarding jquery.module: Thanks very much for the offer, but I have limited Drupal time at the moment and I'd rather mean it whole-heartedly when I say I (co-)maintain something. Also, I haven't quite understood the idea behind the whole thing, but I guess that will happen sooner or later.

tstoeckler’s picture

D.o killed my patch :(

tstoeckler’s picture

FileSize
13.37 KB

Rerolled after the great progress tonight.
Now if this isn't a nicely reviewable little patch :)
#51 still stands as the assumption the code makes.

tstoeckler’s picture

FileSize
13.35 KB

How about one that passes?

sun’s picture

Need to test this "live" to really make any sense of it + double-check whether our assumptions are actually right. For that sake, I've just initialized the jQuery Libraries CVS repository in http://drupalcode.org/viewvc/drupal/contributions/modules/jquery/ -- will work on an initial, basic implementation now.

+++ libraries.api.php	3 Nov 2010 22:56:29 -0000
@@ -323,6 +323,31 @@ function hook_libraries_info() {
+    'variants' => array(
+      'local' => array(
+        'library path' => libraries_get_path('openlayers'),
+      ),

A non-remote variant shouldn't specify a hard-coded library path, I think?

Powered by Dreditor.

tstoeckler’s picture

Status: Needs review » Needs work

From #962290: Various implementation problems (not quoted):
1. local vs. remote aren't actually different variants. I.e. you can have a local source variant and a remote source variant or a local minified variant and a remote minified variant. The variant is still the same in both cases. This applies to libraries whose local and external version are actually identical code-/filewise, which might or might not be all libraries.
2. We need to verify that the version callback doesn't mark any variants installed.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
11.56 KB

New patch.

I went the path of not making 'remote' and 'local' different variants, but simply different locations of the same library. For that reason I introduced a 'location callback' (and resp. 'location arguments') property for modules to determine under which conditions which location is to be used. This defaults to libraries_get_path() [Note: I changed that function quite a bit, including #961476: Make libraries_get_path() return FALSE by default. If we ever come close to a commit here, we'll have to think about the implications of that, but for now I went for the (IMO) cleanest API], which returns FALSE if it doesn't find the library locally. You can optionally specify a $remote_path, which it returns instead. That's how external-local-double libraries can achieve the local-preferred-but-remote-as-fallback quite easily. The whole thing let me kill all of the crazy logic of the previous patch (Phew!) and also reduced the number of times we were calling libraries_file_exists() to 2, so I simply inlined that.

No tests yet.

Setting to needs review, knowing though, that this is not of great importance in the current issue queue.

tstoeckler’s picture

+++ libraries.module	15 Dec 2010 22:30:19 -0000
@@ -264,14 +260,15 @@ function libraries_detect_library(&$libr
+    } ¶

One always manages to hide from my x-ray vision :)

Powered by Dreditor.

tstoeckler’s picture

Something that just came to mind: If we do want to go with this approach we might want to rename 'library path' to 'location' which makes sense as the return value of 'location callback'.

tstoeckler’s picture

FileSize
7.94 KB

A little updated patch. This makes the change from 'library path' to 'location'
I don't really like location. I'd much rather do 'path' and then 'path callback' (which would also fit with libraries_get_path() (which I renamed libraries_get_location() for now)), but then we need some other name for what is now 'path'.
I didn't change all invokations of 'library path' to 'location', this patch is merely what would change in libraries_detect_library(), which IMO is the most interesting part.

tstoeckler’s picture

FileSize
24.07 KB

Nah, 'location' isn't really nice.
I like this better:
BEFORE -> AFTER
'library path' -> 'path'
'path' -> 'sub path'

I don't think 'sub path' is really optimal, but I would really hate to have a 'path callback' that returns to the 'library path' key. Also cleaned up the patch a bit.

That said, still needs review on the overall concept. Mostly libraries_detect_library is the fun stuff.

* I added an example_test library and a corresponding page callback for testing purposes (no pun intended), but no code in libraries.test to actually test that, because it currently doesn't work.

tstoeckler’s picture

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

Re-categorizing.

sun’s picture

sher1’s picture

OK, so this has been a problem for me with CAS and I would love to see this be fixed (couldn't we just add in the include_path from get_include_path() ?) Anyway, I just added a symlink CAS to /usr/share/pear in my libraries directory, like this
ln -s /usr/share/pear CAS and it resulted in
CAS -> /usr/share/pear/
in my libraries directory.
Unfortunately this only works for one pear module.
Hope this helps someone, at least in the interim

tstoeckler’s picture

Issue summary: View changes

Provide an issue summary.

tstoeckler’s picture

I provided an issue summary for this.
Could be maybe more elaborate but it's a good start, I think.
I made two changes with respect to the last patch:
1. For consistency with all other kinds of callbacks, the path callback should really take $library and not $name.
2. 'sub path' never really made any sense, I went with 'directory'.
Both are debatable, of course, best thing is you just update the issue summary with your likes.
I will provide a rerolled patch with those changes tomorrow-ish.

Just a note: The patch above is a bit more unreadable than it would be in a perfect world, because it moves libraries_get_path() down inside of libraries.module, because it is now just a regular callback, just like libraries_get_version(). We might want to do just the move in a quick followup commit, to make the patch more reviewable. (Or better, libraries.libraries.inc anyone? :))

tstoeckler’s picture

Status: Needs review » Needs work

I guess needs work for now.

tstoeckler’s picture

Issue summary: View changes

Use code tags instead of php

tstoeckler’s picture

Title: Loading of external files » Loading of external libraries
Status: Needs work » Needs review
FileSize
21.22 KB

A couple of notes.
Two API changes relative to previous patches:
1. If you declare $library['path'] manually we don't call file_exists() on it anymore. You are expected to either know what you are doing or specifying an external URL, which we can't validate anyway.
2. The previous patch had (theoretical) support for external PHP files. I dropped that. I doesn't work anyway, and it's also complete nonsense.

tstoeckler’s picture

Oops, for some reason this contains the patch from the cache issue.

tstoeckler’s picture

This should be better.

tstoeckler’s picture

Issue summary: View changes

Modify API changes in accordance to the new (upcoming) patch.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/libraries.api.php
@@ -16,14 +16,35 @@
+ *     returns the full path to the library. If the library cannot be found, it
+ *     returns FALSE. The first argument is always $library, an array containing

If the library cannot be found, it *should* return FALSE.

+++ b/libraries.module
@@ -15,32 +15,31 @@ function libraries_flush_caches() {
+ *   An associative array containing with the following keys:

containing

+++ b/libraries.module
@@ -394,10 +394,18 @@ function libraries_detect($name) {
-  if ($library['library path'] === FALSE || !file_exists($library['library path'])) {
+  if ($library['path'] == FALSE) {

Oh, I think we had === in case '0' ever is a valid path. Don't know if we want to support that :)

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
17.29 KB

Fixed the above.
I left the '==', I think that's clearer actually.

Status: Needs review » Needs work
Issue tags: -D7 stable release blocker

The last submitted patch, 864376-71-libraries-external.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

#71: 864376-71-libraries-external.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D7 stable release blocker

The last submitted patch, 864376-71-libraries-external.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
19.95 KB

The dependencies commit, broke this patch. Rerolled.

(Also yeah for the test bot to return in ~3min!)

tstoeckler’s picture

+++ b/libraries.api.php
@@ -16,14 +16,35 @@
  *   - download url: The URL of a web page on which the library can be obtained.

@@ -377,6 +398,50 @@ function hook_libraries_info() {
+    // This library does not have a download url!

Hmm, 'download url' should be "(optional)" I guess?

+++ b/libraries.module
@@ -15,32 +15,31 @@ function libraries_flush_caches() {
+ * @param $options
+ *   An associative array with the following keys:
+ *   - fallback: An external URL that, if specified, is returned if the library
+ *     is not found locally.

The more I think about it, the more I dislike an $optoins param, which only contains one option. I went with this approach for consistency with libraries_get_version(), but I now think it makes more sense to simply do function libraries_get_path($library, $fallback)

This still badly needs review on the general approach, though.

tstoeckler’s picture

Rerolled for #76.
The patch contains two commits for easier review: The first one is the actual code changes, and the second one just moves libraries_get_path() and libraries_get_directories() down in the module file.
The second one also contains a little bug-fix, which shouldn't have happened. (Still learning that Git thing... :))

Status: Needs review » Needs work

The last submitted patch, 864376-77-libraries-external.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
28.72 KB

Hmmm.... something went wrong in libraries.api.php...
Git doesn't like me :(

Status: Needs review » Needs work

The last submitted patch, 864376-78-libraries-external.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
27.38 KB

Sorry, for the noise.
This one works. I swear. I hope. :)
This time the two-commit-thing should actually make the whole thing quite reviewable also. I hope.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I just reviewed this, and this is ReadyToBeCommitted from my point of view. Now that we have an alpha in 2.x, I think that would make sense.
I am of course still waiting on some architectural feedback, but I have come to the point, where I really like the approach. And we can always rip it out again or revamp it in a follow-up.
I'll definitely not commit anything tonight, though, after what I managed to mess up above :)

sun’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for your work on this, @tstoeckler!

Hm. I'm not 100% convinced of the approach yet.

- path
- path callback
- path arguments
- fallback
- directory

Renaming "path" to "[sub]directory" makes sense, as "path" commonly has a different meaning throughout Drupal (i.e., the relative path from DRUPAL_ROOT to a thing).

However, the other path* declarations are not really easy to understand. Even after reading the docs for them, it wasn't entirely clear to me how or when I'd have to or should or would want to specify them. Looking at the examples helped a bit, but I don't think I could explain them to someone else without looking at the code.

Especially the "fallback" (URL) as part of "path arguments" seems a bit abstract and complex. It looks like this fallback value would have to be adjusted somehow for external libraries available in different versions/variants?

Also, as mentioned right above, I'm not sure whether it is a good idea to mix two different concepts into a single "path" property -- i.e., a local relative path and a fully-qualified URI. Or is the latter supposed to be a "partial" URI in the sense of a "path"? I.e., will the [sub]directory and actual library filenames also be appended there?

Hm, this is tough.

--
Minor things:

- The $base_path argument was useful for modules working with front-end libraries (e.g., Wysiwyg), which almost never need the internal/local path to a library, but rather the URI to it. However, I can see how this argument might be confusing or inappropriate for libraries_get_path(), so it's probably legit to either drop it entirely or introduce a libraries_get_uri() helper function. Can be deferred.

- Let's defer the move of the two functions to a follow-up patch in this or a separate issue. ;)

RobLoach’s picture

Isn't this already possible since drupal_add_js() supports it?

$libraries['something'] = array(
  'files' => array(
    'js' => array(
      'something.js' => array(
        'type' => 'external',
        'data' => 'http://something.com/something.js',
      ),
    ),
  ),
);
tstoeckler’s picture

From the issue queue summary:

While individual JS and CSS files can already be external (@see #1023274: Decide on whether or not to support 'inline' and 'settings' types for CSS/JS), for Libraries API to consider a library installed/existant/available there must be a corresponding subdirectory in the sites/all/libraries directory for the library.
This directory is not only completely worthless for libraries that are hosted externally, but it is also cumbersome (and a bit strange) to ask the user to create that directory each time and it sort of defeats the purpose of an external library in the first place.

I hope that makes the purpose of this issue more clear.

sun’s picture

FYI: Interesting tidbit; the commit that introduced CDN support for jquery_update module:
http://drupalcode.org/project/jquery_update.git/blobdiff/8a3a09fe9cfd231...

tstoeckler’s picture

Marked #1564350: External library pathes not working due to file_exists() validations. duplicate. Will have to re-dig into this, but that might result in me moving the patch from there over here.

Pol’s picture

Indeed this patch could be a great addition for OpenLayers.

I'm being refactoring some parts of the module and I would really need that.

See: #1936036: Improve the library loading and use renderable array..

Thanks !

Mac_Weber’s picture

What's the current status?
Is there anyone working on it?

Just like @Pol, I have similar interest in getting it fixed for Mapping module

bforchhammer’s picture

Stumbled on this issue while discussing CDN support for the qtip_library module; see #2086325: Support CDN version. Would love to see this resolved! :-)

bforchhammer’s picture

Issue summary: View changes

Make the "Proposed solution" part a bit more elaborate.

Sweetchuck’s picture

Re-rolled patch.

I am not sure this is good direction.

Pol’s picture

I was working on integrating CDN support into Openlayers and I ended up creating a new module: Libraries API CDN.
You can use any external libraries from a CDN using it and it integrates itself with Libraries API.

Chris Matthews’s picture

Status: Needs review » Needs work
Issue tags: -D7 stable release blocker +Needs reroll

The 4 year old patch in #91 does not apply to the latest libraries 7.x-2.x-dev and if still applicable needs a reroll.