Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This issue will be a central place for code style / minor implementation problems discovered in other issues.
Patch coming soon.
Comment | File | Size | Author |
---|---|---|---|
#86 | 958162-86-libraries-callbacks.patch | 14.41 KB | tstoeckler |
#84 | 958162-84-libraries-callbacks.patch | 13.43 KB | tstoeckler |
#81 | libraries-defaults-and-pre.patch | 13.19 KB | tstoeckler |
#80 | 958162-80-libraries-callbacks.patch | 17.78 KB | tstoeckler |
#78 | libraries.callbacks.78.patch | 25.06 KB | tstoeckler |
Comments
Comment #1
tstoecklerHere 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.
Comment #2
sunThanks!
For now, we should leave this comment out, in light of #944198: Functions that call drupal_system_listing() act on potentially invalid system items
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.
Looks like the first "libraries" should be "profiles" here?
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.
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.
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?
It may be a good idea to return the result of libraries_load_files()?
Do we still need the $variant argument here now?
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'? :)
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.
uh oh ;)
Powered by Dreditor.
Comment #3
tstoecklerThis 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.
Comment #4
tstoecklerHmm... 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.
Comment #5
tstoecklerNow 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...
Comment #6
sunThanks!
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.
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?
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.
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.
:)
Powered by Dreditor.
Comment #7
tstoecklerHere 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.
Comment #8
sunAwesome, thank you!
Let's also rename the actual $library variable to $name prior to committing ;)
Powered by Dreditor.
Comment #9
tstoecklerAll 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.
Comment #10
tstoecklerAhem.
Comment #11
tstoecklerMissing (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.
Comment #12
tstoecklerhttp://drupal.org/cvs?commit=446144
Marking 'fixed' (for now). Thanks for the awesome reviews!!!
Comment #13
sunUnfortunately, we need the 'machine_name' property, because we are actively using 'name' already. Looks like this is not covered by tests yet?
Comment #14
sunComment #15
tstoecklerWell we actually don't ever use $name later in libraries_detect_library, that's why nothing broke.
Also:
I think the latter should be enough, no?
Powered by Dreditor.
Comment #16
tstoecklerRerolled. I also found one small bug in libraries.test which got introduced in one of the last patches here.
Comment #17
sunyay, thanks!
Comment #18
tstoecklerI 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$name
s 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.
Comment #19
sunwoohoo! LOL -- looks like we overlooked quite some bogus code :-D Thanks, this makes a lot of sense!
Comment #20
tstoecklerWas there any reason you went with 'machine_name' instead of 'machine name'?
Otherwise this is the patch we want.
Comment #21
sunNo 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.
Comment #22
tstoecklerFrom #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.
Comment #23
tstoecklerWell, I said the "make files declarations consistent" part would be easy.
That was pretty stupid.
Comment #24
tstoecklerNow 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.Comment #25
tstoecklerComment #26
sunCan we commit #20 upfront and limit the follow-up patch to the tasks listed in #22? :)
Comment #27
tstoecklerSure. I was thinking the same.
http://drupal.org/cvs?commit=449034
Will reroll in a minute.
Comment #28
tstoecklerHere is the 'files' one. I'll reroll the 'docs for libraries_get_version' one now.
Comment #29
tstoecklerWell, 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.
Comment #30
tstoecklerAhem.
Comment #31
sunWhen 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?
Comment #32
tstoecklerMmmhh... I like that. A lot better than my patch!
hook_libraries_library_prepare() sounds even more awesome.
Comment #33
sunI'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.
Comment #34
tstoecklerComing 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:
drush libraries-download
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:
And then we would have some generic function such as libraries_invoke() which would be more or less:
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.
Comment #35
sunThanks 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?
Comment #36
tstoecklerI 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.
Comment #37
tstoecklerI noticed I'm still assigned to this, but it's more of a team effort by now.
Comment #38
sunHm... 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:
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.,
Technically, the _alter suffix is superfluous given the STAGE already, but I'd consider to leverage D7's shiny new drupal_alter() feature:
Meaning: If mymodule implements
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.
Comment #39
tstoeckler@#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:
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()
andhook_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.
Comment #40
tstoecklerSomething 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.
Comment #41
good_man CreditAttribution: good_man commentedI'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:
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:
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).
Comment #42
tstoeckler@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.
Comment #43
tstoecklerNew 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.
Comment #44
tstoecklerNote 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.
Comment #45
tstoecklerNew 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.
Comment #46
tstoecklerYup. One per patch is a must for me...
Powered by Dreditor.
Comment #47
tstoecklerApply THE callback ...
$machine_name is superfluous.
Can just specify version upfront.
This line deserves a comment.
Will reroll now with that fixed, and with additional tests for the library traversing.
Powered by Dreditor.
Comment #48
tstoecklerHere we go. Assuming I didn't introduce any new whitespace (crossing my fingers...) this has my blessings.
Comment #49
tstoecklermissing word: 'key'
missing word: 'a' (as A test callback)
Powered by Dreditor.
Comment #50
sunIf you still think that these callback groups are better than module hooks, then let's do it!
One final remark:
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?
Comment #51
tstoecklerYup, great! :)
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.
Sure, we could test that.
Comment #52
sunRight 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:
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:
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.
Comment #53
tstoecklerHow about this one?
The tests pass and they actually revealed two bugs in the previous patch.
Comment #54
tstoecklerHere'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:
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.
Comment #55
tstoecklerA 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:
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.
Since the
$group
key is always initialized as an array, we might as well skip the!empty()
, because theforeach
will simply do nothing on an empty array.See above
See above
Specifying 'module' explicitly should not be needed, as 'example_empty' is specified by the same module.
The 'variants' key should be unset in the 'load' phase, shouldn't it? Hmm...
Powered by Dreditor.
Comment #56
tstoecklerWell, both 'versions' and 'variants' can be unset, depending on the callback group. I changed the
!empty()
to aisset()
to make that clear, and because it's faster.Otherwise incorporated #55.
Comment #57
tstoecklerI managed to leave whitespace out of this one, that must be a sign...
Comment #58
tstoecklerRe-categorizing
Comment #59
tstoecklerRe-uploading for testbot, I just saw the HEAD branch passes now, for whichever reason...
Comment #60
tstoecklerFor consistency, we might want to change this to
isset()
as well.We could/should also test loading a specific variant.
Minor, though, so still needs review rather than need work.
Powered by Dreditor.
Comment #61
tstoecklerFixed #60
Comment #62
sunThanks 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.
Comment #63
sunI think I need this little follow-up tweak for jQuery module.
Comment #65
sunok, rather this. oyoyoy, the tests are a bit hard to grok ;)
Comment #66
sunThough... are we (going to) caching the results of library_detect()?
Comment #67
tstoecklerHow about:
- info
- pre-detect
- post-detect
- load
?
Yes, I like that. Makes the tests more readable, definitely.
Why not specify the 'callbacks' array right up front?
..and below.
Yes, code comments do make sense here. (Sorry!)
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.
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.
Comment #68
sunWasn't able to make too much progress on jquery.module yet.
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.
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 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?
Comment #69
tstoecklerFor 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.
D'uh.......
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.
Comment #70
sunTo clarify jquery.module's situation:
jquery-1.5.2.min.js
. The filename contains the version and variant.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.
Comment #71
tstoecklerOkay 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?
Comment #72
tstoecklerHere'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.
Comment #73
tstoecklerOnly in case of a reroll: Wraps to early.
Powered by Dreditor.
Comment #75
tstoecklerOh darn you distributed version control (forgot to git pull before diffing...).
Comment #77
tstoecklerThis one should work, the last one was a bit messed up. I re-ran the tests locally and they pass now.
Comment #78
tstoecklerWhile I'm at it, here's one with better wrapping for libraries_info_defaults().
Comment #79
sunLooks like I copied the phpDoc from somewhere else; mostly invalid here.
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.
Comment #80
tstoecklerHere's a reroll which fixes #79.
Comment #81
tstoecklerHere'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.
Comment #82
sunLooks good, let's do this. :)
Comment #83
tstoecklerhttp://drupal.org/commitlog/commit/10030/e6f845f41b5ab228918573642009e7d...
All right. Will post a new patch in a minute.
Comment #84
tstoecklerWell, here we go.
My internet connection is strange...
Comment #85
sunI 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."
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.
Odd wrapping here.
Powered by Dreditor.
Comment #86
tstoecklerHere 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...
Comment #87
tstoecklerYup.
http://drupal.org/commitlog/commit/10030/f455d71fdf58eb62238fe32dcc2b563...