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.
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...
Comment | File | Size | Author |
---|---|---|---|
#14 | 1224838-libraries-better-tests-2.patch | 3 KB | tstoeckler |
#8 | 1224838-libraries-better-tests.patch | 3.63 KB | tstoeckler |
#2 | libraries.path_.2.patch | 1.08 KB | sun |
#1 | 1224838-1-files_paths.patch | 1.15 KB | mjpa |
Comments
Comment #1
mjpa CreditAttribution: mjpa commentedNeeded the issue number for the patch :)
Comment #2
sunI'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.
Comment #3
mjpa CreditAttribution: mjpa commentedYou 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.
Comment #4
sunSo the only remaining question is how this bug was able to be introduced, even though the existing unit tests seem to cover the functionality...
Comment #5
mjpa CreditAttribution: mjpa commentedIs 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?
Comment #6
sunComment #7
tstoecklerWhile 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.
Comment #8
tstoecklerHere 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).
Comment #9
tstoecklerNote 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.
Comment #11
sunTests also failed for PHP files though?
Comment #12
mjpa CreditAttribution: mjpa commentedI believe the comment said it passes for PHP files.
Comment #13
tstoecklerThat's very strange. It passes for PHP files locally, for me. I will dig into this including a proper fix in the next days.
Comment #14
tstoecklerHere'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.
Comment #16
tstoecklerSome thoughts on the indexed vs. associative array:
There are two competing motives in an info callback:
1:
2:
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?
Comment #17
tstoecklerI 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....
Comment #18
tstoecklerNote: I moved the testing issue to #1299076: Improve JS, CSS, PHP file testing
Comment #19
deltab CreditAttribution: deltab commented#1: 1224838-1-files_paths.patch queued for re-testing.
Comment #20
tstoecklerFixing title in light of the discussion in later comments here.
Comment #21
tstoecklerReading this issue again, I'm pretty certain we should stick to the 'filenames as keys' thing consistently. We should document that as well. Since that would be an API change, I guess that makes this 3.x material.
Comment #22
tstoecklerI think at this point changing any of this would break people's sites, so I think we'll have to live with this. Luckily in D8, Drupal core solved this problem from the getgo.
Comment #23
joelpittetOpening to test something...
Comment #24
joelpittet