Objective

  1. hook_library_info() is one of the last remaining info hooks in D8.
  2. All manual asset file inclusions via former drupal_add_js() and drupal_add_css() have been removed in D8 in favor of library definitions that properly declare their dependencies.
  3. Consequently, almost all modules (and also themes) need to declare their libraries in D8. However, that is currently done in PHP and the format is an arbitrary array of declarative data.
  4. Already starting with its original introduction in D7, hook_library_info() was architecturally designed to supply static declarations only; i.e., the declarations must not depend or rely on any request or context information. Since they are currently declared in PHP, that architectural design aspect is not guaranteed.

Proposed solution

  1. hook_library_info() implementations are converted from PHP into $extension.libraries.yml files.

  2. Drupal core gets it own /core/core.libraries.yml file, which lives right next to core.services.yml.

    → Removing another dependency on System module.

  3. All external libraries that are shipped with Drupal core (the entire Drupal core thing) are centrally declared by core.libraries.yml and should be located in /core/assets/vendor.

  4. Slightly change the declaration format (1) to make more sense in YAML and (2) to help resolve some architectural asset system problems:

    1. Every external library declares a new 'remote' property whose value is the originating repository URL (typically GitHub, but any git remote URL is valid).
    2. For external libraries, the 'version' property denotes a tagged release (git tag) in the originating repository.
    3. The list of 'dependencies' is changed from the former array('provider', 'library-name') PHP syntax into a namespaced string: provider/library-name.
    4. For JavaScript asset files, all former custom 'group' options (for drupal_add_js()) are removed, since all JavaScript should rely on declared dependencies only.
    5. For CSS asset files, the definition structure is slightly changed to be declared in nested SMACSS category names, following our Drupal 8 CSS architecture documentation, and to aid in resolving some hard CSS asset file ordering problems that currently exist.
      → The additional classification info will likely be consumed by #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

API changes

  1. hook_library_info() is replaced by $extension.libraries.yml files:

    Before

      // jQuery Form Plugin.
      $libraries['jquery.form'] = array(
        'title' => 'jQuery Form Plugin',
        'website' => 'http://malsup.com/jquery/form/',
        'version' => '3.39',
        'js' => array(
          'core/assets/vendor/jquery-form/jquery.form.js' => array(),
        ),
        'dependencies' => array(
          array('system', 'jquery'),
          array('system', 'jquery.cookie'),
        ),
      );
    
      // Dropbutton.
      $libraries['drupal.dropbutton'] = array(
        'title' => 'Dropbutton',
        'website' => 'http://drupal.org/node/1608878',
        'version' => '1.0',
        'js' => array(
          'core/misc/dropbutton/dropbutton.js' => array(),
        ),
        'css' => array(
          'core/misc/dropbutton/dropbutton.css' => array(),
          'core/misc/dropbutton/dropbutton.theme.css' => array(),
        ),
        'dependencies' => array(
          array('system', 'jquery'),
          array('system', 'drupal'),
          array('system', 'drupalSettings'),
          array('system', 'jquery.once'),
        ),
      );
    

    After

    (Example from system_library_info() to /core/core.libraries.yml)

    jquery.form:
      remote: https://github.com/malsup/form
      version: 3.39
      js:
        assets/vendor/jquery-form/jquery.form.js: {}
      dependencies:
        - core/jquery
        - core/jquery.cookie
    #
    drupal.dropbutton:
      version: VERSION
      js:
        misc/dropbutton/dropbutton.js: {}
      css:
        component:
          misc/dropbutton/dropbutton.css: {}
        theme:
          misc/dropbutton/dropbutton.theme.css: {}
      dependencies:
        - core/jquery
        - core/drupal
        - core/drupalSettings
        - core/jquery.once
    

    The path of each asset file is resolved relative to the directory of the registering extension. However, absolute paths (e.g., /core/assets/vendor/... or /libraries/...) as well as (stream wrapper) URIs (e.g., http://example.com/foo.js or public://color/bartik/color.css) as well as protocol-free URIs (e.g., //api.google.com/jquery.js) are supported, too.

    The CSS asset file parent keys are mapping 1:1 to Drupal 8's CSS architecture documentation. Please refer to that documentation to learn more.

    Side-benefit for maintenance:

    Larger library declaration files (like core.libraries.yml) are able to leverage YAML references within the scope of the file, which works this way:

    jquery.ui:
      version: &jquery_ui 1.10.2
    ...
    jquery.ui.accordion:
      version: *jquery_ui
    ...
    jquery.ui.autocomplete:
      version: *jquery_ui
    
  2. hook_library_info_alter() now acts on the (raw) library definitions, as declared in $extension.libraries.yml files, and any performed manipulations are persistently cached.

API additions

  1. The new hook_library_alter() allows modules and themes to act right before a specific library is attached and processed.

    This allows extensions to add request-specific and context-specific data to a particular library. For example:

    1. JavaScript settings: E.g., Locale module dynamically adds drupalSettings containing translations for the jquery.ui.datepicker library, which are only needed when the current language is not English.
    2. Additional asset files that should only be loaded when a particular library is attached. This use-case previously required developers to write complex hook_js_alter() implementations.
    /**
     * Implements hook_library_alter().
     */
    function mymodule_library_alter(array &$library, $extension, $name) {
      if ($extension == 'core' && $name == 'jquery.ui') {
        // Add the backwards compatibility layer for jQuery UI.
        $library['dependencies'][] = array('mymodule', 'jquery.ui.compat');
    
        // Add our additional stylesheet to improve the experience of jQuery UI.
        $path = drupal_get_path('module', 'mymodule');
        $library['css'][$path . '/css/mymodule.jquery-ui.css'] = array('weight' => CSS_SKIN);
      }
    }
    

    Note:

    As you can see, hook_library_alter() uses the previously existing/internal format of hook_library_info()/drupal_add_library(). The internal data structure of $library might be revised prior to Drupal 8.0 — in particular, to retain the nested SMACSS category keys for CSS file assets, so as to avoid having to specify custom 'group' and 'weight' options. (Changing that is out of scope for this issue.)

Not addressed here → follow-up issues

  1. Convert all #attached]['library'][] = array(provider, name) into #attached]['library'][] = 'provider/name' throughout core.

    Reasoning: This patch is large enough already + very hard to re-roll/maintain.

  2. Convert drupal_get_library() into a service and:

    1. Inject all dependencies.
    2. Consider to use a single cache item for all extensions.
    3. Resolve the @todo of determining whether $extension is a module or theme.
    4. Resolve the @todo of determining the version of contributed and custom modules/themes.
    5. Introduce proper unit tests for drupal_get_library() (there are none yet).

    Reasoning: Aside from the retrieval + parsing code, drupal_get_library() + consuming code is not affected or changed by this patch.

  3. Ensure that libraries and library dependencies are only processed and resolved once prior to rendering out an HTML page, so that the order of dependencies of all attached libraries is correct and all remaining 'weight' options can be dropped from all declarations.

    Reasoning: drupal_add_library() and drupal_render() is not touched at all by this patch; i.e., the problem exists in HEAD already.

    → This primarily means to help with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

.

.


Original report by @nod_

The recent trend has lead to the removal of almost all info hooks. There is a strong possibility that hook_library_info() would be the only info hook left in D8.

I propose that we move everything in hook_library_info() in a MYMODULE.library.yml or MYMODULE.assets.yml file.

It'll make it easier on themer to declare their script and CSS dependencies #1969916-5: Remove drupal_add_js/css from seven.theme and it's more likely they'll do it instead of some ugly PHP to implement in some file.

We'd keep library_info_alter() because that's very much needed. yml file would work because the information in the library hook isn't dynamic.

CommentFileSizeAuthor
#201 lib.css_.png174.05 KBsun
#193 interdiff.txt944.24 KBsun
#193 library.info.190.patch192.94 KBsun
#166 1996238-library-info-166.patch192.81 KBlongwave
#161 interdiff.txt3.47 KBsun
#161 library.info.161.patch192.87 KBsun
#157 interdiff.txt948 bytessun
#157 library.info.157.patch190.46 KBsun
#155 library.info.155.patch189.39 KBsun
#153 interdiff.txt6.83 KBsun
#153 library.info.153.patch189.26 KBsun
#151 interdiff.txt1.97 KBsun
#151 library.info.151.patch189.26 KBsun
#145 1996238.145.patch188.63 KBalexpott
#142 drupal_1996238_142.patch188.79 KBXano
#141 library.info.141.patch188.63 KBsun
#135 interdiff.txt572 bytessun
#135 library.info.135.patch188.63 KBsun
#133 interdiff.txt395 bytessun
#133 library.info.133.patch188.65 KBsun
#129 interdiff.txt388 bytesWim Leers
#129 library.info.129.patch188.52 KBWim Leers
#128 interdiff.txt1.51 KBsun
#128 library.info.128.patch188.5 KBsun
#119 interdiff.txt10.06 KBsun
#119 library.info.119.patch186.7 KBsun
#108 interdiff.txt3.75 KBsun
#108 library.info.108.patch186.35 KBsun
#106 library.info.106.patch183.6 KBsun
#103 interdiff.txt21.95 KBsun
#103 library.info.103.patch183.55 KBsun
#101 interdiff.txt404 bytessun
#101 library.info.101.patch181.39 KBsun
#98 interdiff.txt14.4 KBsun
#98 library.info.98.patch181.34 KBsun
#96 edit_css_fix-do-not-test.patch1011 bytesWim Leers
#88 interdiff.txt4.07 KBsun
#88 library.info.88.patch183.23 KBsun
#86 interdiff.txt1.18 KBsun
#86 library.info.86.patch180.94 KBsun
#84 interdiff.txt2.58 KBsun
#84 library.info.84.patch180.1 KBsun
#81 interdiff.txt51.21 KBsun
#81 library.info.81.patch179.25 KBsun
#79 interdiff.txt6.43 KBsun
#79 library.info.79.patch179.18 KBsun
#77 interdiff.txt2.33 KBsun
#77 library.info.77.patch176.71 KBsun
#73 interdiff.txt3.72 KBsun
#73 library.info.73.patch175.45 KBsun
#71 interdiff-1996238-71.txt687 bytesdamiankloip
#71 1996238-71.patch175.67 KBdamiankloip
#69 interdiff-1996238-69-rebase.txt9.1 KBdamiankloip
#69 1996238-69.patch175.67 KBdamiankloip
#64 interdiff.txt27.16 KBsun
#64 library.info.64.patch174.88 KBsun
#60 interdiff.txt103.8 KBsun
#60 library.info.60.patch167.87 KBsun
#57 interdiff.txt7.87 KBsun
#57 library.info.57.patch90.83 KBsun
#54 interdiff.txt53.82 KBsun
#53 library.info.53.patch90.67 KBsun
#49 core-library-yml-1996238-49.patch124.42 KBnod_
#33 core-library-yml-1996238-33.patch115.37 KBnod_
#32 core-library-yml-1996238-32.patch78.3 KBnod_
#30 core-library-yml-1996238-30.patch115.95 KBnod_
#24 core-library-yml-1996238-24.patch112.52 KBnod_
#22 core-library-yml-1996238-21.patch109.84 KBnod_
#19 core-library-yml-1996238-19.patch110.05 KBnod_
#17 core-library-yml-1996238-17.patch109.97 KBnod_
#13 core-library-yml-1996238-13.patch100.37 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pol’s picture

FYI: I'm using dynamic informations in hook_library_info() for OpenLayers 7.x-3.x, I haven't find anything better yet.

nod_’s picture

On the dynamic nature of library_info things, I got scolded by sun about it #1737148-89: Explicitly declare all JS dependencies, don't use drupal_add_js which is fair.

About the openlayer case It's convenient to do it like that yet it's still a static list of files. Declaring them one by one would be boring but isn't impossible. I guess you'd have a script generating the data and dumping everything in a yml file.

Pol’s picture

or use hook_library_info_alter() ?

nod_’s picture

that too, yes. Doesn't feel too right though :p

jessebeach’s picture

A module like Editor passes data with the hook_library_info declaration as well.

$libraries['edit.formattedTextEditor.editor'] = array(
  'title' => 'Formatted text editor',
  'version' => VERSION,
  'js' => array(
    $path . '/js/editor.formattedTextEditor.js' => array(
      'scope' => 'footer',
      'attributes' => array('defer' => TRUE),
    ),
    array(
      'type' => 'setting',
      'data' => array(
        'editor' => array(
          'getUntransformedTextURL' => url('editor/!entity_type/!id/!field_name/!langcode/!view_mode'),
        )
      )
    ),
  ),
  'dependencies' => array(
    array('edit', 'edit'),
    array('editor', 'drupal.editor'),
    array('system', 'drupal.ajax'),
    array('system', 'drupalSettings'),
  ),
);

The idea is that one could declare libraries with the hook or the yml file, correct?

Dave Reid’s picture

It's a very common use case to be able to specify the path to my module in the path of the assets. I wonder if we need to require #1308152: Add stream wrappers to access extension files so that we can declare the paths to the assets using module://edit/js/editor.formattedTextEditor.js.

nod_’s picture

Very good point.

@Jesse: I don't see any issue with declaring data. As long as it's not dynamic data. I want only one way of declaring that not both. That'd suck to explain during training.

jcisio’s picture

#6 #7 Would the path not be relative path? Each module/theme declares its own assets and dependencies. There is no need to use those system stream wrappers.

nod_’s picture

at least for the system module that's not true. But yeah, for contrib I'd agree with that.

pounard’s picture

I would tend to think that those stream wrappers could be useful, but not required.

nod_’s picture

Ok so here's why I sort of have.

system.library.yml

drupal:
  title: Drupal
  version: 8.0-dev
  js:
    -
      data: core/misc/drupal.js
      group: -100
      weight: -18
  dependencies:
    - system/domready

drupalSettings:
  title: 'Drupal Settings'
  version: 8.0-dev
  js:
    1:
      data: 0
      type: setting

drupal.ajax:
  title: 'Drupal AJAX'
  website: 'http://api.drupal.org/api/group/ajax/8'
  version: 8.0-dev
  js:
    -
      data: core/misc/ajax.js
      group: -100
      weight: 2
  dependencies:
    - system/jquery
    - system/drupal
    - system/drupalSettings
    - system/drupal.progress
    - system/jquery.once

The current structure is really ugly when dumped in yml. So I made a few tweaks.

  • dependencies: they used to be array('system', 'drupal') and I turned them into system/drupal. It makes sense for several reasons: It's exactly what we'd do if we were using AMD (see #1542344: Use AMD for JS architecture for context) because you can say that system/ maps to this URL and the AMD loader will load the file from the right place by itself. The final data array is not a 2 dimension array with [$module][$key] but a 1 dim with [$module / $key]
  • file info: previously 'path/file/name.js' => array(/* options */) that makes for an ugly export in yml. So I turned it into array('data' => 'path/file/name.js', /* options */) Because that's what is done to it down the line anyway.
  • The weights are a bit ugly but ideally we wouldn't have to use them since all deps are declared, see #1805552: Remove custom weights from library definitions.
  • As for the groups, well It's all up in the air with the Assetic/assets handling rewrite. I'm guessing ultimately we'd want named groups.
jessebeach’s picture

Regarding my concern in #5, I went through the hook_library_info invocations in core now and there are no instances of dynamic data. If one were to need dynamic data on the front end, the best approach would be to set up an AJAX request for the information.

nod_’s picture

Status: Active » Needs review
FileSize
100.37 KB

Ok that's a dirty patch but it works, files load and all. Didn't put the _alter hook (just forgot, it's easy to add back).

The file includes/test.php is what I used to generate the yml files from the library_info hooks. not all modules are updated.

Wasn't as bad as I thought :)

Status: Needs review » Needs work

The last submitted patch, core-library-yml-1996238-13.patch, failed testing.

tim.plunkett’s picture

This works for core modules, since they are always in /core/modules.
How does this work for contrib, where they could be in /modules, /sites/*/modules, /profiles/*/modules....

nod_’s picture

pretty east to add a drupal_set_path in the drupal_get_library call. just need to put relative file path even for core modules.

nod_’s picture

Status: Needs work » Needs review
FileSize
109.97 KB

like that.

I think I got all hook_library_info() converted with this. The result definitely needs to be cached somewhere my patch is probably dead slow.

Status: Needs review » Needs work

The last submitted patch, core-library-yml-1996238-17.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
110.05 KB

maybe this one?

Status: Needs review » Needs work

The last submitted patch, core-library-yml-1996238-19.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

That's where I'm at now, not too happy about some code in drupal_get_library() but it works for now.

drupal:
  title: Drupal
  version: 8.0-dev
  js:
    - { misc: drupal.js, group: -100, weight: -18 }
  dependencies:
    - system/domready

drupalSettings:
  title: 'Drupal Settings'
  version: 8.0-dev
  js:
    - { settings: [] }


drupal.text:
  title: Text
  version: 8.0-dev
  js:
    - { file: text.js }
  dependencies:
    - system/jquery
    - system/jquery.once
    - system/drupal

The type key is gone from the yml files, it makes more sense to use file, external, settings, misc (yeah that's a new one, otherwise it makes solving #15 kind of a pain, see @todo inline) as keys for people to use. It's converted back to data/type in drupal_get_library().

nod_’s picture

With the patch that could help…

One nice thing is that it removes 1k lines of PHP from system.module :)

Status: Needs review » Needs work

The last submitted patch, core-library-yml-1996238-21.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
112.52 KB

Now with less test failures.

nod_’s picture

Title: Declare library_info in yml file » Replace hook_library_info() by *.library.yml file

haha didn't expect green, I'm not complaining though :)

( edit ) the patch is NW really. The library info needs to be cached somewhere, I'm pretty sure the current patch puts a real bad perf hit. I'd like to have opinions though.

Wim Leers’s picture

Overall, I don't see problems with this approach. But the "why?" question isn't adequately/clearly answered by the issue summary yet.


I don't see test coverage yet?


+++ b/core/modules/block/block.library.ymlundefined
@@ -0,0 +1,8 @@
+  version: 8.0-dev

We'll have to update this version string every single time Drupal core is updated for every single core module with libraries. That's… a PITA.

What about version: core or version: CORE_VERSION, i.e. a magical value?

+++ b/core/modules/system/system.library.ymlundefined
@@ -0,0 +1,676 @@
+    - { misc: tabbingmanager.js, 0: group, 1: -100 }

Not quite right :)

tim.plunkett’s picture

Here is why. Added to the issue summary.

The recent trend has lead to the removal of almost all info hooks. There is a strong possibility that hook_library_info() would be the only info hook left in D8.
nod_’s picture

@Wim, good catch: #2002622: Wrong declaration for tabbingmanager script in system_library_info()

Also, Angie raised a concern about themers needing to implement hook_library_info() for their scripts (which they'll need to do) and I'm sure mfer would agree with her. YML file feels like it'd be easier to write for them.

And Yeah, the version think kinda suck. Not sure how to deal with that, get the version from the info file if it's not specified maybe?

Wim Leers’s picture

#27: Thanks! Makes sense :)

#28: In module modulename.info.yml files, we apparently still have VERSION. Can't we make that work here too?

nod_’s picture

Was in the middle or rerolling it :)

Here it is, with VERSION.

Caching needs to be added, closed #1787758: drupal_get_library() / hook_library_info() is not cached to get it done here. I'm not sure where to put the code for that so help is appreciated.

Not posting interdiff, only differences are some updated libraries dependencies declarations that happened in system_library_info(), the 3 lines for supporting VERSION and I removed hook_library_info from system.api.php (not sure where the doc should go though).

cweagans’s picture

Instead of calling the Yaml parser directly, you could just call drupal_parse_info_file(), which is already statically cached and will substitute the value for the VERSION constant.

nod_’s picture

nod_’s picture

with the new files…

Dave Reid’s picture

Some blockers on this approach considering contrib would need to use this YML approach as well:

  1. Modules cannot assume that it's assets live in the same directory as core files, and by nature are dynamic. Modules also need to register external library assets; we can't assume that relative paths are just ok because the reality is that modules register CSS/JS libraries that live in sites/all/libraries, sites/default/libraries, etc. We need a solution like #1308152: Add stream wrappers to access extension files or some kind of token replacement in order for this to actually work for contrib.
  2. While VERSION works great for core modules, it does not work for contrib modules. And there's no real good solution for what modules that provide their own libraries should use for the version string, because it's a pain for those modules to maintain those manually as well. Should the 'VERSION' string token actually check drupal_parse_info_file() to see if that module/theme/etc has an actual version, and if not fall back to the VERSION constant?
nod_’s picture

  1. having the stream wrappers would be nice indeed. For now we don't. the patch as it is makes it hard to reference files outside module dir (you can use external with absolute path though). Libraries, that's not in core as far as I know. We still have library_info_alter() to add stuff from dynamic sources. Since it's dynamic not much to do in a yml file i'd say.
  2. I'd agree with that.
tstoeckler’s picture

Re 1: Module shouldn't be declaring external libraries in library.yml just like they currently shouldn't be declaring external libraries in hook_library_info(). That problem-space is (at least in theory) solved by Libraries API. hook_library_info() has always been specifically for libraries that are shipped with the module, and thus assuming that the files are in the module directory is in fact the only possible assumption.

pounard’s picture

I do not agree with #36, all libraries and dependencies, external or internal, should be declarer via the same registry, in order to ensure the core is able to include everything, and possibly in a certain order.

tstoeckler’s picture

Re #38: In theory I agree 100% with you. hook_libraries_info(), however relies on the fact that a specific version of a library is available. That can only guaranteed if the library is shipped with the rest of the code. For libraries that must be obtained separately there is no way to guarantee a single version. Therefore hook_libraries_info() resp. *.library.yml is not suitable for external libraries. It might that Libraries API ends up implementing something like that in order to facilitate proper ordering and dependencies, but that's a whole different issue. That's just how it is currently, I'm not saying it couldn't be any better.

I was just trying to argue that

Modules cannot assume that it's assets live in the same directory as core files, and by nature are dynamic.

in #34 is not correct.

jcisio’s picture

I agree with both lol. All libraries should be declared via the same registry. However, external libraries that module don't know the path yet, can be still declare in .yml then use hook_library_info_alter() to change the path.

jcisio’s picture

Didn't see #38. "Shared libraries" can be declared via hook_library_info_alter() too.

tstoeckler’s picture

Let's please leave external libraries out of this. There's a reason Libraries API is a non-trivial project. That's why hook_libraries_info() evaded that entire problem-space from the start. We would all like to have (a complete/modern/awesome version of) Libraries API in core, but that's simply not the case. So, yes we *can and should* make the assumption that libraries provided by modules in *.library.yml are within their filepath.

pounard’s picture

I don't think it makes the problem being more complex than it already is. It makes nonsense IMHO to separate external from internal libraries, external should just be a state within the declaration that only change the final aggregation behavior, and nothing else (dependency managing and such would be the exact same). It does not change the way we'd deal with internal libraries and their filepaths.

EDIT: Moreover:
1. Nothing prevents a module (for example jQuery Update) altering the info, and changing an internal library by an external one.
2. I don't think that Libraries API is a good example, I personally avoid using it whenever I can.

tstoeckler’s picture

Sorry for being imprecise, "external" in this case can mean multiple things. I was not talking about externally referenced files, as in the 'external' option of drupal_add_js(). I was talking about libraries that are not shipped with modules.

Either way, the last sentence of #41 stands. Can we get back on topic regarding the actual patch now?

pwolanin’s picture

Version: 8.x-dev » 9.x-dev
Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

This seems inconsistent compared to the pattern of converting info hooks to plugins, but sure that's a good fit here - why not just leave it as an info hook?

tim.plunkett’s picture

Fixing tags

nod_’s picture

@pwolanin: it's much nicer to write yml files than php arrays. Themers will need to write them too.

And to address #34 1. as msonnabaum said IRL, we could reference callbacks in the yml file that will generate the libraries definitions similar to what we do for routing.

nod_’s picture

Issue summary: View changes

added reasoning

sun’s picture

It appears that hook_library_info() is one of the biggest offenders in the remaining list of info hooks in D8:
https://api.drupal.org/api/drupal/8/search/_info

Info hooks should not have a place in D8 anymore.

In addition, the proposed change here enforces an important aspect of the architectural design of hook_library_info():

The library information is declarative.

This aspect is not clear to everyone. In fact, in its very early stages, the (Quick)Edit or Editor module in D8 core registered libraries in a dynamic way, depending on request-specific information, which was incorrect.

By moving hook_library_info() from PHP code into a YAML file,

  1. we not only eliminate one of the last remaining info hooks (DX++)
  2. we also inherently make it clear that the library definitions are declarative and not supposed to be dynamic (DX++)

As a result, this issue should be moved back to D8 and resolved as proposed. Thanks for working on this, @nod_.

andypost’s picture

Version: 9.x-dev » 8.x-dev
Issue summary: View changes

SO let's get back to 8.x because a lot of issues could be seriously improved btw
Mostly this will help with #2073823: [META] Remove drupal_add_js() calls

nod_’s picture

Reroll, be careful it's "yeah whatever I just want it to work" quality.

  1. drupal_get_library() needs to know the path of the theme/module to be able to parse the yml file, so there is an ugly workaround for themes.
  2. Since we split out vendor scripts that used to be in core/misc. There are 3 magic keys for path files now: file (relative to module folder), misc (core/misc), asset, (core/assets/vendor). I need them to make anything work, if there is a better alternative I'm happy to hear about it. module:// sounds like there will be a lot of duplication. How often do people declare stuff that lives outside their directory? If we move everything from misc/ to the system module (where it should be) we can get rid of one magic key.

I included the script to generate the libraries in /lib.php, feel free to use it, should be removed for final patch obviously.

Fair warning, that's not the kind of core work I have fun doing. I have no idea at which level to implement caching and i'm not interested in figuring it out. I really need someone to take care of the rest of those things.

Example:

drupal.announce:
  version: VERSION
  js:
    - { misc: announce.js, group: -100 }
  dependencies:
    - system/drupal
    - system/drupal.debounce
    
jquery.ui.core:
  version: VERSION
  js:
    - { asset: jquery.ui/ui/jquery.ui.core.js, group: -100, weight: -11 }
  css:
    - { asset: jquery.ui/themes/base/jquery.ui.core.css }
    - { asset: jquery.ui/themes/base/jquery.ui.theme.css }
  dependencies:
    - system/jquery
    
Dave Reid’s picture

How often do people declare stuff that lives outside their directory?

Basically every module in contrib that uses sites/[all|default]/libraries?

nod_’s picture

No they don't declare them, they use them as dependencies. It's the libraries module that should declare them. See #41 and #43, who am I to argue against that :p

Status: Needs review » Needs work

The last submitted patch, 49: core-library-yml-1996238-49.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
90.67 KB
  1. Fixed NoJavaScriptAnonymousTest.
  2. Moved "core" (base system) library declarations into /core/core.library.yml.
  3. Replaced the custom "misc:" construct with natural absolute path definitions.

What I did not touch (yet):

  • Syntax/structure of library.yml declarations (which can be simplified)
  • Caching
  • Figuring out whether $module ($extension) is a module or a theme
  • Converting this whole shebang into a service

Due to the massive amount of file changes + created files, now maintained as a proper branch in the Platform sandbox:
http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/librar...

@nod_: Granted you write access to Platform; just in case you want to pick this up again. :)

sun’s picture

FileSize
53.82 KB

Sorry, belated interdiff

sun’s picture

+++ b/core/modules/ckeditor/ckeditor.library.yml
@@ -0,0 +1,64 @@
+ckeditor:

I missed this one; should be moved into the core library declaration.

The last submitted patch, 53: library.info.53.patch, failed testing.

sun’s picture

FileSize
90.83 KB
7.87 KB
  1. Fixed /misc is apparently in /core/misc now. Wowza, yayza! :)
  2. Fixed CKEditor library is provided by core, not ckeditor module.

The last submitted patch, 57: library.info.57.patch, failed testing.

jibran’s picture

+++ b/core/includes/common.inc
@@ -2761,32 +2760,73 @@ function drupal_add_library($module, $name, $every_page = NULL) {
+    //   current extension data, possibly using a module->theme fallback mechanism.

80 chars.

sun’s picture

FileSize
167.87 KB
103.8 KB

Awesomeness warning:

  1. Simplified library info post-processing.
  2. Renamed #attached 'system' library calls to 'core' throughout Drupal core.
  3. Converted stale hook_library_info() implementations into library.yml files; adjusted tests.
  4. Updated drupal.autocomplete JS file group to match latest HEAD (JS_LIBRARY).
  5. Removed stale/obsolete views_ui_library_info_alter() implementation.
  6. Sort core.library.yml alphabetically.
  7. Correction: Specify base library first. + Fix names of jquery[.ui].effect.* libraries.
  8. Restored external library versions and remotes, leverage YAML references for version info.
  9. Renamed base jquery.ui.core library to jquery.ui for consistency.

Have a look at the new core.library.yml. :)

sun’s picture

nod_’s picture

So much win.

jquery.bbq:
  remote: https://github.com/cowboy/jquery-bbq
  # @todo Ping @benalman to create stable release.
  version: v1.3pre
  js:
    - { file: assets/vendor/jquery-bbq/jquery.ba-bbq.js }
  dependencies:
    - core/jquery

This lib is not in core anymore (gone with the overlay). Other than that, the other changes looks great to me.

Wim Leers’s picture

So much win indeed! Thank you, sun!

Fixed CKEditor library is provided by core, not ckeditor module.

I agree with this fix, but IIRC core committers felt it was more appropriate that the library as in fact owned by CKEditor module. So let's not change that in this patch.

Important comments, such as this one, should be retained though:

-      // Depend on Vertical Tabs, so that Vertical Tabs' JavaScript is executed
-      // first, which ensures its behavior runs first.
-      array('system', 'drupal.vertical-tabs'),
+    - core/drupal.vertical-tabs
sun’s picture

Assigned: Unassigned » sun
FileSize
174.88 KB
27.16 KB

A range of clean-ups, also incorporating above comments, before moving forward on the actual library.yml syntax (UX) + processing:

  1. Witness: All JS settings are dynamic. → Settings do not have a place in declarative definitions?
  2. Fixed CKEditor library settings are not static; provide a new method for global editor JS settings.
  3. Fixed Editor library settings depend on request/environment context.
  4. #62 Removed jquery.bbq definition. (stale/obsolete)
  5. Corrected jquery.cookie definition.
  6. Fixed external library definitions in Picture and Tour module.
  7. #63 Restored previously existing comments in library definitions.
  8. Readability: Added a blank line between library definitions of core modules, too.
  9. Removed superfluous "group: JS_LIBRARY" options in all definitions.

Happy to spin off the CKEditor + Editor module fixes into a separate issue, if necessary. They are required for this patch, and it's long overdue that the originally intended architectural design of static declarations is finally enforced. (cf. #47)


core committers felt it was more appropriate that the library as in fact owned by CKEditor module.

Yeah, focus on "was". :)

Back then, core.library.yml did not exist, so System module registering CKEditor module's library made little to no sense.

By introducing the capability for the Drupal core base system itself to register libraries, the new world order is this:

  1. All external libraries supplied by Drupal core are placed into /core/assets/vendor.
  2. All of those libraries are registered by core.library.yml.
  3. Placing an external library into a core module is discouraged and requires a very sound, solid, and sophisticated reasoning.

The first and foremost reason for that is simple:

A contributed or custom module that would like to use the existing bundled library but does not like the usage/implementation of the core module, would (1) either have to needlessly depend on the core module to get the library registered or (2) re-register the library from scratch on its own (which breaks dependencies of other modules, since the library owner/provider would no longer be "core").

Furthermore, re-registering a core library in a contrib/custom module would be close to impossible to manage with regard to the declared library version. Drupal core might ship with an updated version, but that will not be reflected in the contrib module's declaration unless it is updated accordingly.

In short, this is required for e.g. Wysiwyg module to use the CKEditor library bundled with core, without having to depend on the ckeditor.module in core. Same for other libraries/modules/use-cases.

Due to that, I did not revert the registration of the ckeditor library. Instead, I added two @todos for picturefill.js (Picture) and jquery.joyride (Tour) to move those external libraries into core.


Next steps, moving forward:

  1. The common declaration for the vast majority of files has no options, like this:

      js:
        - { file: js/foo.js }
        - { file: js/bar.js, weight: -2 }
    

    Ugly DX. → Researching and studying the YAML specification once more in-depth:

    It is possible to declare hashmap keys without values, as long as the key is terminated, like this:

      js:
        js/foo.js:
        js/bar.js: { weight: 2 }
    

    The parsed value is NULL, which can be easily caught and translated into an empty array in the processing function.

    Although the trailing colon is slightly weird, I believe that this would be preferable in terms of DX.

    Thoughts/Opinions?

  2. To advance on (1), definitions for external files would become this:

      js:
        "http://cdn.me/bar.js": { external: true }
    

    The "external" option could be automated even via url_is_external(), but that's a detail.

    That said, I'm not sure whether externally hosted files really mesh so well with the intended architecture of libraries that are shipped/bundled with your code to begin with, because the library is de-facto not shipped with your code. To me, that rather sounds like a job for the contributed Libraries API, CDN, and other modules.

    But anyway, unless we remove support for externally hosted files, their definition would change to the above.

  3. The group: 0 options do not make any sense.

    The values were using PHP constants previously; i.e., JS_LIBRARY, JS_DEFAULT, JS_THEME, etc.

    I can see two potential remedies:

    1. Change the constant values from integers into strings; i.e.,
      const JS_SETTING = 'setting';
      const JS_LIBRARY = 'library';
      const JS_DEFAULT = 'default';
      const JS_THEME = 'theme';
      
      const CSS_BASE = 'base';
      const CSS_LAYOUT = 'layout';
      const CSS_COMPONENT = 'component';
      const CSS_STATE = 'state';
      const CSS_SKIN = 'skin';
      

      That way:

        js:
          js/foo.js: { group: default }
      
    2. Kinda advancing on (1): Change the structure of library.yml files, so as to enforce that a single library can only live in a single group/category:
      library:
        drupal.contextual
          ...
      
      default:
        drupal.contextual-toolbar
          ...
      

      That is, because it scares me that we apparently allow a single library to be multiple things at the same time in the first place.

      Normally, a single asset either is a standalone plugin of its own, OR it supplies integration code, OR it's a theme-related overload → but please not something in between.

      This approach would eliminate all of the remaining group: 0 options of module integration assets. Instead of adjusting the group of individual files, the entire library would be placed into a dedicated group.

  4. In re-establishing the comments as requested in #63, I noticed that some library interdependencies are very fragile:

    1. Some are declaring dependencies in order to establish execution order. (sensible)
    2. But the vast majority declares very funky, small weight: -1 adjustments, without a single word on why those weight adjustments are necessary.

    Even after spending so much time on all existing library definitions throughout core, I still don't know (1) why those adjustments exist and (2) what the final loading/execution order is. :(

    In particular, I'm not able to see how this system can reasonably scale in a modular/extensible application framework like Drupal. I cannot imagine how many contrib/custom modules need to futz with weights of -0.04...

    If you'd ask me, then we should drop all weights (and support for weights) from library definitions.

    Instead, every library should only have two facilities available: Dependencies ("run after these") and Dependants ("run before these").

    That's most likely out of scope for this issue here, but I do want to raise the concern of how fragile our current library definitions are.

The last submitted patch, 64: library.info.64.patch, failed testing.

sun’s picture

Let's ignore the test failures and discuss and focus on the syntax/architecture instead. :)

nod_’s picture

Few things:

Weights should go away with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. So it is a problem and it is being addressed, feedback welcome :)

I don't like the trailing ':' in the proposed change. People working with that stuff are familiar with JSON, and that's just what it looks like in the current patch (and an extra - that makes it look like a list). So to me it's not really a DX improvement.

On the group topic, it's pretty much aggregation groups, and we'd like to let people define their own aggregation groups (to optimize to their use case, have the CKEditor group that you can aggregate with the Edit group or whatever). I agree with giving them a proper name but not declaring the libs under one specific group like that. I guess aggregation groups should really be aggregation 'tags' with some aggregation rules. Essentially that'd make groups kinda useless.

Wim Leers’s picture

#64.1: "Witness: All JS settings are dynamic. → Settings do not have a place in declarative definitions?"

I think you're both right and wrong :)
*Dynamic* settings don't have a place in declarative definitions. So I agree that what editor.module and ckeditor.module do is intolerable. However, static settings are not problematic. One could argue that if the settings are static, then what is the point of having them at all; why not hardcode them in the JS? Because it might be easier to override them that way. But even then, contrib modules could just optionally provide such a setting, that doesn't mean the library must use them; core could just "hardcode" the defaults and just check if a certain setting is defined, and if so, use that instead.
So: dynamic settings are intolerable in library definitions, static settings are tolerable, but their usefulness is debatable.

In any case, I've rolled patches to right the wrongs you've found: #2161759: Remove unused drupalSettings.ckeditor.modulePath setting and #2161763: Remove unnecessary drupalSettings.editor.getUntransformedTextURL setting :) Please review.

So, I completely agree with

it's long overdue that the originally intended architectural design of static declarations is finally enforced


Thanks for explaining your "external libs belong in core.libraries.yml" rationale. It's very reasonable :) +1


  1. Like nod_, I don't like the proposed trailing colon. But what about just assigning the empty hashmap? So: js/foo.js: {}. That clearly says "no custom options for this one, please".
  2. Good point to question external libraries in Drupal library definitions. But let's tackle that in another issue to keep the scope small. Unless it becomes a blocker.
  3. In the spirit of minimizing scope creep, I think it's best to not dive into your second suggestion. I think the first remedy is solid: keep the same API, just use strings instead of numerical values.
  4. Yes yes yes yes! F'ing weights. Must. Die. And as you say, the worst part is not the "WTF is that doing there?" but the "how the F am I supposed to understand how everything is working together". That absolutely kills the front-end DX.

    stead, every library should only have two facilities available: Dependencies ("run after these") and Dependants ("run before these").

    Funny you say that. That's precisely what sdboyer has been working towards with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets :) You should talk to him! I've been a sounding board for him for that issue, and I think nod_ has been as well, as has been catch. So, I think we're all on the same page: our asset dependency system is very convoluted, fragile (extremely!) and frustrating.
    It's late in the cycle, but sdboyer has buy-in from catch, and he should be able to land that patch without API changes and just getting rid of weights.
    We already have the "this must run after that other library" relationship implicitly (i.e. define dependencies), but we don't yet have the "this must run before that other library" relationship. That's what the "weight: -1" stuff is for, usually. That might require an API addition.
    (Whenever I introduced such a case (once or twice I think), I've made sure to document it. Sadly, that hasn't always happened, and in those cases it's not understandable.)

IIRC, this patch would help #1762204. If we (sun, sdboyer, nod_ and I) could put our combined shoulders under this and #1762204, I think we stand a good chance of landing both and massively improving the front-end DX :)

damiankloip’s picture

Great work sun, this is looking good. Here is a reroll to keep things moving. Attached an interdiff as a few updates were needed.

The last submitted patch, 69: 1996238-69.patch, failed testing.

damiankloip’s picture

FileSize
175.67 KB
687 bytes

Whoops, sorry.

The last submitted patch, 71: 1996238-71.patch, failed testing.

sun’s picture

FileSize
175.45 KB
3.72 KB

Fixed merge conflicts.

Thanks for the re-roll attempt :) Note that due to the massive amount of changes, this patch is maintained in the library-info-1996238-sun branch of the Platform sandbox.

damiankloip’s picture

Meh, sorry. Using the sandbox would have been easier for sure.

The last submitted patch, 73: library.info.73.patch, failed testing.

klonos’s picture

Status: Needs review » Needs work
sun’s picture

Status: Needs work » Needs review
FileSize
176.71 KB
2.33 KB

Fixed hook_editor_js_settings_alter().

With that, we should be back to "square two"... Given how painful this re-roll was after just a short period of time, I wonder whether we shouldn't move the Service-ification & Co into a separate follow-up issue, and instead, just try to add some minimal database caching here as a quick & dirty temporary measure?

klonos’s picture

;)

sun’s picture

FileSize
179.18 KB
6.43 KB

Added naive persistent database cache for library definitions.

jibran’s picture

sun’s picture

FileSize
179.25 KB
51.21 KB

Remaining issues I'd like to tackle & try here:

  1. Change declaration format, moving source file into key, only have options in the hashmap.
  2. Remove 'external' as source declaration key; only keep the 'external' boolean option.
  3. Move settings into new + dedicated 'settings' declaration key.
  4. Update hook_library_info_alter() with proper example.

See attached patch. I think these final improvements make a lot of sense.

Clean-ups I'd like to move into one or more follow-up issues:

  1. Change 'group' from integers to human-readable strings. → Integers are used as priorities for processing aggregates, so their "weights" get lost, which will need a proper solution. :-/
  2. Add a new drupal_add_settings(), so as to improve DX of library definitions and have #attached[settings]. → Stop mixing a completely different data format structure into #attached[js].

Unless there are any major review issues or objections, I'd consider this patch as complete and would recommend to move forward with a commit here.

The last submitted patch, 81: library.info.81.patch, failed testing.

damiankloip’s picture

  1. +++ b/core/includes/common.inc
    @@ -2753,38 +2753,37 @@ function drupal_add_library($module, $name, $every_page = NULL) {
    +  if (!isset($libraries[$extension]) && ($cache = cache()->get('library_info:' . $extension))) {
    

    Is there a reason (it was doing this before too) why we invoke library info on a per extension basis, and not just cache all library info as one cache item?

  2. +++ b/core/includes/common.inc
    @@ -2839,17 +2845,17 @@ function drupal_get_library($module, $name = NULL) {
    +    cache()->set('library_info:' . $extension, $libraries[$extension], CacheBackendInterface::CACHE_PERMANENT, array('library_info'));
    

    we can probably change this whenever, but I think we could improve this tag, also, the format probably needs to be changed to be inline with other tags.

  3. +++ b/core/includes/module.inc
    @@ -112,6 +112,13 @@ function system_list_reset() {
    +  cache()->invalidateTags(array('library_info'));
    

    You should use the Drupal\Core\Cache\Cache::invalidateTags() method here instead. As that will currently only invalidate tags in the default bin. If people switch cache backends, we will need that change.

    Hopefully we can fix the whole cache tags/bins separation before release though :/

sun’s picture

Issue tags: +Avoid commit conflicts
FileSize
180.1 KB
2.58 KB
  1. Fixed merge conflicts.
  2. Fixed cache tag usage.

As this is a pain to merge and re-roll, I hope I'm allowed to tag this issue.

why we invoke library info on a per extension basis, and not just cache all library info as one cache item?

That's taken over from the existing code. I'm not opposed to changing that, but to do so, we'd have to change drupal_get_library() to scan + discover + process all available (enabled) extensions in the system, which I'd like to defer to a follow-up issue (+ service-ification).

The follow-up issue will allow for nice cleanups. I just didn't want to creep the scope further here, as the patch is large enough already + kinda hard to re-roll in case of conflicts...

The last submitted patch, 84: library.info.84.patch, failed testing.

sun’s picture

FileSize
180.94 KB
1.18 KB

Updated locale_library_info_alter() for new library definition structure.

...but wait. On a second thought... the manipulations of that hook implementation depend on the current page/request context; i.e., they cannot be persistently cached.

Hm. I think this gets us back to a major shortcoming of our current library system: Modules (or themes) that want to dynamically "attach" something to a library, but only when the library is used, do not have a hook, event, or proper integration point right now — except of introspecting all contents of hook_js_alter() on all pages + conditionally acting there. :-/

The common abstracted use-case is:

If you're going to add library foo/bar, then let me either (1) adjust what you'll be adding on-the-fly or (2) add my additional library on-the-fly or (3) add/override some context-specific settings on-the-fly.

In essence, a new hook_library_alter() (sans _info), which is invoked by drupal_add_library() like this:

function drupal_add_library($module, $name, $every_page = NULL) {
...
    if ($library = drupal_get_library($module, $name)) {
      // Allow modules to dynamically attach further assets for this library.
      drupal_alter('library', $library);
...
      $elements['#attached'] = array(
        'library' => $library['dependencies'],
        'js' => $library['js'],
        'css' => $library['css'],
      );

I really hoped that we could limit the scope of this issue to the changes in the current patch, but I fear we have to resolve this additional problem space in order to not break such existing use-cases.

The only alternative to the above would be to execute hook_library_info_alter() on the cached + preprocessed library definition on every page request. However, I do not like that idea, because it distorts a clean separation of statically cached declarations vs. context-specific operations. I also dislike that manipulations of static data would repetitively happen on every single request instead of being cached in the static/declarative info already (e.g., jquery_update module would swap out the library on every single request, instead of doing it once and done).

Thoughts?


I'm not able to reproduce the Views test failures locally, so I assume that was testbot fluke.

moshe weitzman’s picture

The only alternative to the above would be to execute hook_library_info_alter() on the cached + preprocessed library definition on every page request. However, I do not like that idea, because it distorts a clean separation of statically cached declarations vs. context-specific operations. I also dislike that manipulations of static data would repetitively happen on every single request instead of being cached in the static/declarative info already (e.g., jquery_update module would swap out the library on every single request, instead of doing it once and done).

I personally don't mind this pattern at all. I think it can perform well, and serve the purpose nicely. We added a variant of this pattern recently to the render system with #post_render_cache which is where callbacks can replace tokens in cached data or tale other dynamic actions.

sun’s picture

FileSize
183.23 KB
4.07 KB

Added hook_library_alter() to perform request-specific adjustments for an attached library.

I'd like to clarify that this does not only resolve the concrete problem space here — I encountered this limitation in the past already; IIRC, for Libraries API and similar modules, having the shared use-case of needing to act when library X is attached, and which currently have to use very complex hook_js_alter() implementations to achieve the desired operation. In other words, this resolves multiple issues at once.

The last submitted patch, 86: library.info.86.patch, failed testing.

The last submitted patch, 86: library.info.86.patch, failed testing.

The last submitted patch, 88: library.info.88.patch, failed testing.

sun’s picture

88: library.info.88.patch queued for re-testing.

Wim Leers’s picture

Add a new drupal_add_settings(), so as to improve DX of library definitions and have #attached[settings]. → Stop mixing a completely different data format structure into #attached[js].

This is also something sdboyer, nod_ and I have discussed several times now. The piggybacking of "JS settings" into the "JS asset type" causes many problems, both technical and DX. So: yes, absolutely, we need to stop mixing those! That's of course out of scope for this issue, but I just wanted to confirm that you're not the only one thinking that :)

Patch feedback:

Amazing progress here, thanks sun!

sun’s picture

  1. The term extension is already used in core and we're steadily seeing more usages of it, the more we're completing the extension system conversion.
  2. I think we want to retain the Editor API adjustment to allow editors to have global JS settings, as that is a good idea either way? Given how troublesome it is to re-roll this patch, it would be much easier to remove those two unnecessary settings after this patch has landed.
  3. Hm. — The 'external' => TRUE definition is simply converted into the existing 'type' => 'external' option, like the old code did. The code in drupal_get_library() is barely testable in its current form, and in fact, this patch does not have to significant test changes, which means that there is no proper test coverage for hook_library_info() yet.

    If possible, I'd like to defer the addition of proper test coverage to the follow-up issue that will hopefully convert drupal_get_library() into a classed service (as well as possibly using a single cache item for all extensions as mentioned in #84), which in turn should allow us to properly test its class methods.

  4. Regarding changing 'group' from integers to human-readable strings. → The integers are used as priorities for processing aggregates, so their "weights" would get lost. I don't really want to futz with that asset aggregation/grouping logic in a patch that is 180KB already... Would it be acceptable to tackle that in a follow-up issue?

Thoughts?

Wim Leers’s picture

  1. Okay :) I guess I just haven't seen that.
  2. If we were to retain EditorPluginInterface::getGlobalJSSettings(), then it'd need test coverage. Plus, because it's *global* settings, it should not receive an Editor entity.
    However, I honestly don't see the value of having that method. There's not a single use case I can think of that was impossible before or that this simplifies significantly (in fact, the only use case in core for it no longer applies). All it does in my opinion, is add complexity, because the result is that each text editor can return JS settings from two methods, and it's unclear how they are related: getGlobalJSSettings() returns a "top level" JS settings array, getJSSettings" returns a drupalSettings.editor.formats.<text format ID>.editorSettings-level JS settings array.
    I strongly disagree that this is an improvement.
  3. There is some test coverage in JavaScriptTest, but it's "incomplete" to put it nicely :P. Therefor I think it's okay to defer adding test coverage for this, especially considering your further plans. Also: chances of regressions are low and impact on the rest of core development would be low too.
  4. I'm confused. You said yourself in #64.3 that this is one of the remaining TODOs. Having to write strange numbers instead of strings is significant DX regression. As much as I'd like to agree it can be tackled in a follow-up, I don't think it can?
Wim Leers’s picture

FileSize
1011 bytes

Currently I'm working on other core patches while this patch is applied, so that I can test it thoroughly. It looks like the resulting order of assets in the HTML is different, though AFAICT not in a breaking way. Ideally, it'd be identical though.

So far, it's working beautifully!

… except in case: the edit_stylesheets functionality — the changes to edit_library_info_alter() and _edit_theme_css() broke the code. Here's a patch that fixes that, which you can apply to your sandbox.

webchick’s picture

Status: Needs review » Needs work

Does not apply.

sun’s picture

Status: Needs work » Needs review
FileSize
181.34 KB
14.4 KB

Sorry for the silence. (was extremely busy elsewhere…)

  1. Fixed merge conflicts caused by #2161759: Remove unused drupalSettings.ckeditor.modulePath setting and #2161763: Remove unnecessary drupalSettings.editor.getUntransformedTextURL setting.
  2. by @Wim Leers: Updated edit_library_info_alter() + _edit_theme_css() for new library definition structure.
  3. Remove 'group' entirely to let the library system be what it is: An asset dependency management system.
  4. Reverted EditorPluginInterface::getGlobalJSSettings() for now, even though that should exist.

Come to think of it, (3) is the one and only valid and solid answer to the topic of "groups":

There are no groups. Define your dependencies instead, kthxbye. :)

Regarding (4) global editor JS settings, I'm actually very sure that this facility is required and should exist. It's particularly useful for editors/plugins that require a massive amount of JS settings/metadata to operate — which would have to be duplicated in drupalSettings for every single text format otherwise. To resolve that, we invented a global → editor → format settings merge/override concept in Wysiwyg — primary purpose being to reduce page size bloat caused by d(r)uplicated drupalSettings. We might not have the use-case in core, but I'm confident that editors/plugins supplied by contrib will run into the problem space.

Given this update - in case it passes, attached patch might actually be in a commit-ready state.

Of course, we'll have to review + confirm that all libraries are properly defining all of their dependencies. However, I think the only realistic way to do that is to commit this patch and afterwards see whether any libraries are loaded too late or too early.

Upfront, I can already tell that the dependency resolution logic of attached libraries does not appear to be correct — for example, the JS part of the seven/install-page library being used in the installer does (or did) not declare any dependencies (which is correct, because it's raw JavaScript), but yet, it is loaded in the middle of nowhere. Thus, the processing and dependency resolution of all attached libraries needs some love. However, that code is not touched at all here, so that is a separate issue. That said, @sdboyer's Assetic patch might eliminate and resolve that entirely (didn't have time to look at that yet).

The last submitted patch, 98: library.info.98.patch, failed testing.

klonos’s picture

Status: Needs review » Needs work
sun’s picture

Status: Needs work » Needs review
FileSize
181.39 KB
404 bytes

Fixed core/drupal.vertical-tabs[.js} must load before core/drupal.collapse[.js].

nod_’s picture

Status: Needs review » Needs work

I'm not a fan of the new syntax. I'm pretty sure people will forget the : {} — every single dev will have to write a lib file at some point — and wonder why things are breaking. When it breaks the error message is not helpful:

    Warning: Invalid argument supplied for foreach() in drupal_get_library() (line 2809 of core/includes/common.inc).
    Warning: Invalid argument supplied for foreach() in drupal_add_library() (line 2712 of core/includes/common.inc).
    Warning: Invalid argument supplied for foreach() in drupal_process_attached() (line 2508 of core/includes/common.inc).

Anyway, I can live with it and it's just a detail but I wanted to voice my disagreement.

Something missing but that could be a follow-up is the dependency declaration and library attach difference:

// In *.library.yml
dependencies:
  - core/normalize

// In php files
$page['#attached']['library'][] = array('core', 'normalize');

// What would be better
$page['#attached']['library'][] = 'core/normalize';

It'd sucks to do in an already painful patch but I want to make sure we agree that's the end goal.

Views UI is pretty busted though :( in core/drupal.ajax the weight needs to be removed, that helps.

And the rest of the styling issues comes from the fact that the CSS is pretty much in the reverse order of what it should be. This is going to be a tricky one.

Other than that, edit, wysiwyg, toolbar, contextual links, autocomplete & co works fine.

sun’s picture

Status: Needs work » Needs review
FileSize
183.55 KB
21.95 KB
  1. Fixed positive weights for JS files (are breaking Views UI) → globally reject positive weights.
  2. Introducing built-in SMACSS categories for CSS file assets in library definitions.

(2) globally fixes the order in which CSS files are being loaded.

It does not fix the fact that drupal_add_library() directly processes + attaches a requested library and its dependencies whenever it is invoked - which means that it is only aware of the immediate libraries that it happens to deal with for a particular invocation, instead of properly accounting for all interdependencies between all libraries (once) when an HTML page rendered. That topic is left for #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

However, by introducing SMACSS categories into our library definitions, we're able to mitigate a large part of that problem space, because (1) we can apply the correct weights and (2) we still have a limited amount of dependency information to work with.

For the SMACSS category names, I followed the naming of our existing CSS_* constants (in common.inc), which seem to be in line with our formalized D8 CSS architecture documentation. — Apparently, you will notice that this documentation page contains some code blocks that show the exact same structure → Profit! :)

For the SMACSS category classification of individual CSS files though, absolutely none of our existing documentation helped me to make an informed decision on how to classify them. :( — For most CSS files, I used "component". And for all files with ".theme.css", ".admin.css", and ".maintenance.css", I used "theme". I also used "state" one or two times, even though that category totally wasn't clear to me.

Based on manual testing (and some quick introspection of CSS style overrides via Firebug), this appears to fix all of the ordering bugs (including Views). That said, Views UI and dropbuttons are both looking a bit odd in Bartik, but then again, I'm not sure whether that isn't expected; our entire admin UI seems to be styled in Seven only... :-/

The last submitted patch, 103: library.info.103.patch, failed testing.

effulgentsia’s picture

Status: Needs review » Needs work

bot fail, so needs work.

sun’s picture

Status: Needs work » Needs review
FileSize
183.6 KB

Merge branch '8.x' into platform/library-info-1996238-sun. Simple diff context conflict, so no interdiff.

nod_’s picture

Views UI works fine. The rest still works properly as well. That's a RTBC to me :D

sun’s picture

FileSize
186.35 KB
3.75 KB
  1. Fixed Edit's library manipulations (again). → Convert into hook_library_alter() to prevent configurable admin theme data from being cached in static library info.
sun’s picture

I believe this is finally ready to be committed now.

The 'group' issue has been resolved in a much cleaner way.

The only remaining issue is to convert all manual uses of #attached['library'] = array('provider', 'name') into #attached['library'] = 'provider/name', but @nod_ and I discussed that it should be fine to do that in a separate follow-up issue.

webchick’s picture

Please update the issue summary so when this gets RTBC a core maintainer can have an easier time getting up to speed on 100+ comments. :)

I also note this is a huge API change, though I don't see where it was explicitly run by a core maintainer for approval, so not giving it the "Approved API change" tag just yet.

sun’s picture

Added a proper issue summary, which is hopefully complete. Reviews/edits welcome. :)

Super-minor: In case I have to re-roll this monster once more, it would be good to add a comment to the top of core.library.yml to clearly state that all libraries are declared in alphabetical order (which is something that seriously drives me nuts in our current service.yml + routing.yml files...)

nod_’s picture

Since it was mentioned a while back diffstat might be an argument for a change :) 106 files changed, 1792 insertions(+), 2643 deletions(-)

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Everything still working fine for me and Issue summary up to date.

Gotta love the new DX :D

jessebeach’s picture

I think this is a real win for DX. No need to edit PHP to introduce assets and dependency chains. Super work!

damiankloip’s picture

I agree this patch is looking great now.

The only thing is that I do not think the cache tags are sufficient. I think the extension tag is a really good idea, and I would like to work on introducing that in more places. I think we still need the 'library_info' tag though. As extension (and extension:name) is pretty generic.

We have already talked about making a single cache entry for this data etc.. in previous comments here, so we could tackle this in that follow up?

longwave’s picture

Should we have consistency/standards for singular vs plural nouns? Just wondering why this is the singular *.library.yml and not *.libraries.yml, when we have plural *.services.yml, *.local_actions.yml and *.local_tasks.yml.

aspilicious’s picture

#116 has a good point

tstoeckler’s picture

hmm... we also have routing.yml instead of route.yml or routes.yml which totally falls out of line. With module name we fairly consistently stick to the singular (i.e. filter.module instead of filters.module) so if this were the first yml file I'd say it would be more consistent to go with the singular, but the examples in #116 are a pretty strong precedent for the plural.

One additional note: In D8 contrib Libraries API is likely to have *.libraries.yml files, so if core were to go with libraries.yml as well we might have to think about a rename of the module.

sun’s picture

FileSize
186.7 KB
10.06 KB

Thanks for the additional feedback! Happy to quickly adjust that here prior to commit:

  1. Polish parsing logic for asset files that are explicitly marked as external.
  2. Added note to top of core.library.yml about alphabetical order.
  3. Added 'library_info' tag to cache items.
  4. Renamed *.library.yml into *.libraries.yml.
klonos’s picture

Title: Replace hook_library_info() by *.library.yml file » Replace hook_library_info() by *.libraries.yml file

...also title update then.

Wim Leers’s picture

  1. all libraries are declared in alphabetical order (which is something that seriously drives me nuts in our current service.yml + routing.yml files...)

    Hehe — glad to see I'm not alone :)

  2. +++ b/core/includes/common.inc
    @@ -2806,12 +2806,32 @@ function drupal_get_library($extension, $name = NULL) {
    +            throw new \UnexpectedValueException("The $extension/$id library defines a positive weight for '$source'. Only negative weights are allowed (but should be avoided). Instead of a positive weight, specify accurate dependencies for this library.");
    

    Explicit failure… THANK YOU!

  3. +++ b/core/modules/edit/edit.module
    @@ -46,17 +46,52 @@ function edit_page_build(&$page) {
    +      $theme = Drupal::config('system.theme')->get('admin');
    

    Ridiculous nitpick: AFAIK we always use \Drupal, never Drupal. But that's such a ridiculous nitpick that it shouldn't need to be rerolled. This is not the only place where this exists.

  4. At first, I wasn't sure that the explicit presence of SMACSS metadata in library declarations (introduced in #103) would be a good idea: it's an extra step. But then I realized that I was being a moron: that explicit presence is a great thing, because now everybody who writes CSS will actually have to think about this metadata (this metadata was pretty much always missing in contrib), and hopefully write better structured CSS because of that! Less cowboy wild west, more simplicity & structure.
    The only somewhat important thing that I'm missing, is test coverage for a sample YML file that contains all possible kinds of SMACSS categories.

Like Jesse said: this is a huge DX improvement.

It doesn't matter if this breaks some things in Drupal core — as sun already said: that's the only way to find out, and it's probably the case because the previous library definition already was incomplete/incorrect. It cannot break core in a major way (tests are green and quite extensive manual testing was done). It might break some things in core in a minor way. But that's definitely worth any follow-ups, in my opinion. Between sun, nod_, I and others, we should be able to fix those quickly.

I think this is RTBC, but I want to make sure that sun saw tstoeckler's rationale for having *.library.yml rather than *.libraries.yml, which is what's in the latest patch. I'm fine with either.

nod_’s picture

star-szr’s picture

Issue summary: View changes

Replacing library.yml with libraries.yml everywhere but the original report in the issue summary.

longwave’s picture

hmm... we also have routing.yml instead of route.yml or routes.yml which totally falls out of line

I think routing.yml is inherited from Symfony, whereas services, local_tasks and local_actions are Drupalisms.

sun’s picture

re: @Wim Leers: SMACSS category test coverage:

Yes, neither the old code nor the new code validates the structure, syntax, and format of library definitions. As mentioned in the issue summary, the plan is to introduce test coverage for this code in the follow-up issue that turns it into a service.

That is, because as soon as we have a separate LibraryHandler::processDefinition() method, we're able to cleanly unit test that function without any files involved.

Also, I personally expect that the objectified parsing code will validate all data structure and data type requirements of each processed definition and that it will throw many more exceptions.

re: libraries.yml vs. Libraries API:

I thought through this before making the change. (1) libraries.yml definitely makes more sense, because the file declares multiple libraries and not just one. (2) The plural form is consistent with other declaration files. (3) I'm skeptical whether it will conflict with Libraries API in the first place, because the libraries.yml files introduced here are located in each Drupal extension, whereas the potential declaration files of Libraries API would live in the /libraries directory, since the whole point of that Libraries API idea/concept is to detach the declaration of external libraries from any particular extension.

In any case, I'm confident that Libraries API will be able to cope with it.

sun’s picture

119: library.info.119.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 119: library.info.119.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
188.5 KB
1.51 KB
  1. Merge branch '8.x' into library-info-1996238-sun
  2. Fixed merge conflicts.
Wim Leers’s picture

FileSize
188.52 KB
388 bytes

Reroll for the changes made in #1636992-19: form.js' formUpdated event is unreliable/incomplete. Necessary, because otherwise almost every form will break with a JS error.


  1. My two concerns in #121 are addressed: additional test coverage will be provided in a follow-up (as unit tests, yay), sun *did* see tstoeckler's rationale.
  2. Manually tested again: create/edit node, in-place editing, toolbar, contextual links, Views. Did not find anything broken (besides the super minor fix in this reroll).

Hence: RTBC +1.

Thanks sun! :)

sun’s picture

Status: Reviewed & tested by the community » Postponed

I'm fairly sure that this no longer applies, since #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit was reverted. That commit was the cause for the second to last re-roll in #128.

I don't understand why #2047633 was reverted, given that there was a follow-up issue to fix the regressions already. It doesn't make sense to re-roll back/forward two monsters of this size and have them compete with each other.

Therefore, I'm not going to roll this back to #119 (but yet including the merge conflict fixes from #128 + #129).

We're going to wait for #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit

sun’s picture

129: library.info.129.patch queued for re-testing.

Status: Postponed » Needs work

The last submitted patch, 129: library.info.129.patch, failed testing.

sun’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 133: library.info.133.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
188.63 KB
572 bytes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: library.info.135.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Testbot fluke.

sun’s picture

135: library.info.135.patch queued for re-testing.

sun’s picture

135: library.info.135.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 135: library.info.135.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
188.63 KB

Clean merge.

Xano’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
188.79 KB

Re-roll due to path.module.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Double-checked that the re-rolled patch matches my local merge. (It matches, except for a strange commit message.)

Xano’s picture

The commit message is because I did git show.

alexpott’s picture

Priority: Normal » Critical
Issue tags: +beta blocker
FileSize
188.63 KB

This needed a reroll because of stuff I've committed... so here is the reroll. Also I'm raising the priority and tagging appropriately.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

I do not understand why we literally cannot release Drupal 8 without this patch, so not sure I understand the critical distinction. Nevertheless...

Just as a general patch scoping comment, there's a *ton* going on in this patch that really would've been better to split out separately, IMO. For example, just simply changing PHP arrays => YAML pretty is non-controversial. Probably so is moving the scope from system module to core. But then this is coupled as well with also renaming arbitrary elements of that info (website -> remote), forcing SMACSS groupings for CSS, etc. and suddenly this turns it into something that's incredibly hard to review, and is therefore going to need 3-4 people to do so, each on their own parts. :(

To be committable, this really needs more review from front-end devs in my opinion, especially themers. This change will disproportionately affect them vs. module developers. I'm sure YAML vs. PHP arrays is a no-brainer, but the specific syntax and groupings here could use a weigh-in by someone with more experience on that level. (So e.g. Lewis Nyman, John Albin, Cottser, that sort of folk.) I also see sdboyer talked about in this issue, but I don't see him actually commenting here, and it seems like his opinion would be good as well, to ensure it's inline with his work on the asset system. Statements like "will likely be consumed by" make me nervous, because we certainly don't want to do this work twice.

Eyeballing it DX-wise, one comment I can say is that I don't understand the use of curly braces here.

+  js:
+    assets/vendor/classList/classList.min.js: { weight: -21, browsers: { IE: 'lte IE 9', '!IE': false } }

Elsewhere when we have deeply nested information, we just put each element on its own line, ala config schema files or what have you.

And it especially looks nasty when it's all by its lonesome, like so:

+maintenance_page:
+  version: VERSION
+  css:
+    theme:
+      css/maintenance-page.css: {}
+  dependencies:
+    - system/maintenance

...what's up with that? The issue summary doesn't explain.

That "dependencies" stuff also looks really problematic, at least to me. All the Bartik theme is doing here is adding a CSS file called maintenance-page.css that happens to override some of the defined maintenance styling in core (though it could also override other styling in core). So in order to do so, now instead of just providing a (granted, convoluted and scary-looking) syntax to say "here's my CSS, please add it to the page," you also now need to also understand the entire family tree of how that CSS you're overriding got to be on that page to begin with.

But how... exactly do you do that, as a theme author? find . -name maintenance-page.css only turns up Bartik's. System module's styling is called system.maintenance.css, how do you know that from here? Even grepping for e.g. ".maintenance-page" didn't turn up any clues.

Normally, what you do as a theme author is whip out Firebug or whatever the tool du jour is, hover over the thing that doesn't look right, and add whatever IDs/classes/rules you need to make it look right. Find the next thing, repeat, etc. Now, your workflow turns into, "inspect the element and pay super careful attention to the filename it comes from" (and hope to $diety that CSS aggregation isn't turned on, or you are completely screwed), then grep for the filename to find the libraries.yml file where it's defined, THEN find its machine name in its libraries definition and add that to your "dependencies" array, and finally move onto the next broken piece." That totally breaks theme dev's workflows, or at least the theme dev workflows I've been part of.

So yeah, colour me concerned. :\ I'd be happy to be told that I'm worrying over nothing, but let's get some themers in here and see what they say.

Danny Englander’s picture

@webchick, thanks for chiming in, as a themer I've been following this for several months now after I ran into #2147021: jQuery.once does not get loaded for anonymous users while porting one of my contrib themes to Drupal 8. I was subsequently directed here. I tested this patch while hanging out at SandCamp but did not have any luck with said contrib theme. By the time I came back to report my findings, the issue had been changed to "needs work" at that point so I figured I would give it a wait and let it play out some more. I hope to find some time this week to retest as I do get a little contrib time from my company. I think it's a good idea for themers to get more involved with this. I think it's a pretty important issue.

As it happens, I got my theme working fine with all the nested arrays but it did take a bit of work. I think having this as YML would be nice to lessen some headaches.

jibran’s picture

Well @webchick is right that it is not a beta blocker. But it is at least major if not critical.

it did take a bit of work

If it is a bit of work for themers then it is fine. Because for module developers we have a lot of work :D (routes, services, events and etc ;)).

I think other then themer's feedback we are done here in terms of development so can we please have a drafted change notice so that themers/module developer can understand the changes more easily and it is also a brand new core gate. I don't think it is that big of an issue as @sun has written brilliant issue summary.

And finally thank you everyone who worked on this and made this huge improvement.

alexpott’s picture

Priority: Critical » Major
Status: Needs review » Needs work
Issue tags: -beta blocker +beta target

Setting the issue status appropriately.

Also if the is to be the big DX win hoped for then the error message for a broken YAML needs to be improved. Currently it is:

Error message

Symfony\Component\Yaml\Exception\ParseException: Unable to parse at line 8 (near "dependencies"). in Symfony\Component\Yaml\Parser->parse() (line 246 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php).
The website has encountered an error. Please try again later.

Let's catch the ParseException and emit something a bit more helpful that includes the name of the file being parsed.

alexpott’s picture

Issue tags: +Needs change record

As per #148 we need a draft change record before commit https://groups.drupal.org/node/402688

Also as a potential followup:

+++ b/core/includes/common.inc
@@ -2747,62 +2747,170 @@ function drupal_add_library($module, $name, $every_page = NULL) {
+      $parser = new Parser();
+      $libraries[$extension] = $parser->parse(file_get_contents($library_file)) ?: array();
+
+      // Allow modules to alter the module's registered libraries.
+      drupal_alter('library_info', $libraries[$extension], $extension);

Blind acceptance of what is in the file - perhaps we can add validation to check that required keys are present.

sun’s picture

Status: Needs work » Needs review
FileSize
189.26 KB
1.97 KB
  1. Rethrow a more helpful exception in case of a Yaml ParseException.
  2. Added basic validation for the primary 'js' and 'css' library definition keys.

I've the impression that the majority of concerns raised in #146 are lacking context with respect to how front-end assets and libraries are being handled in HEAD already. If anything, then the proposed change here only brings the developer-facing API much closer to the intended architecture that already exists in D8 HEAD already.

Yes, since the library definitions are converted into declarations, some unnecessary properties were removed (human-readable library label/title) and some properties were renamed/re-purposed ('website' → [git] 'remote' repository URL). However, a conversion from interpreted/executed PHP code into a static declaration has to address additional details:

The CSS_* PHP constants are no longer available in YAML. Several options were discussed in #64. But their usage in a dependency-managed library system does not make sense in the first place. Only a few libraries are defining a custom 'group' for CSS files in HEAD and all of those instances actually present hacks to work around an apparently broken CSS file asset grouping + ordering problem that already exists in HEAD, which has been introduced by the global conversion from direct drupal_add_css() calls to properly defined libraries (again, all of this is in HEAD).

The lack of CSS grouping information for all CSS asset files is the root cause for those one-off 'group' adjustments. I'm 100% confident that every single of those instances required a dedicated core issue and plenty of manual testing to ensure that the resulting CSS files are actually loaded in the right groups and order. So contrary to the concerns being raised, specifically for the purpose of DX, and specifically for the purpose of not having to change library declarations once more, explicit SMACSS category groups are now an inherent part of the library definition itself.

SMACSS category grouping has been officially and formally adopted for Drupal 8. The existing CSS architecture documentation page explains their purpose and usage, which is 100% in line with the declarations of this patch.

All libraries are declared in YAML, so any valid YAML is fine. It does not matter whether you are using the YAML inline syntax for hashmaps or the more verbose multi-line syntax to specify asset file options. An empty options hashmap is denoted with {} in YAML.

Every library defines a list of its own dependencies. As the author of a library, you always know what your (direct) dependencies are. If you declare a CSS library that you load yourself in your theme and which intends to override System module's maintenance CSS, then that system library is your dependency. However, this entire concept is not introduced here, it exists in HEAD already and is the agreed-on plan for D8, the decision was made already (for good reasons).

alexpott’s picture

Status: Needs review » Needs work

As it stands this patch will prevent us from using the Pecl Yaml parser - see #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available since it uses dots in the node anchors.

jquery.ui:
  version: &jquery.ui 1.10.2

jquery.ui.accordion:
  version: *jquery.ui

Will not work on Pecl.

jquery.ui:
  version: &jquery_ui 1.10.2

jquery.ui.accordion:
  version: *jquery_ui

Will work on both Symfony and Pecl

sun’s picture

Status: Needs work » Needs review
FileSize
189.26 KB
6.83 KB

That's perfectly fine, since the node references are limited to the local scope of a particular file anyway (i.e., they are just maintenance helpers and not a public API; they only exist within the scope of core.libraries.yml).

→ Use 'jquery_ui' instead of 'jquery.ui' as node reference anchor.

Status: Needs review » Needs work

The last submitted patch, 153: library.info.153.patch, failed testing.

sun’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
189.39 KB

Updating example in API changes section of issue summary accordingly.

Merged 8.x.

jhodgdon’s picture

@sun: I went on a committing spree (with your permission) and committed a bunch of changes that touched some of the same files as this patch (but nothing touching hook_library_info()).

The latest patch still applies for me using the patch command but not using git apply, so you might want to reroll.

Thanks for the encouragement -- a lot of those issues had been waiting at RTBC for several weeks at least.

sun’s picture

FileSize
190.46 KB
948 bytes

Fixed merge conflicts.

Status: Needs review » Needs work

The last submitted patch, 157: library.info.157.patch, failed testing.

klonos’s picture

...we should really get used to hiding previous, obsolete patches/files when uploading new ones ;)

alexpott’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
192.87 KB
3.47 KB
  1. Fixed merge conflicts.
  2. Fixed RenderElementTypesTest.
klonos’s picture

...the checkboxes to hide the old files are right there when you upload the new ones. It only takes a couple of extra clicks before hitting the save button.

Wim Leers’s picture

  1. +++ b/core/includes/theme.inc
    @@ -2417,8 +2417,6 @@ function template_preprocess_maintenance_page(&$variables) {
    -  $default_css['library'][] = array('system', 'base');
    -  $default_css['library'][] = array('system', 'admin');
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
    @@ -26,6 +34,12 @@ public static function getInfo() {
    @@ -173,11 +187,9 @@ function testMaintenancePage() {
    
    @@ -173,11 +187,9 @@ function testMaintenancePage() {
         $path = drupal_get_path('module', 'system');
         $default_css = array(
           '#attached' => array(
    -        'css' => array(
    -          $path . '/css/system.module.css',
    -          $path . '/css/system.admin.css',
    -          $path . '/css/system.maintenance.css',
    -          $path . '/css/system.theme.css',
    +        'library' => array(
    +          array('core', 'normalize'),
    +          array('system', 'maintenance'),
             ),
           ),
         );
    

    I think you're essentially fixing a bug here, right? That's what the second highlighted hunk suggests.

    If so: *great* catch! :)

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
    @@ -230,7 +242,7 @@ function testMaintenancePage() {
    -    drupal_static_reset('_drupal_add_css');
    +    drupal_static_reset();
    
    @@ -248,7 +260,7 @@ function testMaintenancePage() {
    -    drupal_static_reset('_drupal_add_css');
    +    drupal_static_reset();
    

    Can you explain why we suddenly need to clear *all* statics?

sun’s picture

re: #163.2:

This test asserts the fully themed page output of the maintenance page, which involves much more than just stylesheets — it also involves scripts and HTML HEAD elements (like meta tags). Each of the corresponding functions are using a drupal_static() cache. If those caches are not cleared/reset, then the consecutively rendered maintenance pages are missing the corresponding elements. → This test only passes in HEAD due to sheer luck and the coincidence of fragile function call chains and test assertions.

(I actually had to invent a new "verbose diff of $expected vs. $actual output" functionality in TestBase to debug and understand what's going on, for which I will create a separate issue, 'cos that was pretty fancy to have :-)).


This patch has been sitting in "needs review" for ages now and was RTBC before. We've reached out to various people already. By now I can only assume that the silence is to be interpreted as "Nice!" or at least "No objections."

Can we at least put it back into the RTBC queue, in order to increase the pressure for raising objections (if any) and clarify that - at minimum - the front-end related subsystem maintainers/stakeholders are happy with this change?

Wim Leers’s picture

Status: Needs review » Needs work

Haha, I'm looking forward to that added verbose diff debug output! :)

I totally agree with the sentiment in the bottom of #164. I've confirmed RTBC before, in #129. I'd do it again (after doing manual testing), but unfortunately this patch needs another reroll because it no longer applies… :(

longwave’s picture

Status: Needs work » Needs review
FileSize
192.81 KB

Rerolled.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Manually tested again: create/edit node, in-place editing, toolbar, contextual links, Views. Did not find anything broken. Back to RTBC! :)

EDIT: Oh, this *should* have a change record now though, per the recent policy change of requiring a change record *before* commit.

nod_’s picture

Created a change notice, it needs work to be more wordy and have better examples but it's a start https://drupal.org/node/2201089

webchick’s picture

I guess I'll not mark this down from RTBC, but I still don't see any reviews from themers like I explicitly asked for in #146. I get that the JS people are all on board, because for JS people it makes sense to know what your dependencies are in advance. For CSS, though, this is a totally foreign concept; the entire point of CSS is you re-declare a rule with different styling and it magically works without you needing to understand the entire family tree of who declared that CSS and where. I'd also like to see validation that the SMACSS stuff makes sense to them, esp. the folks who worked on our CSS coding standards.

webchick’s picture

And it looks like sdboyer is still missing from this issue, too.

markhalliwell’s picture

Review from a themer:
Totally dig this. No PHP arrays!!!!

a totally foreign concept

This is not entirely true. Given more modern CSS workflows (using a preprocess like SASS/Compass) we're pretty used to declaring dependencies in both our config.rb and Gemfile. It would be very nice to do a simple search against "*.libraries.yml" which is an easier to read format than PHP arrays.

Regardless, this really isn't a "CSS" issues. This is about libraries as a whole (both JS, CSS and whatever else). This makes a lot of sense.

PS: I could totally see a sass/compass module/extension being created to easily parse these files and allow them to be included as an actual "library".

nod_’s picture

Something webchick wanted a validation on is the fact that we're adding the base/layout/component/state/theme "groups" when declaring CSS files.

drupal.dropbutton:
  …
  css:
    component:
      misc/dropbutton/dropbutton.css: {}
    theme:
      misc/dropbutton/dropbutton.theme.css: {}

Is that ok for everyone?

markhalliwell’s picture

The groups are perfect. These are libraries. Libraries should add their CSS to an appropriate group. It helps keep the order of specificity in check. If the library provides foundational CSS, then use base/layout. If it's all colorful and alters the specific look and feel of a site, theme. I can already see that for the Icon API module, I'll be using:

icon.selector:
  ...
  css:
    component:
      css/icon.selector.css: {}

This is appropriate because this is a specific component of a site. Of course, if anyone really truly had an issue with a libraries CSS group/weight, it's probably a bug (that an issue should be created to fix) or site specific; in which case it should just be altered.

mortendk’s picture

yes it is - to me it makes it very clear what's what & should make it easier for everybody to understand why it's importent to group the CSS

joelpittet’s picture

drupal.dropbutton:
  …
  css:
    component:
      misc/dropbutton/dropbutton.css: {}
    theme:
      misc/dropbutton/dropbutton.theme.css: {}

The component and theme organization from SMACSS is great for core! Though I'd rather not force that organizational structure through to other developers if they don't want that. Not everybody follows or cares about SMACSS.

Is it possible to have this as well?

drupal.dropbutton:
  …
  css:
   - misc/dropbutton/dropbutton.css: {}
   - misc/dropbutton/dropbutton.theme.css: {}

Sorry for my yaml ignorance if I didn't do that right.

RainbowArray’s picture

As a themer, I'd much rather work with a .yml file than a PHP array.

I think one important note on the SMACSS notation to consider is that, based on the CSS architecture discussion at [https://drupal.org/node/1887922], state and theme styles can be intermingled with component (module) styles in the same file. This is particularly important when dealing with media queries, which are state declaration. It's better to declare variations on a component (or a layout) due to media queries right next to the thing that's being modified at various viewport sizes, rather than splitting those variations out into separate files. Separating the media query (state) variations from the component/layout declarations is very bad for maintenance. The same is true for other state variations such as pseudo-elements like :hover and such.

If somebody wants to separate out state and theme declarations into separate files, that's fine, but I wouldn't want that to be a requirement.

I am a little concerned about Webchick's comments in #146. She's right, if I find something messed up in a site's CSS that I want to fix, I typically find the rule I need to override it, create a reasonable override for that rule, and then I'm done.

That said, I'm not sure that's what's going on here. If I just want to make some changes to the site's appearance, in a subtheme of a base theme, for example, I can just do that in a stylesheet attached in info.yml.

My understanding is that libraries.yml is only for pulling in some sort of external library or framework. So as far as I understand it, the only dependencies that would need to be declared for something in core would be if a theme's libraries.yml was overriding a library that was already in core, right? That's going to be much more rare than the much more common task of overriding a particular CSS rule.

I can understand that it would be a challenge to find the core module necessary to declare jQuery as a dependency if aggregation was turned on, but if I'm working on a theme, I have the ability to turn aggregation on and off. I can't imagine why I couldn't do that in order to find what the dependency is.

So I'm mostly okay with this as long as we're not requiring state declarations, and possibly theme declarations, to be in separate files.

Wim Leers’s picture

Issue tags: -Needs change record

#176: theme CSS isn't declared as libraries. i.e. this remains unchanged before vs. after the patch, in bartik.info.yml:

stylesheets:
  all:
    - css/layout.css
    - css/style.css
    - css/colors.css
  print:
    - css/print.css

The changes here are solely for "libraries", which are logical groups of CSS + JS assets for (reusable) UI components.

LewisNyman’s picture

I've never really seen any theme declare their CSS/JS as a library outside of core and contrib, so the affect this will have on themers is minor? Declaring JS dependancies in a custom theme is going to become required in Drupal 8, because we are no longer loading jQuery on every page, right?

Either way, this issue is a big improvement from a front-end perspective. It's a lot closer to the structure of bower.json and package.json which are becoming more and more common in front-end development. Maybe we could just parse the bower.json files that are included with most of our third party assets in core but that's probably not realistic.

nod_’s picture

Declaring JS dependancies in a custom theme is going to become required in Drupal 8, because we are no longer loading jQuery on every page, right?

Correct. It's already required actually.

So I'm mostly okay with this as long as we're not requiring state declarations, and possibly theme declarations, to be in separate files.

To be clear, if you declare your css file as a library you'll have to use the "groups". So #175 is not possible. But I mean if you just want to add a CSS file you can still use [#attached][css] and add the file directly, bypassing the whole library thing.

Danny Englander’s picture

I just cloned the latest from git and applied the patch and that seemed fine. I then removed the old hook_library_info call in my contrib theme and added this to a new libraries.yml file in my theme:

gratis-corescripts:
  version: VERSION
  js:
    js/scripts.js: {}
  dependencies:
    - core/jquery
    - core/drupal.ajax
    - core/drupal
    - core/drupalSettings
    - core/jquery.once

... and then under a .theme file page_preprocess function:

  $libraries['#attached']['library'][] = array('gratis', 'gratis-corescripts');
  drupal_render($libraries);

I still cannot get jquery.once to show up for anonymous users so I suspect I am doing something wrong here but I just don't know what. Not sure if I should open a separate issue for this.

This is now working with the code above.

nod_’s picture

the "groups" things is only for CSS:

core_scripts:
  version: VERSION
  js:
    js/scripts.js: {}
  dependencies:
    - core/jquery
    - core/drupal.ajax
    - core/drupal
    - core/drupalSettings
    - core/jquery.once

That should work. And don't forget you have to #attach it somewhere.

Danny Englander’s picture

@nod_ - Do you mean I actually have to use #attach to add those scripts in my .theme file or is this more of naming a corresponding function in the .theme file? Are there any examples you can point too? Thanks.

Danny Englander’s picture

BTW, this should probably not be held up on account of my issue above, I think I'll get this sorted, help has been offered outside this issue, sorry for derailing this.

NaheemSays’s picture

@highrockmedia: The comment above yours suggested a change to your libraries.yml file to make it work:

Change

  js:
    theme:
      js/scripts.js: {}

to

 js:
    js/scripts.js: {}
sdboyer’s picture

Issue tags: +Needs change record

this is all fine and +1 from me. the only thing i really need to note is #1762204-134: Introduce Assetic compatibility layer for core's internal handling of assets, which demonstrates that we have not actually done the rigorous work of correctly declaring dependencies in the context of CSS.

it may be that things have improved since then (i haven't been keeping a close watch). my issue there also *may* be fixed by the introduction of these SMACSS layers, which i have yet to accommodate in that patch.

sdboyer’s picture

Issue tags: -Needs change record

sorry, x-post

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I broke this with #2177469: Move node base widgets to the top level of the form, which was a critical that blocks other criticals so it jumped the "avoid commit conflicts" line.

I'm planning to do a huge commit spree right now, so maybe hold off on re-rolls until ~2-3 hours from now so work isn't wasted.

joelpittet’s picture

@_nod so my stance is still the same, I'm not confident that forcing the SMACSS groupings is the best way to go. I still am fine with doing it that way in core but I wouldn't want to force that convention on contrib or anybody else defining libraries. Everything else in this patch is great btw, and it seems not too many people share this opinion about the groupings. If you and others are confident that's the way to go then I'll trust your good judgement.

markhalliwell’s picture

One could easily just use the group "theme" for everything. I think that is why I don't really share the opinion that it's being "forced". It simply allows a level of granularity that is desperately needed to separate for core/contrib that do need it.

That all being said, I will admit there is a little inconsistency (edit: and confusion) between how the JS is implemented than the CSS as evident by #180-#184. I haven't read this entire issue, so this may have already be proposed and may have a reason as to why we're not doing it this way, but an alternative would be:

drupal.dropbutton:
  version: VERSION
  js:
    misc/dropbutton/dropbutton.js: {}
  css:
    misc/dropbutton/dropbutton.css:
      group: component
    misc/dropbutton/dropbutton.theme.css:
      group: theme
  dependencies:
    - core/jquery
    - core/drupal
    - core/drupalSettings
    - core/jquery.once
Jeff Burnz’s picture

The SMACSS stuff seems to have been of most interest for themers to review and this seems reasonably strait forward - however I have one question.

Why the "theme" group (not a group really but a weight)?

For me this caused confusion because the constant is CSS_SKIN which we use in #attached etc, so while this says it is a 1 to 1 mapping to the SMACSS layers (which really it is) but why use theme here at all, I'm not sure I follow the reason why to do this:

// Apply the corresponding weight defined by CSS_* constants.
$options['weight'] += constant('CSS_' . strtoupper($category == 'theme' ? 'skin' : $category));

Instead of just $options['weight'] += constant('CSS_' . strtoupper($category));

Am I wrong here, I have hardly read this issue and only spent a few minutes on it when porting some stuff I am working on and thought I better see how this works.

EDIT: just wanted to add that this is an awesome patch and I found it very easy to port my stuff, and I really like the "remote" key. The actual syntax is a little odd at first compared to other yml files but is just different and nothing major IMO.

benjy’s picture

What @MarkCarver said in #189 about the inconsistency for developers which was proven by #180 was the first thing that came to mind for me too.

Otherwise, I'm really liking the direction of this, great work!

Jeff Burnz’s picture

#191, yes perhaps, for me I work with these yml files a lot so having a different array/data structure for different things within a yml file is totally normal for me, however I can see how someone might be confused by this - core.libraries.yml is a good place to start and after I read that the differing structures were self evident.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
192.94 KB
944.24 KB

Thanks everyone for all the feedback, insights, and reviews! :-)

Most questions have been answered very accurately by @nod_ and @Wim Leers already, so I will only address the last remaining questions of:

A. (In)Consistency between JS and CSS declarations

  1. All JavaScript assets in D8 are driven by libraries. Any manual/direct calls to _drupal_add_js() do not participate in the dependency resolution/management for all other JavaScript assets in D8; i.e., a correct output/loading order cannot be guaranteed. That is why that function has been as private. All code uses #attached instead.
  2. Unlike JavaScript assets, most CSS asset files do not participate in a single dependency chain — i.e., the addition of foo.css is completely unrelated to the addition of bar.css. As a consequence, the "group" of bar.css cannot be derived from foo.css (or any other CSS file/library, for that matter).
  3. But yet, all CSS files still have to be placed into the correct groups. At the same time, there is no way for the library system to automatically figure out that foo.css is a CSS component, while bar.css is a pure skin/theme CSS.
  4. The "group" is strictly required for every CSS file, so that a skin/theme CSS file is not overridden by a component CSS file, because the latter is and will be mistakenly loaded after the former, if there is no "group" information. — Current HEAD is a pure testament to this, because almost all CSS files are loaded in absolutely no particular order. Just because of sheer luck and CSS selector specificity, you happen to see what you're seeing, but that is absolutely not how things are supposed to work. ;)
  5. Because the SMACSS category/group plays a significant role in loading all library CSS asset files (at least) in their appropriate group, it only makes sense to make the SMACSS categories an inherent part of the library definitions themselves.
  6. #1762204: Introduce Assetic compatibility layer for core's internal handling of assets will certainly be able to leverage the additional group information — BUT only to the extent in which there is actually any kind of connection between foo.css and bar.css. If there is no dependency chain between two (or more) CSS files, then even the best dependency resolution (directed acyclic graph) implementation is not able to help you further, because there is simply no relation between A and B. — But yet, we still have to load CSS asset files in their correct groups.

→ Because there is an expected difference with regard to whether any kind of dependency chains exist, the CSS asset file declaration is different from the JS asset file declaration.

→ A JS file without ancestors/sisters/children can be unconditionally loaded first. A CSS file cannot and must be loaded in a specific group.

This elaborate explanation hopefully addresses all concerns with a reasonable argumentation.

B. The "theme" group diverges from the actual SMACSS category "skin"

This patch is really not guilty for that. The group name "theme" is literally derived from the Official Drupal 8 CSS architecture documentation.

AFAIK, the name "theme" was chosen instead of "skin", because Drupal already has a concept of "themes", and themes want to be able to override any kind of .theme.css. In addition, we introduced a formal split into foo.base.css and foo.theme.css for D7 already. That naming decision is also the reason for why the corresponding CSS files are named foo.theme.css instead of foo.skin.css in D8.

I'd be more than happy to change the group name from "theme" to "skin", but given that "theme" is a formal part and collectively agreed-on naming of our CSS architecture standards policy for D8, that appears to be a much larger aspect and discussion of its own.

→ Renaming the group "theme" to "skin" is the only follow-up issue to the new library declaration format being introduced here that I can think of.

→ See you in #2202671: Rename CSS_SKIN constant to CSS_THEME :-)

I hope we can keep that topic in that issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 193: library.info.190.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Ugh, sorryyy — @Mark Carver just pinged me on the inderdiff...

The last interdiff was stupid/nonsensical to attach; it contains the complete differences between 8.x from ~1-2 weeks ago and today. :P

I only attached it out of habit. There were plenty of conflicts when merging latest 8.x, but I was able to resolve them in the merge commit already, so there is no interdiff, and thus, please ignore that interdiff. ;-)


That said, testbot claims a test failure just in time. But that looks like a random testbot fluke to me, so re-testing in a moment.

Jeff Burnz’s picture

@193 - top work sun, thank-you for the detailed explanations and your heroic efforts with this patch.

sdboyer’s picture

i didn't realize there was this much discord over SMACSS.

it needs to be understood that we require *clear* rules on which to do the sorting for the whole new graph-based system to be able to work. it's SMACSS, or not SMACSS, period.

fwiw, "not SMACSS" will produce fewer aggregates.

sun’s picture

Yeah, to clarify further:

This patch just (1) enforces a SMACSS group for every CSS file and (2) processes that group into the existing 'weight' option that is passed to _drupal_add_css() later on:

+        // Prepare (flatten) the SMACSS-categorized definitions.
...
+        if ($type == 'css' && !empty($library[$type])) {
+          foreach ($library[$type] as $category => $files) {
+            foreach ($files as $source => $options) {
...
+              // Apply the corresponding weight defined by CSS_* constants.
+              $options['weight'] += constant('CSS_' . strtoupper($category == 'theme' ? 'skin' : $category));
+              $library[$type][$source] = $options;

Whether we want to keep it that way (just stick with weights), or whether we want to pass on the group information separately to the graph implementation, is a question that we do not need to answer here.

What matters is that every CSS file (1) is guaranteed to have a group and (2) will be loaded [at least] with the correct weight of the corresponding group. Given a group, pretty much all CSS files should not need any further or more granular adjustments to their weight within the group; i.e., hacks like weight: -1 should be obsolete.

In other words:

→ All we're doing here is to ensure that we have the most minimal group/weight information for each CSS file.

→ How exactly that information is used later on when resolving/loading dependencies and producing aggregates is a different question, which doesn't really matter for the library declarations. — The code for processing the library declarations can be happily changed to match and produce whichever information we want to have for resolving/grouping/aggregating assets.

RainbowArray’s picture

I hadn't read through the discussions on CSS architecture changes and the use of SMACSS. Now that I have, it makes a ton of sense.

So +1 to finishing up any necessary rerolls and getting this in.

There's no need to bikeshed SMACSS vs non-SMACSS here. There's already been a ton of thought and work put into heading in the direction of SMACSS organization. This patch just implements those already-made decisions.

Getting this into YML is a huge win. Thanks for all the work @sun.

sdboyer’s picture

@mdrummond - this may be a discussion that needs to go elsewhere, but it is not a bikeshed.

@sun re: 198 - there *is* no weight information. weight is gone. as are the aggregate groups that we used to use.

do what y'all are gonna do. if i can work around it, i will. if i can't, we'll cross that bridge.

sun’s picture

FileSize
174.05 KB

Just to clarify on #200, because it sounds a bit negative: ;)

I just had a longer discussion with @sdboyer in IRC in order to get on the same page and double-check whether this patch introduces a conflict for the upcoming Assetic changes. We both concluded that it does not.

We even questioned whether the SMACSS categorization/groupings on their own aren't (theoretically) sufficient for sorting CSS asset files (sans any further dependency resolution), because the order of individual CSS files within a SMACSS group should (theoretically) not matter... Anyway, in the end, we concluded that the big challenge remains to be and is going to be aggregation, or more specifically, to ensure that multiple aggregate files on the same page will not override more-specific CSS of a former aggregate with less-specific CSS in a later aggregate. But that's rather off-topic here :)

Now, just for extra kicks, I'm attaching a graph of all libraries (in core) that are declaring CSS asset files (excluding JS dependencies).

(Green libraries do not have own dependencies, so if those are loaded on their own, then only the SMACSS category kicks in. Red libraries are flagged as "every_page".)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, awesome. Thanks very much for seeking out themer opinions on this issue. Sounds like everyone's basically on board with this being a good direction, though there are some follow-up issues to discuss further refinement. Also sounds like this has a neutral effect on the work Sam's doing, so +1 there, too.

Committed and pushed to 8.x. YEAH!

webchick’s picture

Btw, it'd be great for one of the folks who tried out porting their libraries to this new syntax to check over the change record at https://drupal.org/node/2201089 and review it prior to publishing to make sure it has everything you would've needed to do so. :)

markhalliwell’s picture

Title: Replace hook_library_info() by *.libraries.yml file » [Change Record] Replace hook_library_info() by *.libraries.yml file
Status: Fixed » Active
Issue tags: +Approved API change, +Needs change record

Adding "Approved API change" for prosperity/record keeping and since it was committed.
Changing title/status and adding "Needs change record" since it's in draft and not published.

sun’s picture

Title: [Change Record] Replace hook_library_info() by *.libraries.yml file » Replace hook_library_info() by *.libraries.yml file
Status: Active » Fixed
Issue tags: -Needs change record

Fixed some faulty HTML markup and published the change notice: https://drupal.org/node/2201089

Created the follow-up issues as stated in the issue summary:

#2203407: Replace #attached library array values with provider-namespaced strings
#2203411: Convert drupal_get_library() into a service

sun’s picture

Also created #2203415: Verify/fix missing dependencies in libraries to evaluate and investigate the currently declared dependencies.

sun’s picture

Wim Leers’s picture

Awesome work, sun!

And thanks to those who provided "themer feedback" for an excellent, mature, non-bikeshedding discussion! :)

Hurray for another DX improvement!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

This broke the Views UI, could use a review on #2217669: [REGRESSION] Views UI broken

webchick’s picture

Here is some developer feedback from a themer actively trying to use this to add in an external JS library to a theme: #2298551: Documentation updates adding/adjusting inline/external JS in themes Would love suggestions there on how we could've made this not an hours-long task, but still retain the benefits of what this patch is trying to do.

cilefen’s picture