Problem

  • Some external libraries are contained in specific core modules, which prevents contrib from using them without having to depend on the respective core module.

Details

  1. Previously, core.libraries.yml did not exist, so e.g., System module registering CKEditor module's library made little to no sense.

  2. 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.libraries.yml.
    3. Placing an external library into a core module is discouraged and requires a very sound, solid, and sophisticated reasoning.
  3. Here is why:

    1. 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").
    2. 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.

Task

  1. Move the picturefill.js library from Picture module into core.
  2. Move the jquery.joyride library from Tour module into core.
CommentFileSizeAuthor
#2 lib.move_.2.patch3.97 KBsun

Comments

sun’s picture

Picturefill is actually a bit weird — does anyone know why we diverged from upstream, instead of contributing upstream?

picturefill:
  remote: https://github.com/scottjehl/picturefill
  # @todo Contribute upstream and/or replace with upstream version.
  # @see https://drupal.org/node/1775530
  version: VERSION
sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
StatusFileSize
new3.97 KB

Let's ignore the topic of #1 here and discuss that in #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0 instead.

Attached patch moves both libraries into core.

wim leers’s picture

Title: Move all external libraries from core modules into core » Move all external libraries from core modules into core.libraries.yml
Issue tags: +JavaScript

Agreed.

Any reason why we don't move CKEditor and others here already? :)

sun’s picture

I added explicit @todos to the new .libraries.yml files in the original conversion. These two libraries are the only two that are not in /core/assets/vendor yet. All others are in there already.

The declaration of the ckeditor library was moved into core.libraries.yml in the parent/original issue already.

Thus, moving these two is all that is required to resolve this issue.

sun’s picture

sun’s picture

Any additional questions? :-)

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

lgtm

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I guess this works. It's a little weird for something in a super optional core module (Tour) to be in "core" core library definitions, though.

This solution obviously won't work for contrib, either, since contrib modules can't hack core.services.yml (beyond hook_library_info_alter() I suppose...), and we don't have a central "contrib.services.yml" to ensure one library registration per Drupal installation. So it's not clear to me how contrib is going to avoid all of the problems listed in the issue summary when for example both Views Slideshow and Media module want to use different versions of the same slider library. I guess we'll need thin module wrappers around every JS lib out there and these modules to declare dependencies on them?

At any rate, though, this hack will work for the libraries used by core, and it sounds like we've already started doing this anyway w/ CKEditor, which is also a dependency of an optional core module, so this is just making all the other libraries consistent with this approach.

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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