Provide a mechanism for dynamic library declarations

We already have two alter hooks. One to alter the static library definitions (https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p...) and one to alter a specific library at runtime, when it is used on a page: https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p....

So i think the only difference for a build hook/callback would be that it can define something completely new.

As mentioned in the other issue, in jw_player, I currently define a dummy entry in libraries.yml that I then alter, so that part would go away. http://cgit.drupalcode.org/jw_player/tree/?h=8.x-1.x and http://cgit.drupalcode.org/jw_player/tree/jw_player.module?h=8.x-1.x#n92 down.

Is #3 a complete workaround for the problem? If so I would demote this to major.

Just adding a couple of tags ... this implementation is pretty good.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -221,13 +219,23 @@ public function buildByExtension($extension) {
   +      catch (InvalidDataTypeException $e) {
   +        // Rethrow a more helpful exception to provide context.
   +        throw new InvalidLibraryFileException(sprintf('Invalid library definition in %s: %s', $library_file, $e->getMessage()), 0, $e);
   +      }
   

   <3

2. +++ b/core/modules/system/theme.api.php
   @@ -644,6 +644,70 @@ function hook_js_alter(&$javascript) {
   +  if (REQUEST_TIME < date_sunrise(REQUEST_TIME) || REQUEST_TIME > date_sunset(REQUEST_TIME)) {
   

   This is quite a day to day usecase ...

... beside the tags it seems to be done now.

Updated the IS, added a CS

One thing which is kinda confusing for me is that the alter hook is executed for every extension, but this is out of scope for this issue, for sure,
its just odd.

Code looks good, test coverage looks sane.

+++ b/core/modules/system/theme.api.php
@@ -644,6 +644,70 @@ function hook_js_alter(&$javascript) {
+  if (REQUEST_TIME < date_sunrise(REQUEST_TIME) || REQUEST_TIME > date_sunset(REQUEST_TIME)) {
+    $libraries['mymodule.vampire'] = [

ROFL :D

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -66,10 +66,8 @@ public function buildByExtension($extension) {
     $library_file = $path . '/' . $extension . '.libraries.yml';

Shouldn't this move into parseLibraryInfo() and remove $library_file as a parameter?

Additionally, looking through where the $library_file parameter is used later:

// If this is a 3rd party library, the license info is required.
if (isset($library['remote']) && !isset($library['license'])) {
throw new LibraryDefinitionMissingLicenseException(sprintf("Missing license information in library definition for '%s' in %s: it has a remote, but no license.", $id, $library_file));
}

This is not touched in the patch, but the incomplete license may not be in the file, it could be in the hook instead.

Once we would have figured out #2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all properly, this issue would probably be just easier, given that there is just once place where the alter is called.

Per #18.

Per #19 :)

So the other issue didn't went anymore, @tstoeckler do you want to (probably) reroll your patch and get it in?

Agreed that this is unblocked.

A reroll of #16.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -77,15 +77,11 @@ public function buildByExtension($extension) {
   -        throw new IncompleteLibraryDefinitionException(sprintf("Incomplete library definition for '%s' in %s", $id, $library_file));
   +        throw new IncompleteLibraryDefinitionException(sprintf("Incomplete library definition for '%s'", $id));
   
   @@ -102,7 +98,7 @@ public function buildByExtension($extension) {
   -        throw new LibraryDefinitionMissingLicenseException(sprintf("Missing license information in library definition for '%s' in %s: it has a remote, but no license.", $id, $library_file));
   +        throw new LibraryDefinitionMissingLicenseException(sprintf("Missing license information in library definition for '%s': it has a remote, but no license.", $id));
   

   Instead of logging the file name, which is indeed not possible for dynamically discovered libraries, we should log the extension. Because there may be multiple libraries with the same name, but in different extensions.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -218,14 +214,26 @@ public function buildByExtension($extension) {
   +    // Allow modules to add dynamic library definitions.
   +    $hook = 'library_info_build';
   +    if ($this->moduleHandler->implementsHook($extension, $hook)) {
   +      $libraries = array_merge($libraries, $this->moduleHandler->invoke($extension, $hook));
        }
   

   Why add a new hook, why not just explicitly make it possible for hook_library_info_alter() to *add* libraries?

   If some other extension wants to alter it, it can still use hook_module_implements_alter() to make its alter hook run later?

3. +++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
   @@ -375,6 +375,28 @@ function testLibraryAlter() {
   +   * Dynamically defines a JavaScript library and alters it.
   

   s/a JavaScript library/an asset library/

Why add a new hook, why not just explicitly make it possible for hook_library_info_alter() to *add* libraries?

If some other extension wants to alter it, it can still use hook_module_implements_alter() to make its alter hook run later?

This strategy prooved to be not successfull in Drupal 7. We do have the same strategy in many other places (route rebuilding, entity type definitions etc.).

Calm down guys, I was merely playing the devil's advocate, to make sure we have a documented (in this issue), solid reason to do an API addition. Well answered :)

Core committers discussed today and agreed we need this.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -218,14 +214,26 @@ public function buildByExtension($extension) {
+      $libraries = array_merge($libraries, $this->moduleHandler->invoke($extension, $hook));

Should we maybe use NestedArray::merge() here instead?

Also we need to address #26 1,3 on top of it.

#26:1,3; #31 addressed.

Fixes phpunit fails

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -77,15 +78,11 @@ public function buildByExtension($extension) {
-        throw new IncompleteLibraryDefinitionException(sprintf("Incomplete library definition for '%s' in %s", $id, $library_file));
+        throw new IncompleteLibraryDefinitionException(sprintf("Incomplete library definition for extension '%s'", $extension));
       }

@@ -102,7 +99,7 @@ public function buildByExtension($extension) {
-        throw new LibraryDefinitionMissingLicenseException(sprintf("Missing license information in library definition for '%s' in %s: it has a remote, but no license.", $id, $library_file));
+        throw new LibraryDefinitionMissingLicenseException(sprintf("Missing license information in library definition for extension '%s': it has a remote, but no license.", $extension));

Let's give as much context as possible and add both the extension and the library ID

Addresses #37

agree

cross-post

Can we please write the beta evaluation and add the api changes to the issue summary?

That itself is a novice task

.

.

Added beta evaluation and draft change records here https://www.drupal.org/node/2402315 .

Thank you a lot!

+++ b/core/modules/system/theme.api.php
@@ -722,6 +722,70 @@ function hook_js_alter(&$javascript) {
+  if (REQUEST_TIME < date_sunrise(REQUEST_TIME) || REQUEST_TIME > date_sunset(REQUEST_TIME)) {

I know it's just a docs example, but since the definitions are cached, this wouldn't work without clearing the cache on a schedule or altering the cache key of the library discovery to take into account sunset/sunrise. So could we do a less dynamic example like module_exists() where we know the definition will get re-discovered at the appropriate time? Could also use a note in the hook documentation that the results are cached.

The config()->get() is similar as well, nothing rebuilds library definitions on config save.

Should it be something like this?

Trying to pass..

+++ b/core/modules/system/theme.api.php
@@ -743,7 +743,7 @@
-  if (module_exists('somebodyelsemodule')) {
+  if (Drupal::moduleHandler()->moduleExists('minizombies')) {
     $libraries['mymodule.zombie'] += [

@@ -769,7 +769,7 @@
   // the library (of course) not be loaded but no notices or errors will be
   // triggered.
-  if (module_exists('mymodule')) {
+  if (module_exists('vampireize')) {
     $libraries['mymodule.vampire'] = [
       'js' => [
         'js/vampire.js' => [],

Let's use the module handler in both places

Thank you, this looks fine for me.

We do have a change record, proper issue summary as well as a beta evaluation.

Thanks for changing the examples (and sorry for spoiling the nice joke :().

Committed/pushed to 8.0.x, thanks!

Great work! :)

The only problem: we have TWO change record drafts! :D This must be an unprecedented problem we've had in the D8 cycle :D

There's both https://www.drupal.org/node/2402315 and https://www.drupal.org/node/2374649. Which to publish? I think we should pick the best parts of both.

vampires need module now :))

@WimLeers
Wow, sorry I didn't notice one was created prior to mine..

Don't worry, this is a *great* problem to have :)

I've updated this change record https://www.drupal.org/node/2374649 anything to revise?

CR need to say that this hook is invoked before library information is cached and not dynamically invoked on every page request. See #48.

@nod_
Right, this is the idea. I agree the word dynamic can be confusing, but its dynamic relative to use a static yml file.

#62/#63: added that, and published the CR: https://www.drupal.org/node/2374649

It was surfaced in #2418907: _drupal_add_js()/css() removed from core that this implementation doesn't allow for libraries to reside outside of the module implementing hook_library_info_build(), which is the main point of the Libraries API module (as it manages libraries in sites/all/libraries or PROFILE/libraries).

I know this issue is over, but still wondering: why changing the pattern every when and since ? Sometimes it's an event, sometimes it's a callback method (dynamic permissions for instance), sometimes it's a hook (like here). Wasn't it a point of D8 to be OOP, thus avoid hooks ?
Why not agree on a unique strategy and stick to it ? That way, when a dev wants to do something, he knows what to search for. Let say it's a callback like in permission, he knows he simply have to find the callback name for dynamic libraries.

Also, more OOP, isn't it possible to define abstract class for things, let says AbstractLibraryDefinition and instead of invoking a hook, invoke all modules class that would implements AbstractLibraryHandler and implements it's virtual method "buildDynamicPermission()" for instance, then its method "alterPermissions($permissions)" and so on ?

Something like (naively):

foreach (get_declared_classes() as $className) {
    if (in_array('AbstractLibraryDefinition', class_implements($className))) {
        // Get dynamic libraries. 
        $libraries = NestedArray::mergeDeep($className->buildDynamicPermission());
    }
}

This is highly likely to be a stupid question, but if I don't ask, I will never learn !
Thanks all !