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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mjpa’s picture

FileSize
1.15 KB

Needed the issue number for the patch :)

sun’s picture

FileSize
1.08 KB

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.

mjpa’s picture

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.

sun’s picture

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

mjpa’s picture

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?

sun’s picture

tstoeckler’s picture

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.

tstoeckler’s picture

Title: JS / CSS files being loaded from root » Regression: JS / CSS files being loaded from root
Priority: Normal » Major
FileSize
3.63 KB

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

tstoeckler’s picture

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.

sun’s picture

Tests also failed for PHP files though?

mjpa’s picture

I believe the comment said it passes for PHP files.

tstoeckler’s picture

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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3 KB

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.

tstoeckler’s picture

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

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

2:

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?

tstoeckler’s picture

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

tstoeckler’s picture

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

deltab’s picture

Status: Active » Needs review

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

tstoeckler’s picture

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

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

tstoeckler’s picture

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.

tstoeckler’s picture

Issue summary: View changes
Status: Active » Closed (won't fix)

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

joelpittet’s picture

Status: Closed (won't fix) » Active

Opening to test something...

joelpittet’s picture

Status: Active » Closed (won't fix)