Statically cache libraries_detect()?

Files: 
CommentFileSizeAuthor
#18 libraries.detect-cache.18.patch3.72 KBsun
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#16 libraries.detect-cache.16.patch3.67 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#13 libraries.detect-cache.13.patch3.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] 128 pass(es), 0 fail(s), and 52 exception(es).
[ View ]
#9 libraries.detect-cache.9.patch2.88 KBsun
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database.
[ View ]
#8 1325524-8-libraries-detect-static.patch2.53 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#5 1325524-5-libraries-detect-static.patch8.98 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#5 1325524-5-libraries-detect-static-FOR-REVIEW.patch2.49 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#3 for-commit.patch8.98 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/libraries/libraries.module.
[ View ]
#3 for-review.patch2.58 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/libraries/libraries.module.
[ View ]
static.patch1.56 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 126 pass(es).
[ View ]

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
StatusFileSize
new2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/libraries/libraries.module.
[ View ]
new8.98 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/libraries/libraries.module.
[ View ]

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
StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
new8.98 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]

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
StatusFileSize
new2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]

Here we go.

sun’s picture

StatusFileSize
new2.88 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database.
[ View ]

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

StatusFileSize
new3.67 KB
FAILED: [[SimpleTest]]: [MySQL] 128 pass(es), 0 fail(s), and 52 exception(es).
[ View ]

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
StatusFileSize
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]

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
StatusFileSize
new3.72 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]

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.