Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Assigned: Unassigned » sun

This is RTBC from me, but sun has mentioned before that he wants to avoid "over-caching", so he should pull the trigger on this one.

sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work
+++ b/libraries.module
@@ -537,7 +532,7 @@ function libraries_detect($name) {
+  return $libraries[$name] = $library;

This doesn't do what you think it would do.

Otherwise, I guess this looks fine to me.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
8.98 KB

Rerolled the patch. While I'm not against the early-return-if-available pattern in the patch above, we use the build-it-if-not-available-and-return-at-the-end pattern in libraries_info(), so I thought we should have that consistent. Doing that I notices the early out returns we have in libraries_detect() for our various error conditions. I fixed that as well. Because I had to change the indentation for that, I attached a second patch for easier reviewing.

Status: Needs review » Needs work

The last submitted patch, 1325524-libraries-detect-static-2.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
8.98 KB

That was weird, I messed something up big time, there. This should work.

sun’s picture

For atomic functions that do exactly one thing (ideally every function), I really prefer the early-return pattern as in the patch in #0, because it avoids the entire-effin-function-indented-consequence. Personally, I only consider the condition/indentation approach if the function performs more than what is being statically cached; i.e., if another action is performed after reading or retrieving the cached value -- in most cases, that's poor code/design in the first place though.

tstoeckler’s picture

Status: Needs review » Needs work

That's funny. I had thought that you had said that to me at one point, but seeing libraries_info() was convinced otherwise. I'll reroll with the early-return style as in #0. I'll file a little follow-up to change libraries_info() accordingly.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.53 KB

Here we go.

sun’s picture

Let's do some more magic and save some server memory here.

RobLoach’s picture

Status: Needs review » Needs work

Oh, I like that... As long as the tests pass, I'm saying this is RTBC.... Other than that, quick doc question:

+++ b/libraries.moduleundefined
@@ -320,7 +320,7 @@ function libraries_detect_dependencies(&$library, $version = NULL, $variant = NU
- * @return
+ * @return array|false
  *   An associative array containing registered information for all libraries,
  *   or the registered information for the library specified by $name.

Should there be a quick note here about if the library isn't defined, it'll return FALSE?

RobLoach’s picture

Grr, meant RTBC.

RobLoach’s picture

Status: Needs work » Reviewed & tested by the community

Holy crap.

sun’s picture

I'm down with that

Status: Reviewed & tested by the community » Needs work

The last submitted patch, libraries.detect-cache.13.patch, failed testing.

tstoeckler’s picture

That looks pretty cool.
I think I'm now beginning to grasp the return-by-reference thing. I'm not sure if 'libraries_info' is the correct name then, though, if we re-use it in libraries_detect(). Haven't come up with a better one yet, though.
Also:

+++ b/libraries.module
@@ -364,7 +365,8 @@ function libraries_info($name = NULL) {
+    return !empty($libraries[$name]) ? $libraries[$name] : $false;

If I read the test exceptions correctly, $libraries[$name] should be put in a dedicated variable as well for the return-by-reference thing to work.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

This one passes.

sun’s picture

Status: Needs review » Needs work
+++ b/libraries.module
@@ -364,7 +365,8 @@ function libraries_info($name = NULL) {
+    $library = (!empty($libraries[$name]) ? $libraries[$name] : FALSE);
+    return $library;

This assigns a copy of $libraries[$name] to $library, so the returned $library is no longer a reference to $libraries[$name].

sun’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

Forgot about the ternary operator.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Excellent... Anything holding off an alpha2? Maybe #1299076: Improve JS, CSS, PHP file testing?

tstoeckler’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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