Posted by mjpa on July 20, 2011 at 7:55pm
5 followers
| Project: | Libraries API |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | D7 stable release blocker |
Issue Summary
Using the following hook_library_info:
function my_module_libraries_info() {
return array(
'my_library' => array(
'version' => '1.0',
'name' => 'My Library',
'files' => array(
'js' => array('dependency-min.js', 'library-min.js'),
'css' => array('library.css'),
),
),
);
}The JS/CSS files were being loaded from the root of the site. This appears to be linked to #1023258: Make 'files' (and 'integration files') declarations consistent which changes how the files are set in the library. Attaching a patch that (i think) fixes the adding of the JS/CSS files...
Comments
#1
Needed the issue number for the patch :)
#2
I'm not sure whether I'm making sense, but I'd like to retain support for loading indexed arrays as a last resort -- alter hook implementations may not play nice with our structures (or may assume that indexed arrays would also be supported, just like for drupal_add_js/_css()).
What you've essentially fixed is attached patch, I believe.
#3
You do make sense and the patch does pretty much what mine did without potentially breaking when the files array is still indexed. Quickly tested the patch and it works fine for me.
#4
So the only remaining question is how this bug was able to be introduced, even though the existing unit tests seem to cover the functionality...
#5
Is it possible to get this patch committed seeing as there is no released version of the 2.x branch that doesn't have this issue?
#6
#7
While our tests pass, the functionality is currently broken, as a regression of #1023258: Make 'files' (and 'integration files') declarations consistent. You can see that by manually enabling libraries_test.module.
That just shows our poor testing support of JS/CSS files. The tests just verify that the filenames are somewhere in the DOM. Which they are. But they don't get loaded because of the very bug found here.
Will review and test the patch perhaps tomorrow, but it looks like the correct fix upon first sight.
I'll also see if we can at least tighten our tests to verify the correct path.
#8
Here is a patch that tests for the exact HTML, that each of the file types appear as in the DOM. It correctly fails for JS and CSS and passes for the PHP files (which were adapted in #1023258: Make 'files' (and 'integration files') declarations consistent).
#9
Note that it's quite a bit of code, because I couldn't decide whether to loop over extensions or filenames. Could be shortened maybe, but at the bottom of libraries.test code beauty isn't critical I guess.
#10
The last submitted patch, 1224838-libraries-better-tests.patch, failed testing.
#11
Tests also failed for PHP files though?
#12
I believe the comment said it passes for PHP files.
#13
That's very strange. It passes for PHP files locally, for me. I will dig into this including a proper fix in the next days.
#14
Here's a cleaned up test patch. It is essentially the same code as above.
It passes for PHP files with the 7.x-2.x branch.
It passes everything with the patch in #2.
If it fails for PHP files again, it would be awesome if someone could test/debug this locally, because I'm unable to (because it passes...).
I decided to commit #2 as a stopgap fix (and mess up the commit message of course...). We can debate some more in this issue, but it fixes the imminent problem. I also think we should commit some form of the attached patch. I'm not entirely sold on indexed arrays yet, but haven't thought about it too much.
#15
The last submitted patch, 1224838-libraries-better-tests-2.patch, failed testing.
#16
Some thoughts on the indexed vs. associative array:
There are two competing motives in an info callback:
1:
<?phpfunction mymodule_info_callback(&$library, $version = NULL, $variant = NULL) {
$library['files']['js'][] = 'mymodule.js';
}
?>
2:
<?phpfunction mymodule_info_callback(&$library, $version = NULL, $variant = NULL) {
foreach ($library['files']['js'] as $filename => &$options) {
$options['weight'] = 23717;
}
?>
We currently kind of support both (because of the patch committed above), but that doesn't really help, because currently detect and load callbacks cannot rely on a properly associative array, which defeats the purpose of making them properly associative in the first place. So I don't think that really is a good way out.
I think the best way would be to define another callback group ('pre-info' ?) and then document that 'pre-info' is for 1. and 'post-info' is for 2.
It's a bit clumsy, but in the end, everyone wins.
Thoughts?
#17
I just realized that we also have hook_libraries_info_alter() which runs BEFORE the info callback group, so we could also document to add files only there. Hmm....
#18
Note: I moved the testing issue to #1299076: Improve JS, CSS, PHP file testing
#19
#1: 1224838-1-files_paths.patch queued for re-testing.