Add support for third party libraries in site specific and install profile specific libraries folders

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -189,7 +190,12 @@ public function buildByExtension($extension) {
                  if ($source[1] !== '/') {
   -                $options['data'] = substr($source, 1);
   +                if (strpos($source, '/libraries/') === 0) {
   +                  $options['data'] = $this->findLibrary($source);
   

   That's a neat idea.

   It means sites can just stop installing libraries.module, and all those existing contrib modules will continue to work with a non-standard library location. And once they require an appropriate core version, they can just drop the libraries.module integration.

   Existing replacements should continue to work although I'll want to explicitly test that.

   Also, the additional file exists checks should be OK because that only happens on a cache miss.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -235,6 +241,37 @@ public function buildByExtension($extension) {
   +    // If don't find anything return the original path without the leading
   +    // slash.
   +    return substr($path, 1);
   

   should we repeat the reason for why we do this from buildByExtension()?

What about the use case of checking for the existence of the library in hook_requirements()? I suppose to we could get the library definition and check the path in there? Or should we make this API public to allow more direct access?

Hey there ;-). Some thoughts from a Libraries API (non-)maintainer:

• The Problem/Motivation is spot on in regards to Libraries API. @sun and I schemed some very ambitious plans at DrupalCon Munich (!) but I never managed to materialize those into anything remotely functional, so it doesn't make sense to hold out any hope at this point.
• I am fine with granting maintainership to someone here to pursue a less ambitious version of Libraries API. Absolutely not opposed to solving this issue in core, but I thought I'd mention it.
• libraries_get_path() was originally intended as an "extension" of drupal_get_path(), i.e. it was a shim for drupal_get_path('library', ...) which did (and does) not exist. Since the conceptual descendant of drupal_get_path() is ExtensionList it may (or may not!) make sense to have a real extension.list.library service, etc. Again, not at all opposed to the current patch in particular, just thought I'd bring that up as general context.

1. +++ b/core/lib/Drupal/Core/Asset/LibrariesDirectoryFinder.php
   @@ -0,0 +1,107 @@
   +/**
   + * @todo
   + */
   +class LibrariesDirectoryFinder {
   +
   

   :)

2. +++ b/core/lib/Drupal/Core/Asset/LibrariesDirectoryFinder.php
   @@ -0,0 +1,107 @@
   +
   +    // Remove the directories/ from the path so it can be replaced.
   +    $updated_path = substr($path, 10);
   +    foreach ($directories as $dir) {
   +      if (file_exists($this->root . '/' . $dir . $updated_path)) {
   +        return $dir . $updated_path;
   +      }
   +    }
   

   I'm not sure i understand why we require libraries/ when we then cut it off. Ah, I guess we have to cut it of somewhere?

   What if we leave out "libraries" from the $directories list, then we don't have to cut off?

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -67,6 +76,11 @@ public function __construct($root, ModuleHandlerInterface $module_handler, Theme
        }
        $this->streamWrapperManager = $stream_wrapper_manager;
   +    if (!$libraries_directory_finder) {
   +      @trigger_error('Calling LibraryDiscoveryParser::__construct() without the $libraries_directory_finder argument is deprecated in drupal:8.9.0. The $libraries_directory_finder argument will be required in drupal:9.0.0. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   +      $libraries_directory_finder = \Drupal::service('library.libraries_directory_finder');
   +    }
   +    $this->librariesDirectoryFinder = $libraries_directory_finder;
      }
   

   Sure about 9.0, wouldn't this have to be 10 now?

This looks pretty good to me, I've tested it with dropzonejs and it works nicely and the simplification there is great: #3099836: Use new LibrariesDirectoryFinder

1. +++ b/core/lib/Drupal/Core/Asset/LibrariesDirectoryFinder.php
   @@ -0,0 +1,100 @@
   +
   +/**
   + * Finds libraries that are required by contributed and custom code.
   + */
   +class LibrariesDirectoryFinder {
   

   Not sure if it should be LibrariesDirectoryFinder or just LibraryFinder? It actually "finds" specific files of a library, not a directory (although that would work too I suppose), so just LibraryFinder might work too?

   Also not quite sure about the contributed and custom code part.. is that even relevant? Core doesn't need it because it's allowed to include all JS code that it needs, but is that really relevant?

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -189,7 +203,15 @@ public function buildByExtension($extension) {
                  if ($source[1] !== '/') {
   -                $options['data'] = substr($source, 1);
   +                $source = substr($source, 1);
   +                // Non core provided libraries can be in multiple locations.
   +                if (strpos($source, 'libraries/') === 0) {
   +                  $path_to_source = $this->librariesDirectoryFinder->find(substr($source, 10));
   +                  if ($path_to_source) {
   +                    $source = $path_to_source;
   +                  }
   +                }
   +                $options['data'] = $source;
                  }
   

   One use case in dropzonejs.module is an optional library. Not sure how common that is and whether or not we want to support that. We could have a flag like optional: true and if set, we'd unset the library definition. But the new code is also fairly simple although it means the module still needs to implement a hook, but it's pretty simple.

   So probably this is good enough for now?

   Another thing is documentation. A change record is one thing, but I'm wondering where/how we'd exactly document this new feature? We don't really have a standard for this, hook_library_info_build() mentions that you can also define it in $module.libraries.yml, \Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo() mentions editor.libraries.yml by example.

I think this looks good.

This does add a D10 deprecation, but it's only a constructor thing and I only found a single subclass that doesn't override the constructor: http://grep.xnddx.ru/search?text=extends%20LibraryDiscoveryParser. So the chances that anyone will see this are slim and definitely not with drupal-check or similar static analysis tools.

The issue summary and CR already explain quite well that this would be very useful to have already in 8.9. Many common modules optionally integrate with the libraries module only to support these dynamic paths. There are two parts to this, the main functionality here just works, so basically a site can just stop installing libraries.module and it will still work. As the patch in #3099836: Use new LibrariesDirectoryFinder shows, modules that have e.g. hook_requirements() checks need to be adjusted to use the new API and would therefore depend on 8.9, but it's not an API break.

We also have the +1 from the current libraries.module maintainer in #8. As @tstoeckler said himself, libraries.module is currently not really maintained and has a very unclear future.

Some of the relatively common D8 modules that would benefit from this:
* Slick/Blazy
* Dropzonejs
* Select2
* Shariff
* (Before 8.8 also paragraphs, but that library is now in core, so we can remove that once we require 8.8)
* There are 9 pages of results on http://grep.xnddx.ru/search?text=libraries_get_path&filename= (of course also finds the module itself, some are for php libraries and other stuff, but still, it's quite a lot)

As a small API/site-builder facing addition I think this is probably still viable to commit to 8.9.x. The constructor deprecation is fine too.

I haven't reviewed the patch properly yet though.

I considered LibrariesFinder and LibraryFinder when I wrote the patch. The reasons I went for LibrariesDirectoryFinder are:
that this is all about whether a file or folder is in specific libraries directory

One issue with this is that we support placing different libraries in different directories on the same install - i.e. Library A in sites/SITE/libraries, library B in /libraries, and library C in profiles/PROFILE/libraries. So it is finding library directories (i.e. for one library), not the libraries directories themselves. Would LibraryDirectoryFinder be more self-documenting maybe?

To me this is a new (site-admin-facing) feature and should just go into 8.9.x

Exactly, my plan is to basically do this in contrib modules:

if (new service exists) {
use that API
}
elseif (library module exists) {
use library module API
}
else {
use default location
}

and eventually once a module then requires 8.9, we can clean it up, and require the new service and stop integrating with library module at all. For users, the switch should be transparent, in 8.8, they need the library module if they have it in a non-default location, once they update to 8.9 they can uninstall that and then update to 9.0 without it.

I updated my patch for dropzonejs with this approach: #3099836: Use new LibrariesDirectoryFinder. Seems to be working nicely and I think this is ready, @catch agreed on slack with the name. I also updated the change record with the new name and this pattern.

For users, the switch should be transparent, in 8.8, they need the library module if they have it in a non-default location, once they update to 8.9 they can uninstall that and then update to 9.0 without it.

So we want users to be able to do a direct update from 8.8 to 9.x too, I can see two ways to do this:

1. Uninstall libraries module immediately prior to doing the 9.0.x update
2. Libraries could potentially do a 9.x 'port' which is just an .info.yml so that sites can do a clean uninstall after upgrading.

Opened #3110305: Drupal 8.9.x deprecation signposting against libraries module for potentially doing that stub release, and documenting options on the project page.

Fixed this on commit:

FILE: ...upal/core/lib/Drupal/Core/Asset/LibrariesDirectoryFileFinder.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 89 | WARNING | [x] Empty PHP statement detected: superfluous
    |         |     semi-colon.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed 8dad334 and pushed to 9.0.x and 8.9.x. Thanks!