Posted by RobLoach on October 28, 2011 at 11:40pm
4 followers
| Project: | Libraries API |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Statically cache libraries_detect()?
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| static.patch | 1.56 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 126 pass(es). | View details |
Comments
#1
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.
#2
+++ 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.
#3
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.
#4
The last submitted patch, 1325524-libraries-detect-static-2.patch, failed testing.
#5
That was weird, I messed something up big time, there. This should work.
#6
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.
#7
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.
#8
Here we go.
#9
Let's do some more magic and save some server memory here.
#10
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?
#11
Grr, meant RTBC.
#12
Holy crap.
#13
I'm down with that
#14
The last submitted patch, libraries.detect-cache.13.patch, failed testing.
#15
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.
#16
This one passes.
#17
+++ 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].
#18
Forgot about the ternary operator.
#19
Excellent... Anything holding off an alpha2? Maybe #1299076: Improve JS, CSS, PHP file testing?
#20
Good stuff. Thanks all!
Comitted to 7.x-2.x.
http://drupal.org/commitlog/commit/10030/256d7c10ca51032137d5b9350a51f17...
#21
Automatically closed -- issue fixed for 2 weeks with no activity.