This issue will be a central place for code style / minor implementation problems discovered in other issues.

Patch coming soon.

CommentFileSizeAuthor
#86 958162-86-libraries-callbacks.patch14.41 KBtstoeckler
#84 958162-84-libraries-callbacks.patch13.43 KBtstoeckler
#81 libraries-defaults-and-pre.patch13.19 KBtstoeckler
#80 958162-80-libraries-callbacks.patch17.78 KBtstoeckler
#78 libraries.callbacks.78.patch25.06 KBtstoeckler
#77 libraries.callbacks.77.patch25.06 KBtstoeckler
#75 libraries.callbacks.75.patch24.95 KBtstoeckler
#72 libraries.callbacks.72.patch24.99 KBtstoeckler
#65 libraries.callbacks.65.patch14.53 KBsun
#63 libraries.callbacks.63.patch2.13 KBsun
#61 libraries-callback-groups-8.patch16.41 KBtstoeckler
#59 callback-groups-7_1.patch16.62 KBtstoeckler
#56 callback-groups-7.patch16.62 KBtstoeckler
#56 callback-groups-7.patch16.62 KBtstoeckler
#54 callback-groups-6.patch16.78 KBtstoeckler
#53 callback-groups-5.patch16.8 KBtstoeckler
#48 callback-groups-4.patch12.96 KBtstoeckler
#45 callback-groups-3.patch12.02 KBtstoeckler
#43 callback-groups-2.patch11.68 KBtstoeckler
#41 library_add.patch10.44 KBgood_man
#40 callback-groups.patch9.96 KBtstoeckler
#36 libraries-invoke.patch11.84 KBtstoeckler
#31 libraries-HEAD.prepare-files.31.patch8.84 KBsun
#30 libraries-get-version-cleanup.patch1.54 KBtstoeckler
#28 libraries-make-files-consistent.patch6.97 KBtstoeckler
#25 libraries-HEAD.cleanup.23.patch12.56 KBtstoeckler
#23 libraries-HEAD.cleanup.22.patch12.19 KBtstoeckler
#20 libraries-HEAD.machine-name.20.patch7.23 KBtstoeckler
#18 libraries-HEAD.machine-name.18.patch5.7 KBtstoeckler
#16 libraries-HEAD.machine-name.16.patch2.43 KBtstoeckler
#14 libraries-HEAD.machine-name.14.patch2.12 KBsun
#10 958162-clean-up-9.patch37.21 KBtstoeckler
#7 958162-clean-up-7.patch35.24 KBtstoeckler
#5 958162-clean-up-5.patch35.24 KBtstoeckler
#4 958162-clean-up-4.patch35.21 KBtstoeckler
#3 958162-clean-up-3.patch35.47 KBtstoeckler
#1 958162-clean-up-1.patch36.64 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Active » Needs review
FileSize
36.64 KB

Here we go.
This contains a bunch of changes related to #919632: Allow library information to be stored in info file because part of that was already committed.
#919632: Allow library information to be stored in info file is fixed after this is committed.

sun’s picture

Thanks!

+++ libraries.api.php	31 Oct 2010 12:02:01 -0000
@@ -345,6 +357,11 @@ function hook_libraries_info_alter(&$lib
+ * Note that placing library info files in module directories can result in
+ * problems, as Drupal thinks the info files belong to modules. Setting
+ * "hidden = TRUE" is recommended at the very least.
+ *
+ *

For now, we should leave this comment out, in light of #944198: Functions that call drupal_system_listing() act on potentially invalid system items

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -28,9 +28,7 @@ function libraries_get_path($library, $b
-    // 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;

While I think I understand your reasoning for changing this default return value, that's a major API change, since other modules may rely on the fact that we're always returning a path. I guess the change makes sense, but we need to discuss it a bit more carefully.

Background: This behavior got introduced, in order to remove knowledge about Libraries API internals from implementing modules, so they can merely do a file_exists() to check that a library actually exists, and if it does not, use the returned path to output a help text instructing the user where to install/place the library.

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -138,19 +136,17 @@ function libraries_info_files() {
   $directories[] = "libraries/$profile/libraries";

Looks like the first "libraries" should be "profiles" here?

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -138,19 +136,17 @@ function libraries_info_files() {
+      $files = array_merge($files, file_scan_directory($dir, '/[a-z[a-z0-9_]+.info/', array(

The first brackets in the regex are still not closed -- but not sure whether fixing this belongs into this issue/patch. Due to aforementioned core issue, we likely have to change .info file names either way, so revisiting this in the original or a separate issue sounds more reasonable.

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -187,18 +184,23 @@ function libraries_info($library = NULL)
+        'name' => $name,
         'title' => $name,

Hm - this looks like duplication? Are we facing an API change due to the .info file support (and since .info files commonly use "name" instead of "title" ?) -- looks like we should tackle this in the original .info file issue.

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -187,18 +184,23 @@ function libraries_info($library = NULL)
+        'library path' => FALSE,

What is the purpose of preemptively setting 'library path' to FALSE? Doesn't that break the necessary isset()/empty() logic elsewhere?

I mean, if the default is FALSE and libraries_get_path() can also return FALSE, how do we know whether we already tried to look up a library?

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -348,11 +352,24 @@ function libraries_detect_library(&$libr
+  libraries_load_files($library, $variant);
+  return TRUE;

It may be a good idea to return the result of libraries_load_files()?

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -364,15 +381,6 @@ function libraries_load($library, $varia
 function libraries_load_files($library, $variant = NULL) {

Do we still need the $variant argument here now?

+++ libraries.module	31 Oct 2010 12:02:01 -0000
@@ -383,6 +391,9 @@ function libraries_load_files($library, 
+  $path = (!empty($library['path']) ? $library['library path'] . '/' . $library['path'] : $library['library path']);

Since 'library path' is always used, it would be better to start with

$path = $library['library path'];

followed by an if condition that checks for the optional path:

if ($library['path'] !== '') {

Since 'path' defaults to an empty string, it's better to use the type-agnostic string comparison instead of empty() here. Who knows whether a library may need to set 'path' to '0'? :)

+++ tests/libraries.test	31 Oct 2010 12:02:01 -0000
@@ -125,29 +156,30 @@ class LibrariesTestCase extends DrupalWe
+    $this->assertLibraryFiles('example_2', 'Version overloading: ');

@@ -163,22 +195,27 @@ class LibrariesTestCase extends DrupalWe
+   * @param $prefix

1) $label would be a better parameter name.

2) Colon and space should be automatically appended to $label; i.e., not specified by the caller.

+++ tests/libraries_test.module	31 Oct 2010 12:02:02 -0000
@@ -226,6 +214,10 @@ function libraries_test_libraries_info()
+  ¶

@@ -346,27 +338,54 @@ function libraries_test_menu() {
+ * ¶

uh oh ;)

Powered by Dreditor.

tstoeckler’s picture

FileSize
35.47 KB

This fixes all mentioned issues, I moved all the stuff back to that was related to it back to #919632: Allow library information to be stored in info file. It seems I had missed a couple anyway (the regular expression for example). Will re-roll that after this one, hopefully.
For the return value of libraries_get_path() I opened #961476: Make libraries_get_path() return FALSE by default. You are right, that we shouldn't be doing it yet.
I now made libraries_load_files() return the number of loaded files, which was the first thing that came to mind, when you said return value. Don't know if you prefer anything else.

tstoeckler’s picture

FileSize
35.21 KB
+++ libraries.module 31 Oct 2010 12:02:01 -0000
@@ -187,18 +184,23 @@ function libraries_info($library = NULL)
+        'name' => $name,
         'title' => $name,

Hm - this looks like duplication? Are we facing an API change due to the .info file support (and since .info files commonly use "name" instead of "title" ?) -- looks like we should tackle this in the original .info file issue.

Hmm... well we currently use 'name' as machine name and 'title' as UI name. If we can, we should be consistent with core and use 'name' as UI name. The question is whether we actually need the 'name' property. While I can see it being useful, to access it, you already need to know the name, so if we remove it, we are at worst making it harder (not impossible) to access this property.

I rerolled with that change.
Note that I didn't change libraries_info_example.info for that, because in #919632: Allow library information to be stored in info file I am moving the file anyway and I wanted to avoid conflicts.

tstoeckler’s picture

FileSize
35.24 KB

Now with less notices. :)

I just tried this out and it's quite nice. It also fixes the tests (except the info file one, which is fixed at the info file issue).
I'll let you review it once more and would like to commit then, even if it's not all perfect. The patch file has gotten quite big again, and there's always the next commit...

sun’s picture

Thanks!

+++ libraries.module	3 Nov 2010 20:41:33 -0000
@@ -179,14 +179,15 @@ function libraries_info($library = NULL)
       foreach (module_invoke($module, 'libraries_info') as $name => $properties) {
         $properties['module'] = $module;
-        $properties['name'] = $name;
         $libraries[$name] = $properties;

Looks good for now, and it's definitely better to keep 'name' consistent with the rest of Drupal (core). If we need $name in a separate property at some point, we can add it back as 'machine_name', which would describe the value a bit more precisely + consistently with Drupal core.

+++ libraries.module	3 Nov 2010 20:41:33 -0000
@@ -195,10 +196,11 @@ function libraries_info($library = NULL)
+        'library path' => FALSE,

I'm still not convinced that default 'library path' to FALSE is a good idea. Since libraries_get_path() also returns FALSE, the default needs to be NULL resp. not set at all, so as to be able to figure out whether we invoked libraries_get_path() already - we only want to scan the filesystem once for a certain library. Am I wrong?

+++ libraries.module	3 Nov 2010 20:41:33 -0000
@@ -341,48 +345,60 @@ function libraries_detect_library(&$libr
  * @param $library
- *   The name of the library to load.
+ *   An array of library information as returned from libraries_info().
...
 function libraries_load($library, $variant = NULL) {
   $library = libraries_info($library);

This change seems wrong to me, since the first function line replaces $library with the library information. I think we should change the parameter/variable name to $name instead of $library, but keep the current logic; i.e., libraries_load() invokes libraries_info() on its own, not the caller. At least that would be consistent with other _load() functions in Drupal - you always pass an ID or name to load something.

+++ libraries.module	3 Nov 2010 20:41:33 -0000
@@ -341,48 +345,60 @@ function libraries_detect_library(&$libr
+  // Count the number of loaded files for the return value.
+  $count = 0;

@@ -419,9 +436,12 @@ function libraries_load_files($library, 
+  return $count;

OK, let's keep this count for now. I rather thought of an indication of whether the actual loading of files was successful (TRUE/FALSE), but since we're calling all kind of API functions here, I realize that it's going to be hard to provide this indication. So the file count might very well be the best we can do at this point. We can revisit and rethink later.

+++ tests/libraries_test.module	3 Nov 2010 20:41:34 -0000
@@ -226,6 +214,10 @@ function libraries_test_libraries_info()
+ ¶

:)

Powered by Dreditor.

tstoeckler’s picture

FileSize
35.24 KB

Here we go.
- default value of 'libraries path': I had thought that setting this to FALSE, simplifies the detection logic in the external files issue, but I just looked, and it seems that's really not true.
Fixed that and everything else you mentioned.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thank you!

+++ libraries.module	3 Nov 2010 21:34:11 -0000
@@ -340,7 +344,7 @@ function libraries_detect_library(&$libr
- * @param $library
+ * @param $name

@@ -348,41 +352,53 @@ function libraries_detect_library(&$libr
 function libraries_load($library, $variant = NULL) {
   $library = libraries_info($library);

Let's also rename the actual $library variable to $name prior to committing ;)

Powered by Dreditor.

tstoeckler’s picture

All right. Some last-minute touch-ups.
1) 'title' => 'name' required some changes in libraries_test.module (the test libraries) and libraries.module (the error messages)
2)I renamed the empty library from 'empty' to 'example_empty' to be more inline with the rest of the libraries. (Actually I just like the output of drush libraries-list better that way :) ).
3) I made the tests pass, which only required to update outdated information in libraries.test.

Attaching for a final Dreditor self-review before commit.

tstoeckler’s picture

FileSize
37.21 KB

Ahem.

tstoeckler’s picture

+++ tests/libraries.test	3 Nov 2010 22:08:00 -0000
@@ -163,22 +185,31 @@ class LibrariesTestCase extends DrupalWe
+   *   A label to prepend to the assertion messages, to make them less ambiguous.

Missing (optional)

Otherwise looked good to me. I just re-verified that
1) It passes tests.
2) Manual testing works
3) drush libraries-list gives the expected output.

Will commit this now (and fix the above pre-commit).

Powered by Dreditor.

tstoeckler’s picture

Status: Reviewed & tested by the community » Fixed

http://drupal.org/cvs?commit=446144
Marking 'fixed' (for now). Thanks for the awesome reviews!!!

sun’s picture

Status: Fixed » Needs work
function libraries_detect_library(&$library) {
  $library['installed'] = FALSE;
  $name = $library['name'];

Unfortunately, we need the 'machine_name' property, because we are actively using 'name' already. Looks like this is not covered by tests yet?

sun’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
tstoeckler’s picture

Well we actually don't ever use $name later in libraries_detect_library, that's why nothing broke.
Also:

+++ libraries.module	4 Nov 2010 01:13:01 -0000
@@ -185,6 +185,7 @@ function libraries_info($library = NULL)
+        $properties['machine_name'] = $name;

@@ -200,6 +201,7 @@ function libraries_info($library = NULL)
+        'machine_name' => $name,

I think the latter should be enough, no?

Powered by Dreditor.

tstoeckler’s picture

Rerolled. I also found one small bug in libraries.test which got introduced in one of the last patches here.

sun’s picture

Status: Needs review » Reviewed & tested by the community

yay, thanks!

tstoeckler’s picture

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

I just noticed that we were actually using $name in libraries_get_path($name), just our test libraries don't notice that because we specify 'library path' upfront...
http://drupal.org/cvs?commit=446430

Looking through libraries.module with the above patch I fond a bunch of inconsistencies in variable naming, which the attached patch tries to fix.
With this patch $name === $library['machine name']. In libraries_info() there were two $names involved: 1. The $name of the library whose info should be returned. 2. The names of the library that build of the $libraries array. I converted the second one to use $machine_name.
Also renames $name -> $variant_name in the appropriate places.

sun’s picture

Status: Needs review » Reviewed & tested by the community

woohoo! LOL -- looks like we overlooked quite some bogus code :-D Thanks, this makes a lot of sense!

tstoeckler’s picture

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

Was there any reason you went with 'machine_name' instead of 'machine name'?
Otherwise this is the patch we want.

sun’s picture

No opinion on that. machine_name merely referred to $machine_name, but I think that you're right in that our other properties don't use underscores either.

tstoeckler’s picture

Status: Needs review » Needs work

From #962290: Various implementation problems:
1. For all 'files'-alike properties, we want to standardize an original definition of
'js' => array('foo.js'),
into
'js' => array('foo.js' => array()),
before any other function is invoked.

2. Default version callback documentation, phpDoc, and example pattern is wrong. 'pattern' always needs to capture the entire version string in \1.

Regarding the above: Let's go with 'machine name' for consistency then.

tstoeckler’s picture

Well, I said the "make files declarations consistent" part would be easy.
That was pretty stupid.

tstoeckler’s picture

Now with a couple more/better comments in libraries_make_files_consistent() (better name plz!!!)

Also: splitting library_traverse_library() makes sense, as we'll probably re-use that in the external files issue.

tstoeckler’s picture

sun’s picture

Can we commit #20 upfront and limit the follow-up patch to the tasks listed in #22? :)

tstoeckler’s picture

Sure. I was thinking the same.
http://drupal.org/cvs?commit=449034
Will reroll in a minute.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
6.97 KB

Here is the 'files' one. I'll reroll the 'docs for libraries_get_version' one now.

tstoeckler’s picture

Well, that's what I found, don't know if you have more :)
Made the docs for 'file' verbose about not including 'path'.
Also added an early-out return if no 'pattern' was specified.

tstoeckler’s picture

Ahem.

sun’s picture

When adding new functions, always try to find the best and most logical position in the existing file. Flow:

Scan -> Info -> Prepare -> Detect -> Process -> Load (Render)

Not sure whether you're working with an IDE, but there are many people who are not (including me), and those have a much happier time to understand what's going on in some file, if related functions are grouped together.

What we just happen to introduce are preparation and processing phases for libraries. Most of what we do maps almost identically to existing patterns elsewhere; as arbitrary example, Form API:

Scan -> Build -> Prepare -> Alter -> #process -> #after_build [-> Validate -> Submit] -> Render.

Attached patch is _not_ complete, but merely outlines how to ensure that other modules can "hook" into our recursive library processing. Technically, this could also be done via regular hook names; i.e., hook_libraries_library_prepare(&$library, $section)

Thoughts?

tstoeckler’s picture

Mmmhh... I like that. A lot better than my patch!

hook_libraries_library_prepare() sounds even more awesome.

sun’s picture

I'm not sure which way would be better. The 'callbacks' approach has the benefit that libraries/modules can add dedicated callbacks for a certain library only (also via hook_libraries_alter()). A hook_libraries_library_prepare() + similar would imply that every hook implementation needs to check the passed in $library for whether it wants or needs to act on it. But then again, such kind of hooks are more common and usual in Drupal than the custom concept of a 'callbacks' stack to invoke.

tstoeckler’s picture

Coming back to this after a while, I'm trying to give a detailed overview of the situation.
Below is a conceptual overview of what currently happens when you load a library. For each step in the process I try to reason whether and how we should allow other modules to hook in.
This might a bit verbose and information overflow, but for me at least it helped to grok the bigger picture and, as you will see below, propose a solution.

The steps are:
1. Gathering of information (libraries_info())
2. Detection of libraries (libraries_detect() resp. libraries_detect_library())
3. Loading of libraries (libraries_load() resp. libraries_load_files())

0.9 Pre-info:

What happens: At this stage we gather places from where we get the library information. Currently we hardcode a hook ("hook_libraries_info()") and info files ("foo.libraries.info").
Whether to make this pluggable: Here we could allow other modules to define new ways in which they provide library information. If we (or some other module author) find(s) out there are ways to provide this information that we haven't thought of, though, we should implement this directly in Libraries API. In my opinion, we shouldn't make this pluggable.

1.1 Post-info:

What happens: At this stage the arrays provided by hook_libraries_info or in the libraries info files are consumed and gathered into the giant $libraries array. We currently use this to provide defaults for the top-level keys. And we just discovered that we need to make the files declarations consistent at this stage.
Whether to make this pluggable: Possible use-cases for other modules, that I thought of:

  • Modules that declare own top-level properties (e.g. WYSIWYG) might want to provide defaults for these properties.
  • If we ever find out how to discover the latest version of libraries programmatically, we might want to add a 'latest version' key for consumption via drush libraries-download
  • Just like we found out, that we need to traverse all versions and variants to make the files declarations consistent, other modules might need to the same.

So I think it makes sense to make this pluggable.

1.9 Pre-detection:

Since libraries_info() and libraries_detect_library() are usually called right after one another (at least neither are ideally called at runtime) this could arguably fall together with 1.1, I'm just seperating this for conceptual clarity.
What happens: Before the libraries array is passed to libraries_detect_library() for detection, we get a chance to alter the library and prepare it for detection. Currently we have no real use-case for this. In theory, we could do what currently the patch in the external files issue does in this stage, i.e. to set certain parameters depending on whether the library or certain variants are external. In general, this doesn't seem to be a generally useful use-case.
Whether we should make this pluggable: Because whatever should happen here could happen in 1.1 and because we don't have a real use-case for this now, I think we shouldn't do anything here, i.e. no pluggability.

2.1 Post-detection:

What happens: This is the first time that a library's version is known. Therefore, this might be a useful time for modules to do something. The version- and variant-dependent filenames issue could make use of this to replace version strings in filenames. That is arguable, though, because we only know the variant name upon loading and, hence, we need to replace the variant name in filenames in 2.9 anyway, so we might just do both (version string and variant name replacing) in 2.9. That is a discussion for that other issue, though.
Whether to make this pluggable: Because this, in comparison to 2.9, ideally doesn't run on most page views, you could make an argument for this, in case modules need to do some heavy processing on libraries once they know the version. In general I think this can't hurt. Because it is between the same larger steps we could only do 2.9, but, personally, I'd rather do this and 2.9 and if we find out that's overkill then rip out one of them, instead of later finding out that we do need it when we don't have it.

2.9 Pre-loading:

What happens: As stated above, this is needed for the version- and variant-dependent filenames issue, because this is the first time we know the variant that is to be loaded, so the first time we can replace the variant name in filenames. This can either be not cached at all, or cached per-variant (technically), but let's assume this is not cached for now.
Whether to make this pluggable: I think this will also be a valuable stage for modules (like WYSIWYG) who provide their own top-level properties, because they can at this stage rely on variant and version-information. On the other hand, the filenames thing is the only use-case we currently have, so hard-coding it wouldn't hurt technically. Personally, I'd rather err on the side of caution, though and make this pluggable.

3.1 Post-loading:

What happens: There's currently no use-case for this that we have come up with. The only hypothetical thing I could come up with would be something like a registry of loaded libraries that updates some static cache, whenever a library has been loaded. That is something we should be doing anyway, because we shouldn't ever be loading libraries twice, which we currently don't disallow. I guess that should be a separate issue.
Whether this should be made pluggable: Since there's no real use-case, I don't think this should be pluggable.

Summary: I would vote for 1.1 (Post-info), 2.1 (Post-detection) and 2.9 (Pre-loading) as pluggable.

How to make it pluggable

Since for all those stages, modules could require to traverse all versions (at least 1.1) and variants (at least 1.1 and 2.1) we should make this a generic function, like the libraries_traverse_library() above. Then we would allow modules to provide callbacks for each step in a hook and then call those callbacks while traversing the library. Because of the potential necessity of library traversion, I think using callbacks makes more sense than simply passing the library directly to a hook.
Something like the following would work:

foreach ('post_info', 'post_detect', 'pre_load') as $stage) {

/**
 * Declare callbacks to be applied to a library during the $stage stage.
 *
 * These callbacks are applied to all versions and variants of a library (in
 * case they have been declared). These callbacks should have the following
 * parameters:
 * - $library: An array of library information. This might be the top-level
 *   array, but also an array of version- or variant-specific information.
 * - $version: If the passed $library is version-specific information, the
 *   version string.
 * - $variant: If the passed $library is variant-specific information, the
 *   variant name.
 *
 * If your callback only needs to be applied to one of the three types of
 * library information arrays (top-level, version-specific, variant-specific)
 * you need to check the $version and $variant parameters accordingly in your
 * callback.
 */
hook_libraries_$stage() {
  return array('my_callback_1', 'my_callback_2');
}

}

And then we would have some generic function such as libraries_invoke() which would be more or less:

/**
 * Invokes library callbacks for a certain stage.
 *
 * @param $stage
 *   Either 'pre_info', 'post_detect' or 'pre_load'.
 * @param $library
 *   A full libraries array to pass.
 */
function libraries_invoke($stage, $library) {
  $callbacks = module_invoke_all('libraries_' . $stage);
  foreach ($callbacks as $callback) {
    libraries_traverse_library($library, $callback);
  }
}

If it's really that easy for us to call and provide such a hook, which I think it should be, then we should consider simply providing all of 0.9, 1.1, 1.9, 2.1, 2.9, 3.1 simply for completeness and because we do not know how Libraries API will be consumed. The fact that the very first serious consumer of our API made us rethink the whole thing in such a way says a lot, to my mind.

Thoughts?

P.S.: I posted this here, because we were already discussing this in here. Judging from the issue title, this would rather belong in #962290: Various implementation problems. We could also open a new issue for this. I don't care.

sun’s picture

Thanks for the summary!

One thing I'm missing here are the existing alter hooks -- they should basically map to all of the "post xyz" callbacks/stages, no?

Overall, however, it's kinda hard to evaluate the outlined options here as they are very abstracted. Perhaps it might make sense to try to limit the scope a bit...

In the meantime, for the remaining problem of this issue (the transformation/standardization of 'files'), I came to the conclusion that the libraries_prepare_library() callback _always_ needs to be invoked, and likely always needs to be invoked first; i.e., before any other callbacks.

So technically, for that purpose we wouldn't even need to introduce entire stacks of 'prepare' callbacks. Of course, we could *try* to come up with an extensible pattern, but it might be easier to tackle the handling of further callbacks in more dedicated issues (e.g., the others you mentioned already).

Thoughts?

tstoeckler’s picture

FileSize
11.84 KB

I was thinking something like this.

I don't quite get how those alter hooks you mentioned would work. All you are doing is altering the library, so an alter hook to alter the alteration would be overkill, right? Or do you mean that a module can alter which callbacks are being applied? That would be possible of course, but it I'm not really sure you meant that.

Pros of this approach: It's incredibly flexible and allows ourselves to do a lot of stuff without touching the main functions and also allows a lot of stuff in contrib.

Drawbacks: If you only want to something for a single library, say on pre_load(), the process is a bit cumbersome:
You have to implement hook_libraries_pre_load() but instead of altering the library right there, you have to write a callback and in that callback you have to check the name of the library and then you can alter it.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

I noticed I'm still assigned to this, but it's more of a team effort by now.

sun’s picture

Hm... I don't really like the level of abstraction of having to implement hooks to register callbacks to invoke.

If we go with the callbacks approach, then I'd rather go back to the approach in ~#33, whereas the flow would look like this:

  1. A module registers a library via hook_libraries_info(). The module registering the library might define callbacks ahead of time:
      $libraries['foo']['callbacks']['prepare'][] = 'mymodule_library_prepare';
    
  2. During collecting of libraries, Libraries API itself adds its own callbacks, and ensures that they are invoked first:
      $libraries['foo'] += array('callbacks' => array());
      $libraries['foo']['callbacks'] += array('prepare' => array(), 'detect' => array(), 'load' => array());
      array_unshift($libraries['foo']['callbacks']['prepare'], 'libraries_library_prepare_files');
    
  3. Before any callbacks are invoked, Libraries API invokes hook_libraries_library_alter() in all modules, to give modules a chance to alter the library as well as add or change callback definitions.
  4. At the end of each stage, Libraries API invokes libraries_traverse_library($library, $hook), whereas $hook is 'prepare', 'detect', or 'load'.

Overall, I think that would be a more sane approach.

--

However, as mentioned before, we could as well skip that entire registry of callbacks, and merely make libraries_traverse_library() and other functions invoke dedicated hooks per library and per stage; e.g.,

hook_libraries_library_STAGE_alter()
hook_libraries_library_LIBRARY_STAGE_alter()

Technically, the _alter suffix is superfluous given the STAGE already, but I'd consider to leverage D7's shiny new drupal_alter() feature:

  $hooks = array();
  $hooks[] = 'libraries_library_prepare';
  $hooks[] = "libraries_library_$name_prepare";
  drupal_alter($hooks, $library, $section [= NULL, 'versions', 'variants'], $full_library);

Meaning: If mymodule implements

mymodule_libraries_library_jquery_prepare_alter()

then that hook is invoked for the 'jquery' library only, and only for the 'prepare' stage. The only downside I can see with the alter hook approach is that jQuery module needs to implement an alter hook itself in order to correctly prepare/detect/load its own libraries.

tstoeckler’s picture

@#38.A. I agree that the double-level-abstraction is not so cool.
I also liked your approach with setting the callbacks right in hook_libraries_info().
I didn't go with that approach because I thought it would be impossible for a module to register a callback for all libraries. I just realized, though, that that would be as easy as:

mymoduley_libraries_info_alter($libraries) {
  foreach ($libraries as &$library) {
    $library['callbacks'][$group] = 'my_callback';
  }
}

I don't know what I was thinking.

So yes, let's go with that.
Two questions regarding that, though:
1. I don't see anything against it, but I also don't quite get why Libraries API must ensure its callbacks run first. I fear I am once again missing something essential.
2. I don't get step 3. Specifically: what would be the difference between hook_libraries_info_alter() and hook_libraries_library_alter()?

That last question basically also applies to all of #38.B: I don't get what the alter hooks are supposed to do.

I'll see if I can roll a patch for the first part of #38.A and how that turns out.

tstoeckler’s picture

FileSize
9.96 KB

Something like this.

A couple notes.
- I originally had $stage, you mentioned $group, so I went with that. I don't care.
- Instead of 'hardcoding' the 'libraries_prepare_files' into libraries_info() we could also implement hook_libraries_info_alter() ourselves and add the callback there. I personally think that's a bit of over-abstraction, but no strong feelings in case you disagree.
- In above patches, I made 'providing defaults' a callback itself, but because we add 'libraries_prepare_files' as a callback to $library['callbacks']['detect'], we need that to be initialized, so I just removed that.
- The libraries_test.module hunk is theoretically unrelated. I stumbled upon it when debugging this. The justification is the title of this issue :)

I tested it, and it seems it actually works in terms of the 'make files consistent' thing.

good_man’s picture

FileSize
10.44 KB

I'm trying to understand the whole approach from #34, lots of ideas and brainstorming, maybe need to reread it again later.

I have one question re. libraries_load_files(), now the $library param. is consistent. e.g. from comments:

$library['files']['js'] = array(
  'example_1.js' => array(),
  'example_2.js' => array(),
);

and libraries_load_files() will load the library by calling the appropriate function (e.g. drupal_add_js()). but as I can see in drupal_add_js() the paramaters must be in form ($data, $options), where $data is the fullpath or relative path to the file, and options to determine whether it's file (default) and also group number. but the libraries_load_files() is passing the params to drupal_add_js() as follow:

$data = jquery.js, // library name
$options = array(
  'data' => 'jquery-1.4.4.min.js', // only file name not path + file
  'group' => 0,
)

should it be $library['library path'] . '/' . $options['data'] in place of first $data param? look at line #253 -> #275 in the attached patch (I've tested it, and it works) (based on latest patch #40
).

tstoeckler’s picture

@good_man: You're right we have to change the logic in libraries_load_files(). Thanks for pointing that out, I hadn't noticed that. I think we will have to change the entire logic, though, more than just adding that line in your patch. #40 is still needs review for the general approach. Although tests would be really great here, to see if it actually works the way we want it to, I'm not sure, I'll get to writing some before Christmas/New Year's.

tstoeckler’s picture

FileSize
11.68 KB

New patch. It cleans up the bit in libraries_load_files(). All of the special-casing that we inherited from drupal_process_attached() basically comes from the need to support 'inline' CSS/JS and JS settings.
I pondered about this a bit and am pretty sure we don't want to support either of those. That would allow us to simplify that whole chunk of code, as in the attached patch.

tstoeckler’s picture

Note that that is also, technically, not necessarily part of this patch, but I had to touch it anyway.
It's probably about time, we change the title of this to "Allow groups of callbacks to be applied to libraries" and open a real clean-up issue.

tstoeckler’s picture

Title: The clean-up issue » Allow groups of callbacks to be applied to libraries
FileSize
12.02 KB

New patch. This is as self-contained as it gets. It comes with basic tests, that verify that the callbacks are applied correctly. It does not test the whole variant and version traversing, yet.
I will move the 'make-files-declarations-consistent' thing to another issue and open a new 'clean-up' issue for various code-style things either later today or tomorrow.

tstoeckler’s picture

+++ tests/libraries.test	8 Jan 2011 20:17:58 -0000
@@ -151,6 +151,27 @@ class LibrariesTestCase extends DrupalWe
+    ¶

Yup. One per patch is a must for me...

Powered by Dreditor.

tstoeckler’s picture

Status: Needs review » Needs work
+++ libraries.module	8 Jan 2011 20:17:57 -0000
@@ -158,6 +158,62 @@ function libraries_scan_info_files() {
+  // Apply callback to versions.
...
+  // Apply callback to variants.

Apply THE callback ...

+++ libraries.module	8 Jan 2011 20:17:57 -0000
@@ -212,11 +268,21 @@ function libraries_info($name = NULL) {
+    foreach ($libraries as $machine_name => &$properties) {

$machine_name is superfluous.

+++ tests/libraries_test.module	8 Jan 2011 20:17:58 -0000
@@ -215,6 +215,41 @@ function libraries_test_libraries_info()
+    'version callback' => '_libraries_test_return_version',
+    'version arguments' => array('1'),
...
+    'version callback' => '_libraries_test_return_version',
+    'version arguments' => array('1'),
...
+    'version callback' => '_libraries_test_return_version',
+    'version arguments' => array('1'),

Can just specify version upfront.

+++ tests/libraries_test.module	8 Jan 2011 20:17:58 -0000
@@ -301,6 +336,26 @@ function _libraries_test_return_installe
+  drupal_set_message('Callback applied!');

This line deserves a comment.

Will reroll now with that fixed, and with additional tests for the library traversing.

Powered by Dreditor.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
12.96 KB

Here we go. Assuming I didn't introduce any new whitespace (crossing my fingers...) this has my blessings.

tstoeckler’s picture

+++ tests/libraries_test.module	11 Jan 2011 18:10:11 -0000
@@ -301,6 +350,29 @@ function _libraries_test_return_installe
+ * Sets the 'callback applied' of a library to TRUE.

missing word: 'key'

+++ tests/libraries_test.module	11 Jan 2011 18:10:11 -0000
@@ -301,6 +350,29 @@ function _libraries_test_return_installe
+ * This function is used as test callback for all callback groups.

missing word: 'a' (as A test callback)

Powered by Dreditor.

sun’s picture

If you still think that these callback groups are better than module hooks, then let's do it!

One final remark:

+++ tests/libraries_test.module	11 Jan 2011 18:10:11 -0000
@@ -301,6 +350,29 @@ function _libraries_test_return_installe
+function _libraries_test_callback(&$library, $version, $variant) {

We should perform some more actual unit testing here; e.g.,

Was it invoked...
- for each callback group?
- for every version?
- for every variant?
- and for every variant in every version?

Powered by Dreditor.

EDIT:

- Are the changes of a callback in a previous callback group still contained?

tstoeckler’s picture

If you still think that these callback groups are better than module hooks, then let's do it!

Yup, great! :)

Was it invoked...
- for each callback group?
- for every version?
- for every variant?
- and for every variant in every version?

I don't quite get what you specifically want to test in addition to the added tests. The patch tests whether it was invoked in the right callback group (for each callback group) and it checks a version, a variant and a variant in a version for the callback.

- Are the changes of a callback in a previous callback group still contained?

Sure, we could test that.

sun’s picture

+++ tests/libraries_test.module	11 Jan 2011 18:10:11 -0000
@@ -301,6 +350,29 @@ function _libraries_test_return_installe
+function _libraries_test_callback(&$library, $version, $variant) {
+  $library['callback applied'] = TRUE;
+  // Because the 'load' callbacks are applied directly prior to loading, to
+  // check whether they were applied we need to actually load the library and
+  // can then check for the message below.
+  drupal_set_message('Callback applied!');
+}

Right now, the actual unit test that is performed here merely checks whether the callback has been invoked. That is, because none of the passed in parameters are used anywhere in the function body, and it is merely throwing a generic message.

To achieve the actual unit testing mentioned earlier, we probably have to do something along the lines of:

  $library['test callback applied'] = TRUE;
  $cache = variable_get(__FUNCTION__, array();
  $cache[LIBRARYNAME][CALLBACKGROUP] = $library;
  variable_set(__FUNCTION__, $cache);

As mayhaps visible in that code snippet, we likely need separate callback functions for each group that invoke a central one, in order to figure out the group:

function _libraries_test_prepare_callback(&$library, $version, $variant) {
  _libraries_test_callback($library, $version, $variant);
}

No idea what to do about LIBRARYNAME though -- perhaps not even required, as the callback is selectively applied to a certain library only?

If you have better ideas for unit testing this, please do so. Not really happy with my proposal here. ;)

Powered by Dreditor.

tstoeckler’s picture

FileSize
16.8 KB

How about this one?

The tests pass and they actually revealed two bugs in the previous patch.

tstoeckler’s picture

FileSize
16.78 KB

Here's a re-roll (didn't apply due to the clean-up issue) and an explanation on the approach.

This patch introduces a new test library (in libraries_test_libraries_info()) which registers a prepare, detect and load callback in the top-level library, in a version, in a variant, and in a variant inside a version (i.e. every possible place). Also, in all of those places the 'callback' key is set to FALSE.

The three callbacks (_test_libraries_prepare_callback(), _test_libraries_detect_callback(), and _test_libraries_load_callback()) simply call _test_libraries_callback() and specify their phase as an argument. _test_libraries_callback then figures out which part of the library was passed in and sets the 'callback' key to one of the following:

  • 'applied (top-level)'
  • 'applied (version x.y)'
  • 'applied (variant example)'
  • 'applied (version x.y, variant example)'

The tests then simply call libraries_info(), resp. libraries_detect_library(), resp. libraries_load() and check if the 'callback' key is set correctly. Note that the keys are sometimes non-trivial (i.e. after the detect phase 'applied (version x.y)' can be found in the top-level because the version-specific information was merged on to the top-level array) but that just proves that the whole thing works.

The bugs that were revealed are:
1. In case of a variant inside a version, instead of passing the version string we were passing the whole version-info-array
2. I can't really remember but it was something similarly trivial.

This has my blessings. Code-wise it is basically still #48, but now comes with some thorough tests.

tstoeckler’s picture

Status: Needs review » Needs work

A little clarification:
The test doesn't use one 'callback' property to see if the callbacks were applied, but a 'prepare callback', a 'detect callback', and a 'load callback' property, and checks each for the correct values in the correct places.

The confusion arises (as explained above) because after the detect phase the library in the top-level has the following keys:

'prepare callback' => 'applied (version 1)',
'detect callback' => 'applied (top-level)',

and that's actually correct.

I read through the entire patch and I'm pretty happy with it, except for the minor nitpicks below.
I'll reroll, and then set this to RTBC. I won't commit though, until you've given it another review, so no hastle.

+++ libraries.module	3 Feb 2011 16:25:37 -0000
@@ -145,6 +145,62 @@ function libraries_scan_info_files() {
+  if (!empty($library['callbacks'][$group])) {
+    foreach ($library['callbacks'][$group] as $callback) {
+      libraries_traverse_library($library, $callback);
+    }
+  }

Since the $group key is always initialized as an array, we might as well skip the !empty(), because the foreach will simply do nothing on an empty array.

+++ libraries.module	3 Feb 2011 16:25:37 -0000
@@ -145,6 +145,62 @@ function libraries_scan_info_files() {
+  if (!empty($library['versions'])) {
+    foreach ($library['versions'] as $version_string => &$version) {
...
+      }
+    }

See above

+++ libraries.module	3 Feb 2011 16:25:37 -0000
@@ -145,6 +145,62 @@ function libraries_scan_info_files() {
+  if (!empty($library['variants'])) {
+    foreach ($library['variants'] as $variant_name => &$variant) {
+      $callback($variant, NULL, $variant_name);
+    }
+  }

See above

+++ tests/libraries.test	3 Feb 2011 16:25:37 -0000
@@ -148,6 +148,74 @@ class LibrariesTestCase extends DrupalWe
+    $expected = array_merge(libraries_info('example_empty'), array(
...
+      'module' => 'libraries_test',

Specifying 'module' explicitly should not be needed, as 'example_empty' is specified by the same module.

+++ tests/libraries.test	3 Feb 2011 16:25:37 -0000
@@ -148,6 +148,74 @@ class LibrariesTestCase extends DrupalWe
+    // Test a callback in the 'load' phase.
...
+    $expected['variants']['example_variant']['load callback'] = 'applied (variant example_variant)';

The 'variants' key should be unset in the 'load' phase, shouldn't it? Hmm...

Powered by Dreditor.

tstoeckler’s picture

+++ libraries.module	3 Feb 2011 16:25:37 -0000
@@ -145,6 +145,62 @@ function libraries_scan_info_files() {
+  if (!empty($library['versions'])) {
+    foreach ($library['versions'] as $version_string => &$version) {
...
+      }
+    }

See above

+++ libraries.module	3 Feb 2011 16:25:37 -0000
@@ -145,6 +145,62 @@ function libraries_scan_info_files() {
+  if (!empty($library['variants'])) {
+    foreach ($library['variants'] as $variant_name => &$variant) {
+      $callback($variant, NULL, $variant_name);
+    }
+  }

See above

Well, both 'versions' and 'variants' can be unset, depending on the callback group. I changed the !empty() to a isset() to make that clear, and because it's faster.

Otherwise incorporated #55.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

I managed to leave whitespace out of this one, that must be a sign...

tstoeckler’s picture

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

Re-categorizing

tstoeckler’s picture

FileSize
16.62 KB

Re-uploading for testbot, I just saw the HEAD branch passes now, for whichever reason...

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
+++ libraries.module	8 Feb 2011 13:43:32 -0000
@@ -145,6 +145,60 @@ function libraries_scan_info_files() {
+      // Versions can include variants as well.
+      if (!empty($version['variants'])) {

For consistency, we might want to change this to isset() as well.

+++ tests/libraries.test	8 Feb 2011 13:43:32 -0000
@@ -148,6 +148,73 @@ class LibrariesTestCase extends DrupalWe
+    // Test a callback in the 'load' phase.

We could/should also test loading a specific variant.

Minor, though, so still needs review rather than need work.

Powered by Dreditor.

tstoeckler’s picture

Fixed #60

sun’s picture

Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to master.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

Status: Fixed » Needs review
FileSize
2.13 KB

I think I need this little follow-up tweak for jQuery module.

Status: Needs review » Needs work

The last submitted patch, libraries.callbacks.63.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
14.53 KB

ok, rather this. oyoyoy, the tests are a bit hard to grok ;)

sun’s picture

Though... are we (going to) caching the results of library_detect()?

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/libraries.api.php
@@ -118,9 +118,12 @@
+ *     - info: Callbacks registered in this group are applied as soon as the
...
+ *     - prepare: Callbacks registered in this group are applied when a library
...
  *     - detect: Callbacks registered in this group are applied as soon as

How about:
- info
- pre-detect
- post-detect
- load
?

+++ b/libraries.module
@@ -246,34 +246,15 @@ function libraries_info($name = NULL) {
+      libraries_info_defaults($properties, $machine_name);

Yes, I like that. Makes the tests more readable, definitely.

+++ b/libraries.module
@@ -284,6 +265,41 @@ function libraries_info($name = NULL) {
+  $library += array(
...
+    'callbacks' => array(),
+  );
+  $library['callbacks'] += array(
+    'info' => array(),
+    'prepare' => array(),
+    'detect' => array(),
+    'load' => array(),
+  );

Why not specify the 'callbacks' array right up front?

+++ b/tests/libraries.test
@@ -159,57 +160,82 @@ class LibrariesTestCase extends DrupalWebTestCase {
+    // Successfully detected libraries should only contain version information
+    // for the detected version and thus, be marked as installed.

..and below.
Yes, code comments do make sense here. (Sorry!)

+++ b/tests/libraries.test
@@ -159,57 +160,82 @@ class LibrariesTestCase extends DrupalWebTestCase {
+    $this->verbose('Expected:<pre>' . var_export($expected, TRUE) . '</pre>');

Yes I like that. I already have a half-finished patch on my box which does that (+ksort()) consistently, but I'm not necessarily against including it here also.

Though... are we (going to) caching the results of library_detect()?

Yes, I think I'm able to make sense of static caching by now, and I think it really makes sense for libraries_detect().

Setting to needs work, but please do commit this for your work on jquery.module. I'll just post a followup patch in that case.

Powered by Dreditor.

sun’s picture

Wasn't able to make too much progress on jquery.module yet.

- info
- pre-detect
- post-detect
- load

yeah, makes more sense. Though not sure whether we should go with hyphens, underscores, or spaces. Elsewhere, we use spaces...

These detailed questions are kinda stupid, but in the end, they make the difference between intuitive and ugly APIs.

Why not specify the 'callbacks' array right up front?

The += operator only affects the first level of array keys; doesn't work recursively. If a library definition specifies a callback in one of the groups, then the top-level 'callbacks' key and the manually defined group is set, but none of the other groups, because the keys already existed.

I think I'm able to make sense of static caching by now, and I think it really makes sense for libraries_detect().

I rather meant database caching though. In jQuery module, we need the library path to manipulate the entire library definition in order to change the 'files' keys. This operation requires a filesystem scan to check for the available jquery-x.x.x[.variant].js files. The library path is not available in the 'info' callback yet (and shouldn't be), it's only at the 'pre detect' time. libraries_get_path() isn't cached either (and could happily use a static (not drupal_static) cache). I don't really know how to proceed with jquery.module's case. Perhaps it's wrong to cache the info only, perhaps the library path assignment needs to be moved to library_info(), I don't know. Any ideas?

tstoeckler’s picture

Though not sure whether we should go with hyphens, underscores, or spaces. Elsewhere, we use spaces...

For that reason I had spaces in mind first, as well, but I was unsure whether "pre detect" actually counts as a (specifically: 2) word(s). Don't really mind that, though.

The += operator only affects the first level of array keys; doesn't work recursively. ...

D'uh.......

In jQuery module, we need the library path to manipulate the entire library definition in order to change the 'files' keys. This operation requires a filesystem scan to check for the available jquery-x.x.x[.variant].js files.

For me the later part sounds like a 'variant callback', which checks the availability of the each variant. The variant callback can't change the libraries definition, hence, you are not able to change the 'files' keys, but you would if we were to pass the &$library by reference. On the other hand, I don't really know why you would need to change the files definitions. You can specify the files upfront in the variant definitions and then mark the variant selectively as installed in the variant callback.

Depending on what the week has to offer university-wise I might play a bit with the code in jquery.module and see whether that really works the way I think it should.

sun’s picture

To clarify jquery.module's situation:

  1. The vendor-supplied library file is in the form of jquery-1.5.2.min.js. The filename contains the version and variant.
  2. Since the filename is dynamic, the library info is not able to specify the actual file, and the version detection is not able to find the library file.
  3. jQuery module therefore needs to scan the filesystem to figure out whether the library exists, what the filename is, and which version and which variant it is. The results of that at least need to replace the 'files' definition in the library info.
  4. While jQuery module could implement a callback in the info group to scan the filesystem to figure this out, IMHO this would be technically and architecturally wrong, as it involves calling libraries_get_path(), doing a filesystem scan, etc. — the info phase is solely meant to deal with registered library information, and these kind of operations clearly belong to the library detection phase.
  5. But the current detect callback is invoked too late; jQuery module needs to act and manipulate the library info as soon as the library path has been (successfully) detected, but before Libraries API tries to detect the library version.

From a distance, one could say that our current system works well for libraries that always have the identical filenames. It falls short when it comes to libraries that have version numbers and variant names in their filenames.

tstoeckler’s picture

Okay that explains the reasoning for a pre(-)detect phase. I have the feeling that there is some better, more generalized way to enable this sort of thing, but that shouldn't keep us from committing this patch, IMO. There's always the next one :)

What this doesn't explain, though, is the need for database caching. We already cache the entries in libraries_load() (which calls libraries_detect() right before caching), and I don't see libraries_detect() being performance-critical when called on its own. Since this does some file-system checks and invokes a bunch of callbacks static caching makes sense here, I think. What do you think?

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
24.99 KB

Here's a reroll with a static cache for libraries_detect(). As I haven't quite understood your point about caching above, I'm not sure it's inline with what you have in mind, but I like it. :)

Otherwise no changes except 'pre detect' and 'post detect'.

Tests pass locally.

tstoeckler’s picture

+++ b/libraries.module
@@ -284,6 +265,41 @@ function libraries_info($name = NULL) {
+ *   The machine name of a library to return registered information
+ *   for, or FALSE if no library with the given name exists. If omitted,
+ *   information about all libraries is returned.

Only in case of a reroll: Wraps to early.

Powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch, libraries.callbacks.72.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
24.95 KB

Oh darn you distributed version control (forgot to git pull before diffing...).

Status: Needs review » Needs work

The last submitted patch, libraries.callbacks.75.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
25.06 KB

This one should work, the last one was a bit messed up. I re-ran the tests locally and they pass now.

tstoeckler’s picture

FileSize
25.06 KB

While I'm at it, here's one with better wrapping for libraries_info_defaults().

sun’s picture

Status: Needs review » Needs work
+++ b/libraries.module
@@ -282,6 +263,41 @@ function libraries_info($name = NULL) {
+ * @param $machine_name
+ *   The machine name of a library to return registered information for, or
+ *   FALSE if no library with the given name exists. If omitted, information
+ *   about all libraries is returned.
+ */
+function libraries_info_defaults(&$library, $machine_name) {

Looks like I copied the phpDoc from somewhere else; mostly invalid here.

+++ b/libraries.module
@@ -313,101 +329,113 @@ function libraries_info($name = NULL) {
+  $detected = &drupal_static(__FUNCTION__, array());

mmm, would you mind if we move the caching to a separate issue? I think we need to revisit the entire static/db caching - we need to prevent "overcaching".

Powered by Dreditor.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
17.78 KB

Here's a reroll which fixes #79.

tstoeckler’s picture

Here's a quick patch, with just the libraries_info_defaults() part and the < pre > part in the tests.
I'd like to commit this as is, for the rest of this patch to come more easily reviewable, and for the tests to become easier debuggable. I'll reroll a patch with the rest of the patch afterwards.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, let's do this. :)

tstoeckler’s picture

Status: Reviewed & tested by the community » Active
tstoeckler’s picture

Status: Active » Needs review
FileSize
13.43 KB

Well, here we go.
My internet connection is strange...

sun’s picture

Status: Needs review » Needs work
+++ b/libraries.api.php
@@ -118,10 +118,14 @@
  *     Valid callback groups are:
- *     - prepare: Callbacks registered in this group are applied as soon as the
+ *     - info: Callbacks registered in this group are applied as soon as the
  *       library information has been retrieved via hook_libraries_info() or
  *       info files.
- *     - detect: Callbacks registered in this group are applied as soon as
+ *     - pre detect: Callbacks registered in this group are applied when a
+ *       library is about to be detected, before the version callback is
+ *       invoked. At this point the following additional information is available:
+ *       - $library['library path']: The path on the file system to the library.
+ *     - post detect: Callbacks registered in this group are applied as soon as
  *       library detection has been completed. At this point the library
  *       contains the version-specific information, if specified, and following
  *       additional information is available:

I think we can clarify these descriptions a bit. Also happy to do that post-commit in a follow-up patch though.

info: "as soon as" => "after"

pre detect: "after the library path has been determined and before the version callback is invoked."

post detect: "after a library has been successfully detected."

+++ b/libraries.module
@@ -287,8 +287,9 @@ function libraries_info_defaults(&$library, $name) {
   $library['callbacks'] += array(
-    'prepare' => array(),
-    'detect' => array(),
+    'info' => array(),
+    'pre detect' => array(),
+    'post detect' => array(),
     'load' => array(),
   );

Now, returning to this issue after a while and seeing the new groups again, the spaces are looking a bit odd in this particular case -- I think your earlier proposal of treating them like actual words, i.e. "pre-detect" and "post-detect", might indeed look and feel a bit better and more natural.

+++ b/tests/libraries_test.module
@@ -322,45 +327,37 @@ function _libraries_test_return_installed($library, $name, $installed) {
+function _libraries_test_info_callback(&$library, ¶
+$version, $variant) {

Odd wrapping here.

Powered by Dreditor.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
14.41 KB

Here we go. I'm uploading this for a final Dreditor review and will commit this then. It passes tests locally, so I'm not waiting for all core tests to pass...

tstoeckler’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.