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.
I guess we want to do this eventually.
This is basically the minimal code.
It makes the assumption that the library can be found by libraries_get_libraries() (which means it is some kind of a sites/all/libraries folder (we should probably add a hook_libraries_directories() for greater flexibility)) and that a [name].info file is located inside of a directory with the same [name]. Doesn't do recursive directory checking yet, I don't know how we want to do that.
Definitely needs work, but I want to see what the bot says.
Comment | File | Size | Author |
---|---|---|---|
#33 | 919632-info-files-32.patch | 2.72 KB | tstoeckler |
#31 | 919632-info-files-31.patch | 2.06 KB | tstoeckler |
#30 | 864376-libraries-cleanup-36.patch | 17.71 KB | tstoeckler |
#27 | 919632-libraries_info_files-27.patch | 10.2 KB | tstoeckler |
#26 | 919632-libraries_info_files-26.patch | 10.54 KB | tstoeckler |
Comments
Comment #1
tstoecklerYes, I have a notion of being contradictory. Really setting to 'needs review' this time. (Again, only for the bot.)
Comment #2
tstoecklerAlrighty, then. Bot is happy. Back to needs work.
Comment #3
tstoecklerA little less assumptions. Moves recursive directory searching and a hook_library_paths() into libraries_get_libraries. Includes tests.
Still assumes that the info file is named after the directory (ie: sites/all/libraries/$name/$name.info), I don't know if we can get by with that. Maybe yes, maybe no. Anyway this is needs review now.
Comment #4
tstoecklerAnd the patch...
Comment #6
tstoecklerWell, to be able to find the info file, I should probably include it in the patch...
Comment #7
sunThis looks pretty awesome already! :)
Some totally open thoughts that crossed my mind while reading the patch:
- _libraries_scan_directory() is cool. I wonder whether we should use an $options array like file_scan_directory() in D7. Also, given $options, it might make sense to limit the recursion to 2-3 directory levels? Right now, I think we're only interested in the first level even?
- I fear a high risk of abuse of hook_libraries_paths(), which might lead to more problems than it solves. Don't get me wrong - the API design, code, documentation, and idea is awesome! But in this case, I'm not sure whether it wouldn't be more safe to hard-code our testing exception. I may be mistaken, but I think that http://api.drupal.org/api/function/drupal_system_listing/7 cannot be altered for the same reason: standardization. It would be easy to add that hook back later, if we find out that it is needed.
- As I still have little ideas of where and how and by whom .info files will ultimately be defined, it's not easy to evaluate the currently proposed processing code. It seems to assume that .info files are placed within the library directory and use the directory name as basename; e.g.
sites/all/libraries/jquery.ui/jquery.ui.info
. I see that this might be one option, though I rather had thought of either1) a central .info file repository (i.e., the library paths itself, e.g.,
sites/all/libraries/jquery.ui.info
) or2) an include directory in modules; whereas an include directory in Libraries API module itself might be likely to achieve a central "community repository" (e.g.,
sites/all/modules/libraries/repo/jquery.ui.info
)Perhaps we just need to allow everything... (D'oh. :-| )
But overall, again, this is some awesome work you did here! Let's share and discuss some more ideas :)
Comment #8
sunRe-reading my follow-up, I just realized that putting .info files within the library directory is a bad idea -- users will want to update those directories like they are updating modules; i.e., delete the entire directory and extract the new -- .info file: gone ;)
So I guess it boils down to supporting 1) and 2)...?
Comment #9
sunSorry upfront for flooding this issue ;)
I guess it would make much sense if we'd scan for .info files first, and only afterwards scan for libraries. That is, because I think that an .info file definition should ideally look similar to this:
No? Yes? Bleh? :)
Comment #10
tstoecklerI thought long about where to expect the info files and then thought it would be good to stay in line with modules and themes, where the info files reside alongside the rest of the files. As you mention, though, that idea is flawed.
New approach (basically 1) and 2) from above):
- Look for info files in the libraries directories (sites/all/libraries et al) and allow modules to specify additional paths to look for via
hook_libraries_info_file_paths()
(better/shorter name welcome). The actual search uses file_scan_directory().- The above is done in a dedicated
libraries_info_files()
which is then called fromlibraries_info()
.Additional remarks:
- Variants and the like specifying alternative paths should actually work, regardless of this issue and also with info files (as of this issue), but it might not. I'm not sure. I think that's actually a good candidate for a more in-depth test. IMO not to be dealt with in this issue, though, unless we realize we are actually breaking something here.
- I used
'/[a-z][a-z0-9_]+.info/'
as the regular expression to look for, because I thought that's pretty much the standard one for machine names. There might be something better, though.Comment #11
tstoecklerhttp://drupal.org/cvs?commit=434084
I actually only wanted to add the example.info file for easier patch rolling in this issue, but ended committing the above patch. Sorry!
Since it doesn't break anything, I didn't roll it back, but feel free to do so.
Leaving at needs review, because the above patch does, while the current code in CVS obviously "needs work".
Comment #12
sunhahahah, nice way to force reviews! :-D
Mostly looks/looked good :)
Can we rename this to something that contains "scan"? The idea is to follow Drupal core's pattern of similar functions.
Pattern error and therefore PCRE compilation failure here: Missing closing ]
Also:
2) . should be \.
3) Let's delimit expressions with @, if possible. The "standard" / most often gets in the way, because of directory paths and stuff.
4) I think there should be a file_exists($dir) before invoking file_scan_directory().
5) The files are added via +=, but the order of directories is actually reversed; i.e., more specific libraries come after less specific ones. We either need to reverse the directory list or change += into array_merge().
Is libraries_info() cached? This scan is quite expensive.
2) The scan function should return proper file objects, not just the name/URI.
Shouldn't we test at least the title property here, or something? :)
Powered by Dreditor.
Comment #13
tstoecklerSmall simplification of the code.
I also noticed working on #542940: [meta] Support for downloading libraries via Composer, that it might be cooler to actually name the file example_info_file.info, so that the library name shows up as, 'example_info_file'. That would make my previous commit even more stupid...
EDIT: I dropped the 'module' key for libraries, because for libraries coming from info files, we have no way of recording that information. So it seems consistent to not record it at all.
Comment #15
tstoecklerThanks for the review, and sorry for stealing your very well spent time...
Patch attached including your and my suggestions.
Comments:
- Regarding caching the list of info files, I went with
drupal_static(__FUNCTION__)
. I have yet to find out how that relates tostatic $var
and what should be used where, but I just looked at how libraries_info() does it currently.- While it being incredibly minor, I'm still torn on the naming of the test info file. I went with 'example_info_file.patch'. We basically have to choose between better naming (just 'example' is not very descriptive) or consistency of file names (all other filenames are example_$i except README).
- Testing for the title seems to be a pretty good idea, because the info file parsing was completely broken in the previous patch. (The wrong filepath was passed to drupal_parse_info_file()) :)
Comment #17
tstoeckler#15: 919632-libraries_info_files-14.patch queued for re-testing.
Comment #18
sunThanks! :)
Ok, let's leave the caching out for now -- I think there's a separate issue for that anyway.
Note that when doing any kind of static/db caching pattern, then we usually do the opposite; i.e., test isset($files) and early-return that, if it is set. Otherwise, build it from scratch. That pattern saves us from putting all kind of code into conditions.
So let's revert that here.
1) Why do we enforce a letter, btw? While I don't know of any, I think that a library name of "11fools" would be perfectly valid.
2) mmm, sorry, I also didn't realize that the pattern is not anchored in any way -- it should start with ^ and end with $, so we skip any temporarily commented out files, such as tinymce.info.orig
The removed code assigns $module and $name to the library. I think we want to keep that.
It looks like .info files always win over module-defined libraries. Probably makes sense, but would be good to leave a code comment here.
$path usually means a directory path; ideally rename to $filepath or $infofile.
Powered by Dreditor.
Comment #20
tstoecklerSure thing.
About 'module' and 'name'.
I moved 'name' into the "// Provide defaults" section. That allows people to override that property from their own declarations, which is bad. But it's an undocumented property, and if they want to break their code, why not let them do it?
I removed the 'module' property, because we can't store that for info-file libraries anyway, and I found it better to be consistent.
Comment #21
sunOn third sight: ;)
. and - also need to be valid characters; e.g., jquery.ui.info or jquery-ui.info.
Minor: Indentation hiccup here.
I understand your idea, but then again, it seems like the 'module' property is the only way to figure out whether a library has been defined by a module or an .info file? :)
mmm, how about:
".info files override module definitions."
?
Powered by Dreditor.
Comment #23
tstoecklerNext round.
While I still don't like the idea of the data differing depending on where it came from, I added the 'module' parameter back. But if we really need to know where the data came from, we should do that for info file libraries as well, added an 'info file' property. I guess it can't hurt, in the end.
Regarding the regular expression. Should we just come up with a list of names we support and then test against that? That might also be helpful for developers to know how they can name their files. Speaking of that:
Again a very minor point, but while I included it in the regular expression for now, I dislike having '.' in the library names. In Drupal points are used to singalize different types of files (.tpl.php, .admin.inc), and I find that a bit confusing. '-' I can live with, even though that is also an inconsistency with Drupal module and theme naming.
Comment #24
sunMinor: A dot (.) does not need to be escaped in a character range (i.e., [])
Do we need $filepath elsewhere? If not, we don't have to define it, just pass that concatenated info file path.
Thanks, that makes even more sense! :)
:)
Btw, are we actually asserting elsewhere that 'module' is set?
Powered by Dreditor.
Comment #26
tstoecklerWe use $filepath to store $library['info file'], so kept that.
Added a test, that was apparently missing, that the library information ends up correctly in libraries_info().
Comment #27
tstoecklerThat shouldn't be removed. (Leftover from the module_invoke_all() approach).
Rerolled for that.
I just reviewed the patch and this looks pretty much ready to go for me.
Just a note as to why I added titles to all the example libraries:
1. We'll want to do that eventually anyways, when we add a UI.
2. We already tested for the the title of the example_info_file library, so it seemed consistent to test for a title with example_simple as well.
3. It seemed weird to add a title to only one library and not all of them.
Of course I can remove that, if you want, but I think those changes are pretty self-contained.
I still think, we should somehow document, and possibly test, what types of filenames we want to allow. As you can see from me, not everyone speaks the language of regular expressions fluently...
That is something for a follow-up issue, though, IMO.
Powered by Dreditor.
Comment #29
tstoecklerI tried to reroll, but this is sort of postponed on #939174: Notices on the modules page (due to fake info file) now.
Comment #30
tstoecklerIf my thinking is correct, this works, but will fail until the cleanup patch in #864376: Loading of external libraries is committed.
Comment #31
tstoecklerI think this is all that is left for this issue.
Comment #32
tstoecklerWell we can actually account for #944198: Functions that call drupal_system_listing() act on potentially invalid system items here I think. Tested with our new Drush command (yay!).
Comment #33
tstoecklerEhh.. And the patch.
Comment #34
sunVery nice work! Does this patch need to wait for the clean-up patch? Either way, ready to fly :)
Comment #35
tstoecklerWell, the tests don't pass with or without this patch, so...
http://drupal.org/cvs?commit=446140
In fear of stating the obvious I removed the
tests/libraries_info_example/libraries_info_example.module
tests/libraries_info_example/libraries_info_example.info
files prior to commit. The testbot is choking anyway currently due to some other problem, and it looks like #944198: Functions that call drupal_system_listing() act on potentially invalid system items isn't too far off.