Allow library information to be stored in info file

This looks pretty awesome already! :)

Some totally open thoughts that crossed my mind while reading the patch:

- _libraries_scan_directory() is cool. I wonder whether we should use an $options array like file_scan_directory() in D7. Also, given $options, it might make sense to limit the recursion to 2-3 directory levels? Right now, I think we're only interested in the first level even?

- I fear a high risk of abuse of hook_libraries_paths(), which might lead to more problems than it solves. Don't get me wrong - the API design, code, documentation, and idea is awesome! But in this case, I'm not sure whether it wouldn't be more safe to hard-code our testing exception. I may be mistaken, but I think that http://api.drupal.org/api/function/drupal_system_listing/7 cannot be altered for the same reason: standardization. It would be easy to add that hook back later, if we find out that it is needed.

- As I still have little ideas of where and how and by whom .info files will ultimately be defined, it's not easy to evaluate the currently proposed processing code. It seems to assume that .info files are placed within the library directory and use the directory name as basename; e.g. sites/all/libraries/jquery.ui/jquery.ui.info. I see that this might be one option, though I rather had thought of either

1) a central .info file repository (i.e., the library paths itself, e.g., sites/all/libraries/jquery.ui.info) or

2) an include directory in modules; whereas an include directory in Libraries API module itself might be likely to achieve a central "community repository" (e.g., sites/all/modules/libraries/repo/jquery.ui.info)

Perhaps we just need to allow everything... (D'oh. :-| )

But overall, again, this is some awesome work you did here! Let's share and discuss some more ideas :)

Re-reading my follow-up, I just realized that putting .info files within the library directory is a bad idea -- users will want to update those directories like they are updating modules; i.e., delete the entire directory and extract the new -- .info file: gone ;)

So I guess it boils down to supporting 1) and 2)...?

Sorry upfront for flooding this issue ;)

I guess it would make much sense if we'd scan for .info files first, and only afterwards scan for libraries. That is, because I think that an .info file definition should ideally look similar to this:

; $Id$
type = library
name = jQuery UI
path = jquery.ui/lib
...
variants[minified][path] = jquery.ui/lib/minified
...
versions[1.8][path] = jquery-ui/lib
versions[1.8][variants][minified][path] = jquery-ui/lib/minified

No? Yes? Bleh? :)

hahahah, nice way to force reviews! :-D

Mostly looks/looked good :)

+++ libraries.module	6 Oct 2010 22:24:51 -0000
@@ -113,6 +113,49 @@ function libraries_get_libraries() {
+function libraries_info_files() {

Can we rename this to something that contains "scan"? The idea is to follow Drupal core's pattern of similar functions.

+++ libraries.module	6 Oct 2010 22:24:51 -0000
@@ -113,6 +113,49 @@ function libraries_get_libraries() {
+  foreach ($directories as $dir) {
+    $files += file_scan_directory($dir, '/[a-z[a-z0-9_]+.info/', array(

Pattern error and therefore PCRE compilation failure here: Missing closing ]

Also:

2) . should be \.

3) Let's delimit expressions with @, if possible. The "standard" / most often gets in the way, because of directory paths and stuff.

4) I think there should be a file_exists($dir) before invoking file_scan_directory().

5) The files are added via +=, but the order of directories is actually reversed; i.e., more specific libraries come after less specific ones. We either need to reverse the directory list or change += into array_merge().

+++ libraries.module	6 Oct 2010 22:24:51 -0000
@@ -140,23 +183,31 @@ function libraries_info($library = NULL)
+    foreach (libraries_info_files() as $name => $path) {

Is libraries_info() cached? This scan is quite expensive.

2) The scan function should return proper file objects, not just the name/URI.

+++ tests/example/example.info	6 Oct 2010 22:24:51 -0000
@@ -0,0 +1,7 @@
+; $Id$
+
+; This is an example info file for a library used for testing purposes. It
+; contains no actual library information, but, since it is named example.info,
+; if Libraries API detects a library named 'example', we know that this file was
+; properly detected.

Shouldn't we test at least the title property here, or something? :)

Powered by Dreditor.

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -126,35 +126,38 @@ function libraries_get_libraries() {
+function libraries_scan_info_files() {

Thanks! :)

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -126,35 +126,38 @@ function libraries_get_libraries() {
+  $files = drupal_static(__FUNCTION__);
+
+  if (!isset($files)) {

Ok, let's leave the caching out for now -- I think there's a separate issue for that anyway.

Note that when doing any kind of static/db caching pattern, then we usually do the opposite; i.e., test isset($files) and early-return that, if it is set. Otherwise, build it from scratch. That pattern saves us from putting all kind of code into conditions.

So let's revert that here.

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -126,35 +126,38 @@ function libraries_get_libraries() {
+        $files = array_merge($files, file_scan_directory($dir, '@[a-z][a-z0-9_]+\.info@', array(

1) Why do we enforce a letter, btw? While I don't know of any, I think that a library name of "11fools" would be perfectly valid.

2) mmm, sorry, I also didn't realize that the pattern is not anchored in any way -- it should start with ^ and end with $, so we skip any temporarily commented out files, such as tinymce.info.orig

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -178,23 +181,17 @@ function libraries_info($library = NULL)
-    foreach (module_implements('libraries_info') as $module) {
-      foreach (module_invoke($module, 'libraries_info') as $name => $properties) {
-        $properties['module'] = $module;
-        $properties['name'] = $name;
-        $libraries[$name] = $properties;

The removed code assigns $module and $name to the library. I think we want to keep that.

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -178,23 +181,17 @@ function libraries_info($library = NULL)
+    $libraries = module_invoke_all('libraries_info');
+
     // Gather information from .info files.

It looks like .info files always win over module-defined libraries. Probably makes sense, but would be good to leave a code comment here.

+++ libraries.module	10 Oct 2010 00:52:24 -0000
@@ -178,23 +181,17 @@ function libraries_info($library = NULL)
+      $libraries[$name] = drupal_parse_info_file($path);

$path usually means a directory path; ideally rename to $filepath or $infofile.

Powered by Dreditor.

+++ libraries.module	10 Oct 2010 01:30:56 -0000
@@ -143,18 +143,18 @@ function libraries_info_files() {
+      $files = array_merge($files, file_scan_directory($dir, '@^[a-z0-9_]+\.info$@', array(

On third sight: ;)

. and - also need to be valid characters; e.g., jquery.ui.info or jquery-ui.info.

+++ libraries.module	10 Oct 2010 01:30:56 -0000
@@ -143,18 +143,18 @@ function libraries_info_files() {
+        'key' => 'name',
+       'recurse' => FALSE,
+      )));

Minor: Indentation hiccup here.

+++ libraries.module	10 Oct 2010 01:30:56 -0000
@@ -178,23 +178,19 @@ function libraries_info($library = NULL)
-    foreach (module_implements('libraries_info') as $module) {
-      foreach (module_invoke($module, 'libraries_info') as $name => $properties) {
-        $properties['module'] = $module;

I understand your idea, but then again, it seems like the 'module' property is the only way to figure out whether a library has been defined by a module or an .info file? :)

+++ libraries.module	10 Oct 2010 01:30:56 -0000
@@ -178,23 +178,19 @@ function libraries_info($library = NULL)
+    // This information precedes that from the hook_invokation.

mmm, how about:

".info files override module definitions."

?

Powered by Dreditor.

+++ libraries.module	10 Oct 2010 02:19:22 -0000
@@ -143,18 +143,17 @@ function libraries_info_files() {
+      $files = array_merge($files, file_scan_directory($dir, '@^[\.a-z0-9_-]+\.info$@', array(

Minor: A dot (.) does not need to be escaped in a character range (i.e., [])

+++ libraries.module	10 Oct 2010 02:19:22 -0000
@@ -178,23 +177,27 @@ function libraries_info($library = NULL)
+      $filepath = DRUPAL_ROOT . '/' . $file->uri;
+      $properties = drupal_parse_info_file($filepath);

Do we need $filepath elsewhere? If not, we don't have to define it, just pass that concatenated info file path.

+++ libraries.module	10 Oct 2010 02:19:22 -0000
@@ -178,23 +177,27 @@ function libraries_info($library = NULL)
+      $properties['info file'] = $filepath;

Thanks, that makes even more sense! :)

+++ libraries.module	10 Oct 2010 02:19:22 -0000
@@ -178,23 +177,27 @@ function libraries_info($library = NULL)
+debug($libraries['example_info_file']);

:)

+++ tests/libraries.test	10 Oct 2010 02:19:22 -0000
@@ -39,6 +40,7 @@ class LibrariesTestCase extends DrupalWe
+      'info file' => DRUPAL_ROOT . '/' . drupal_get_path('module', 'libraries_test') . '/example/example_info_file.info',

Btw, are we actually asserting elsewhere that 'module' is set?

Powered by Dreditor.

Very nice work! Does this patch need to wait for the clean-up patch? Either way, ready to fly :)