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...

Files: 
CommentFileSizeAuthor
#14 1224838-libraries-better-tests-2.patch3 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 35,853 pass(es), 12 fail(s), and 0 exception(es).
[ View ]
#8 1224838-libraries-better-tests.patch3.63 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 35,839 pass(es), 12 fail(s), and 0 exception(es).
[ View ]
#2 libraries.path_.2.patch1.08 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,823 pass(es).
[ View ]
#1 1224838-1-files_paths.patch1.15 KBmjpa
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1224838-1-files_paths_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new1.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1224838-1-files_paths_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Needed the issue number for the patch :)

StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 35,823 pass(es).
[ View ]

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.

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.

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...

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?

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.

Title:JS / CSS files being loaded from rootRegression: JS / CSS files being loaded from root
Priority:Normal» Major
StatusFileSize
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] 35,839 pass(es), 12 fail(s), and 0 exception(es).
[ View ]

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).

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.

Status:Needs review» Needs work

The last submitted patch, 1224838-libraries-better-tests.patch, failed testing.

Tests also failed for PHP files though?

I believe the comment said it passes for PHP files.

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.

Status:Needs work» Needs review
StatusFileSize
new3 KB
FAILED: [[SimpleTest]]: [MySQL] 35,853 pass(es), 12 fail(s), and 0 exception(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1224838-libraries-better-tests-2.patch, failed testing.

Some thoughts on the indexed vs. associative array:
There are two competing motives in an info callback:
1:

<?php
function mymodule_info_callback(&$library, $version = NULL, $variant = NULL) {
 
$library['files']['js'][] = 'mymodule.js';
}
?>

2:

<?php
function 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?

Status:Needs work» Active

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....

Note: I moved the testing issue to #1299076: Improve JS, CSS, PHP file testing

Status:Active» Needs review

#1: 1224838-1-files_paths.patch queued for re-testing.

Title:Regression: JS / CSS files being loaded from rootFix the inconsistency between filenames as array keys and array values

Fixing title in light of the discussion in later comments here.

Version:7.x-2.x-dev» 7.x-3.x-dev
Status:Needs review» Active

Reading 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.