Our support of drupal_add_js/css is a bit superficial currently.
There are four 'modes' (drupal_add_css only has 3, but that doesn't really matter):

  1. file
  2. external
  3. inline
  4. setting (JS-only)

We (implicitly) say in libraries.api.php that we support all of them, but currently that's not entirely true.
Because we have to prepend the library path to filenames (type 'file'), we currently also prepend the library path to inline JS (and CSS). 'setting' currently works (it should work, at least, I haven't tried it).
'external' also works, but is not really necessary, because we check with url_is_external() for external filepaths.

So the question is, whether we want to support 'inline' and 'setting' JS/CSS. It would be most flexible, but IMO both 'inline' and 'setting' defeat the purpose of external libraries. If we don't want to support them libraries_load_files() will get a bit simpler, otherwise a bit more complex.

I will try to roll patches for both.

CommentFileSizeAuthor
#10 1023274-10-libraries-js-css.patch11.25 KBtstoeckler
#8 js_css.patch14.25 KBtstoeckler
#7 test_js_css.patch16.5 KBtstoeckler
#2 libraries-yes-inline-and-setting.patch1.75 KBtstoeckler
#1 libraries-no-inline-or-setting.patch1.59 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
libraries-no-inline-or-setting.patch1.59 KB

So much for the easy part.

PS: The part in the op about 'external' isn't true, I was falsely assuming that some version of #864376: Loading of external libraries was already in HEAD.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
libraries-yes-inline-and-setting.patch1.75 KB

Now the harder part, which wasn't as hard as I had thought. Probably needs some love, though. Also big reg.exp.-disclaimer!

Although I originally didn't like it, if we can support all 4 modes ('external' to be handled by #864376: Loading of external libraries) and get away with this little code (basically the same LOC as before), then I'm tempted to say we should do it.

Leaving at active because this is more a philosophical question at this point than a code-question.

Round 1: Fight!!!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Just a note that, whichever way we go, we should add tests in this issue that test all our officially supported methods.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I'm going to personally postpone working on this, until #864376: Loading of external libraries gets in.
Just a note that we probably don't want to do a preg_match() on every single JS/CSS file to be loaded on runtime.
A nicer way would be to add a callback (#958162: Allow groups of callbacks to be applied to libraries) which detects the type of JS/CSS files and specifies that in the 'prepare' phase, so that we can just pass on the type to drupal_add_js/css().

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

Postponing this on #958162: Allow groups of callbacks to be applied to libraries.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Active » Postponed

Right...

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
FileSize
test_js_css.patch16.5 KB

Here's a patch to test all types of JS and CSS. It correctly shows that we currently only support local files. It doesn't change the actual tests run, you'll need to enable libraries_test.module and check manually.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Postponed » Needs review
FileSize
js_css.patch14.25 KB

Here's a patch, which contains #1023258: Make 'files' (and 'integration files') declarations consistent and the above patch and also fixes support for all types of JS and CSS. See it in action at http://libraries-7.x-2.x-dev.euve23100.vserver.de/

Status: Needs review » Needs work

The last submitted patch, js_css.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
1023274-10-libraries-js-css.patch11.25 KB

Here's a reroll. I tested it and it still works. You need to test manually, though. SimpleTest passing doesn't mean anything here, because we can't test specific JS or CSS.

Some notes on the patch:
- I forked the syntax of declaring the "special" types from hook_library. Some reasons for that are below. Upfront just a note that, if we decide to go that way, we need to update our documentation in libraries.api.php.

1. The syntax for declaring e.g. JS settings vs. inline code vs. ... with hook_library() is not really consistent, self-explanatory.
2. We have to fork the code anyway to prepend the library path (which is what broke compatibility with the "special" types in the first place, and led me to open this issue).
3. Unlike drupal_process_attached() (which does the all the work for hook_library()) which processes the files on-the-fly, we know in advance which files we are going to load, so we can also determine the type in advance (which is what libraries_prepare_files() does with this patch).

That allows us to be lenient with the syntax, which IMO is better DX. Compare the test library in the patch with the (I'm guessing undocumented) way of adding e.g. JS settings with hook_library().

I'm very much open to suggestions, though, I'm only sold about 75% on the patch myself.

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented

Was looking at #864376: Loading of external libraries, and thought this was already working since you can pass in your own $options array. You're right though, it's weird sticking settings into "files"... Maybe rename "files" to "assets"?

$libraries['something'] = array(
  'files' => array(
    'js' => array(
      'something.js' => array(
        'type' => 'setting',
        'data' => array(
          'mysettings' => 5,
        ),
      ),
    ),
  ),
);
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Well it is currently possible to do 'inline' and 'settings'. Because the syntax is kind of weird, I think, I wasn't aware of that when posting this issue. The thing is, though, that, in contrast to drupal_render() we have to parse/manipulate this data anyway, to add the filepaths, so we might as well detect the type of JS being added, in order to simplify the syntax.

About renaming 'files' to 'assets' not sure, although I wouldn't be directly opposed. Have to ponder on that one...

joachim’s picture

joachim CreditAttribution: joachim commented

If this issue is going nowhere, it would be nice to at least change the api.php documentation so it's accurate.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Title: Decide on whether or not to support 'inline' and 'settings' types for CSS/JS » Document how to use 'inline' and 'settings' types for CSS/JS
Issue summary: View changes
Status: Needs review » Active

Both inline CSS/JS and JS settings are not relevant as far as I can tell for external libraries. Not sure, why we would support them.

OTOH, for integration files, this does somewhat make sense, so I agree with #13 that we should update the documentation. I think that would be better than some form of more explicit support.