Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Statically cache libraries_detect()?
Comment | File | Size | Author |
---|---|---|---|
#18 | libraries.detect-cache.18.patch | 3.72 KB | sun |
#16 | libraries.detect-cache.16.patch | 3.67 KB | tstoeckler |
#13 | libraries.detect-cache.13.patch | 3.67 KB | sun |
#9 | libraries.detect-cache.9.patch | 2.88 KB | sun |
#8 | 1325524-8-libraries-detect-static.patch | 2.53 KB | tstoeckler |
Comments
Comment #1
tstoecklerThis 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.
Comment #2
sunThis doesn't do what you think it would do.
Otherwise, I guess this looks fine to me.
Comment #3
tstoecklerRerolled 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.
Comment #5
tstoecklerThat was weird, I messed something up big time, there. This should work.
Comment #6
sunFor 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.
Comment #7
tstoecklerThat'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.
Comment #8
tstoecklerHere we go.
Comment #9
sunLet's do some more magic and save some server memory here.
Comment #10
RobLoachOh, I like that... As long as the tests pass, I'm saying this is RTBC.... Other than that, quick doc question:
Should there be a quick note here about if the library isn't defined, it'll return FALSE?
Comment #11
RobLoachGrr, meant RTBC.
Comment #12
RobLoachHoly crap.
Comment #13
sunI'm down with that
Comment #15
tstoecklerThat 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:
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.
Comment #16
tstoecklerThis one passes.
Comment #17
sunThis assigns a copy of $libraries[$name] to $library, so the returned $library is no longer a reference to $libraries[$name].
Comment #18
sunForgot about the ternary operator.
Comment #19
RobLoachExcellent... Anything holding off an alpha2? Maybe #1299076: Improve JS, CSS, PHP file testing?
Comment #20
tstoecklerGood stuff. Thanks all!
Comitted to 7.x-2.x.
http://drupal.org/commitlog/commit/10030/256d7c10ca51032137d5b9350a51f17...