This is a follow-up to #2090623: Turn hardcoded library_load_files() into a more modern, flexible system.

Library locators now use stream wrappers when scanning for the library sources, which is great. However, as it stands now there is a one-to-one relationship between each lib type and the stream wrapper used to locate the lib sources. Moving forward I think that we need to be able to:

  1. Support a one-to-many relationship between lib types and stream wrappers (at an architectural level).
  2. Allow some configuration around which stream wrappers can be used in the location process.
  3. Allow other extensions (specifically profiles) to programmatically add stream wrappers that can be used in the location process.

For #1 I think we could benefit from a new locator plugin that can be passed multiple stream wrapper schemes, which are then tested in order, before returning a found/not-found results.

#2 is a little tricky. On one hand the current design seems to lean more towards lib-type-specific logic, so we could have separate configuration about which wrappers are checked per lib type. On the other hand I'm thinking that approach could be tricky to scale, given that lib types are extendable. A simpler way might be to offer some configurable "global" lib source wrappers. These would be configurable stream wrappers (part of libraries.settings that could even eventually have GUI options around it) that would always be checked as fallbacks for all lib types (asset, php-file, etc...).

For #3, I'm personally not sure what would be best. I don't think we want to force extensions that bundle lib sources (like profiles) to alter/override libraries.settings config to get their custom lib locations recognized, so I'm guessing a separate option is needed here. My instinct in D7 would be an alter hook, but we probably have better options now.

In all of this I am assuming that any custom lib source locations (set via config or via programmatic alterations) must be defined as stream wrappers. So for a profile (for example) to bundle its own lib sources it would also have to define its own stream wrapper. This is of course a notable change from D7 where we automatically scan a bunch of common locations dynamically and automatically.

CommentFileSizeAuthor
#10 interdiff-2818689-6-10.txt14.84 KBrjacobs
#10 2818689-10.patch13 KBrjacobs
#6 2818689-6.patch6.34 KBrjacobs
#4 2818689-4.patch6.33 KBrjacobs
#3 2818689-3.patch6.12 KBrjacobs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rjacobs created an issue. See original summary.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Related issues: +#2090623: Turn hardcoded library_load_files() into a more modern, flexible system

Adding relation to #2090623: Turn hardcoded library_load_files() into a more modern, flexible system.

I'm working on a preliminary patch to get this idea rolling.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Active » Needs review
FileSize
2818689-3.patch6.12 KB

Here's a patch that takes a stab at the idea. It:

  1. Adds a new locator plugin called StreamMultipleLocator.
  2. Sets lib types AssetLibrary, MultipleAssetLibrary and PhpFileLibrary to use it. For the asset cases I also passed-in both the asset and php-file schemas such that asset libs could still be found in sites/all/libraries (for legacy support). We would probably want to encourage the new sites/all/asset/vendor location (asset scheme) but I figure it might be easiest to initially support both.
  3. Adds a new global_locator_streams option to libraries.settings config. This is a sequence value that can be used to pass in 0-n stream wrapper schemes that can be used as "global" lib source locations (fallback locations for all library types).

Some solution for programmatically adding lib source locations needs to be discussed, and a test for the new config option is probably needed too... assuming of course that this is even the right direction.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
FileSize
2818689-4.patch6.33 KB

Here's a further iteration of the idea. The only change from #3 is that I'm basing the list of locations to check on URIs as opposed to stream wrapper schemes. This also seems a bit more consistent with what we would do with local definition location conf. This would allow the config sources (to check) that are passed to the locator, or defined in the new libraries.settings option, to be full URIs. These would be based on shipped stream wrappers by default (asset:// and php-file//) but could also allow options like public://some/sub/path, which could be useful to end users who want custom source locations but don't want to implement new stream wrappers.

I think this all revolves around what strategy we want for exposing baseline config options for the module.

Status: Needs review » Needs work

The last submitted patch, 4: 2818689-4.patch, failed testing.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Needs work » Needs review
FileSize
2818689-6.patch6.34 KB

Whoops, had a small config schema error in that last one.

tstoeckler’s picture

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

I think this is great. I originally used the asset directory for consistency with core, but already a bunch of modules are hardcoding external libraries in the/libraries directory, so maybe we are better off using that as default. In any case we should be as flexible as possible in that regard, which is why I like this issue a lot.

I had originally hoped for #1308152: Add stream wrappers to access extension files to get in, so that we could use a single stream wrapper, to implement the /libraries / sites/all/libraries / sites/example.com/libraries fallback logic that we will need at some point, but that never happened :-/

So just like we moved away from hardcoding the implementation to stream wrappers for the definition discovery I think this similarly makes a lot of sense here.

Some concrete notes on the patch:
I like the URI-based locator a lot better than the current one that just uses the actual name of the stream wrapper. However, I don't think we necessarily need to have multiple URIs set up in a single locator. Instead, I think it should be possible to create a ChainLocator, that has a addLocator($locator) method and then just iterates over the given locators that were passed in its own locate() method. That would allow to transform the current StreamLocator into a UriLocator by simply adding the $target handling (and renaming) it. Also that would make it so that the setup is very similar to that of the definition discoveries.

(Thinking about that and looking at the various classes I actually think that we should rename FileDefinitionDiscoveryBase to UriDefinitionDiscoveryBase, because it actually primarily deals with URIs. That would make the two subsystems even more similar.)

What do you think?

tstoeckler’s picture

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

Thinking about this more, if we go with URIs and not stream wrapper names everywhere, I guess we can actually kill off all those stream wrappers, which would be nice. I originally thought that would be a nice layer of abstraction, but I guess it's more confusing than anything else at this point.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Needs review » Needs work

Your comments make good sense. I think we can certainly iterate a bit more here.

don't think we necessarily need to have multiple URIs set up in a single locator. Instead, I think it should be possible to create a ChainLocator, that has a addLocator($locator) method and then just iterates over the given locators that were passed in its own locate() method.

I think I see what you are proposing with the ChainLocator idea, but I need to revisit the details on that. I guess this would mean that a Library's getLocator() could contain something like:

return $locator_factory
  ->createInstance('chain')
  ->addLocator('uri', ['uri' => 'asset://'])
  ->addLocator('uri', ['uri' => 'php-file://']);

If so, yes, that's likely a more flexible way to accomplish the same thing, and could allow mixing-and matching other locator types (stream-based, uri-based, remote-url-based). I still assume we would need some form of "global locators" option that could be tied into the config and that could still build off of this?

if we go with URIs and not stream wrapper names everywhere, I guess we can actually kill off all those stream wrappers, which would be nice

Do we have sufficient core stream wrappers to leverage (via the right paths) to capture our standard locations? I think I jumped straight to the assumption that we did not given that you had added asset and php-file already, but admittedly I didn't look into what core was offering there. #1308152: Add stream wrappers to access extension files is a pretty interesting reference.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Needs work » Needs review
FileSize
2818689-10.patch13 KB
interdiff-2818689-6-10.txt14.84 KB

Here's a new patch that leverages the chain locator idea. An interdiff is included as well but it's sufficiently different that a fresh review of the new patch is probably easiest.

The chain logic is pretty straightforward, but what I am still not sure about is how to incorporate the idea of "global" locators that can be set in the config. I personally feel like this will be an important feature, but there are so many different ways it could be done. What I've done in this iteration is to make the idea intrinsic to the chain locator itself (e.g. it's the job of the chain locator plugin to check the libraries.settings conf and incorporate any global locator settings it finds there). This of course means that any library types that don't use the chain locator would not take advantage of this concept. That's probably not a problem, especially as this patch sets all existing library types to use the chain concept, but it's still worth noting. I've also tried to generalize the way that a global locator can be defined in conf as much as possible. It's setup so that any locator, of any type (not just URI), could be set as global. I'm just not sure if this convolutes the conf schema too much.

It might also still be worth adding a way to programmatically add a global locator (alter hook or the like) so that profiles could get in on all this goodness without the need to alter any conf or override existing locator logic.

This patch does not make any changes to the stream wrappers, maybe that should be a separate issue? I wonder if it might be worth creating some kind of "libraries-sites" stream wrapper that's tied to the /sites/all path. Then we could pass URIs like "libraries-sites://assets/vendor" and "libraries-sites://libraries". I dunno, something about that idea just doesn't feel quite right though.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Note to self: the "global locators" idea probably needs a test if the idea gets traction.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Needs work

Yay, thanks. The chain locator is exactly what I was thinking.
Providing some global configuration is an interesting idea. Not sure what to if different kinds of libraries should use different locators, but, yeah, making it configurable in some way is good I think.

  1. +++ b/config/schema/libraries.schema.yml
@@ -25,3 +25,16 @@ libraries.settings:
               title: 'The URL of the canonical library registry.'
...
+      title: 'Global library locators'
...
+        title: 'Global locator plugin'
...
+            title: 'The locator plugin id'
...
+            title: 'The plugin configuration'

    I see this is a pre-existing issue, but the key should actually be "label" not "title". 

  2. +++ b/config/schema/libraries.schema.yml
@@ -25,3 +25,16 @@ libraries.settings:
+            type: sequence

    We can actually provide a proper schema for the plugin configuration by specifying the types as e.g. "libraries.locator.[%parent.id]" and then provide schema for e.g. "libraries.locator.uri". We can also move this to a follow-up, though, if this doesn't work for you. It's not super critical. 

  3. +++ b/src/ExternalLibrary/Asset/AssetLibrary.php
@@ -118,7 +118,9 @@ class AssetLibrary extends LibraryBase implements
+      ->addLocator($locator_factory->createInstance('uri', ['uri' => 'asset://']))
+      ->addLocator($locator_factory->createInstance('uri', ['uri' => 'php-file://']));

+++ b/src/ExternalLibrary/Asset/MultipleAssetLibrary.php
@@ -113,7 +113,9 @@ class MultipleAssetLibrary extends LibraryBase implements
+      ->addLocator($locator_factory->createInstance('uri', ['uri' => 'asset://']))
+      ->addLocator($locator_factory->createInstance('uri', ['uri' => 'php-file://']));

    Referencing the "php-file" stream wrapper here shows that we have some cleanup to do. I'm fine with cleaning it up in a follow-up, but maybe we can add @todo's here, to make it clear that this is not ideal. 

  4. +++ b/src/Plugin/libraries/Locator/ChainLocator.php
@@ -0,0 +1,87 @@
+  /**
+   * Constructs a chain locator.
+   *
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The Drupal config factory service.
+   * @param \Drupal\Component\Plugin\Factory\FactoryInterface $locator_factory
+   *   The locator factory.
+   * @param boolean
+   *   A boolean to indicate whether-or-not global locators (defined in conf)
+   *   should be added to the chain.
+   */
+  public function __construct(ConfigFactoryInterface $config_factory, FactoryInterface $locator_factory, $use_global) {
+    // Seed the locators list with any configured global locators.
+    if ($use_global) {
+      foreach ((array) $config_factory->get('libraries.settings')->get('global_locators') as $global_locator_conf) {
+        $this->addLocator($locator_factory->createInstance($global_locator_conf['id'], $global_locator_conf['configuration']));
+      }
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    return new static(
+      $container->get('config.factory'),
+      $container->get('plugin.manager.libraries.locator'),
+      isset($configuration['use_global']) ? $configuration['use_global'] : TRUE);
+  }

    Not sure about this. I think providing a global configuration for the locators is actually not a bad idea but I'm not sure this is the right place to hook it up.

    Maybe we could add a method for this to LocatorManager?

    We could also punt this to a follow-up, not sure.

Also would be nice to create the patch with -M so that the diff is not that big. Alternatively, you can create a feature branch for this issue, if you want. See #2825940: Remove LibrariesServiceProvider madness for more information on that.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Thanks! This is great feedback.

  1. I see this is a pre-existing issue, but the key should actually be "label" not "title".

    I see that you have cleaned this up for the existing schema over in #2825940: Remove LibrariesServiceProvider madness. I'll make sure to adjust any schema additions from this issue as well.

  2. We can actually provide a proper schema for the plugin configuration by specifying the types as e.g. "libraries.locator.[%parent.id]" and then provide schema for e.g. "libraries.locator.uri".

    I was wondering about this, but ended up sort-of short-circuiting the idea when I saw that plugins all took the same "type" and "configuration" inputs (the latter also being all scalar... I think). Anyway, as you noted formally defining the schema here is probably the right thing to do if taking this "global locators" idea forward.

  3. Referencing the "php-file" stream wrapper here shows that we have some cleanup to do.

    Agreed. Consolidating the bundled stream wrappers in some way could probably best be done in a separate issue.

  4. Not sure about this. I think providing a global configuration for the locators is actually not a bad idea but I'm not sure [using the ChainLocator] is the right place to hook it up. Maybe we could add a method for this to LocatorManager?

    Yeah, I totally understand that this feels a bit out of place. I'm not sure I follow how things would work with a new method on LocatorManager though. Do you mean some new logic that could then be called in LibraryTypeBase::onLibraryCreate() as a fallback? I'm wondering if perhaps the first step would be to just break out the configuration-based locator stuff into an actual locator plugin (e.g. ConfLocator). This could function much like ChainLocator but instead of the locators to check being pulled-in via a setter method they would be fetched via conf. This Locator could then be used in any number of ways.... perhaps passed into a ChainLocator chain (for more Library-type specific usage) or more "hardwired" into the module via something like a fallback in LibraryTypeBase (for true global usage).

Thanks for the diff tip. I think I'll spin-off a branch here.

  • rjacobs committed 2af9f79 on 2818689-chain-locators 
    Issue #2818689: Add ChainLocator and refactor StreamLocator into...
rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

I've started a feature branch for this. The last commit is a fairly simple one that only adds the ChainLocator and UriLocator. It does not include any global/conf locator stuff. Given that the details on taht are still being worked out I'll isolate that in separate commit(s).

  • rjacobs committed 6e02257 on 2818689-chain-locators 
    Issue #2818689: Add global locator.
rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Needs work » Needs review

The previous commit is a new approach for the "global locators" idea. As per my notes in #13 it adds a GlobalLocator plugin that acts very much like the ChainLocator, but uses the global conf to determine which locators to check. On one hand it feels strange to create yet another locator plugin for this, but I think this could be a very flexible way to encapsulate things. At the moment this locator is invoked as a "fallback" to the other lib-type specific locators within LibraryTypeBase::onLibraryCreate(), but in it's current form it would be very easy to move around.

I still want to add some test coverage, but marking "Needs Review" now as I think the concept is in a shape for new consideration.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Hiding outdated patch files.

  • rjacobs committed aed9638 on 2818689-chain-locators 
    Issue #2818689: Add GlobalLocator integration test.
rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

#19 adds a test for the GlobalLocator. I'm not sure if I'm nesting it correctly in the test directory tree or if this should be added as another method on an existing test, but this does add basic coverage for the feature.

I also thought about adding an alter hook on the global locators that are checked (mainly to allow profiles to declare their own shipped sources) but is occurs to me that they could also just use a configuration override to do the same thing. I'm not sure on that... maybe that's a follow-up.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Regarding the unknowns about the shipped stream wrappers (#8 and #9) I broke of a follow-up: #2841085: Finalize and consolidate shipped stream wrappers.

  • rjacobs committed 769aded on 8.x-3.x 
    Issue #2818689 by rjacobs, tstoeckler: Add ability to scan multiple...
rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented
Status: Needs review » Fixed

I went ahead and merged this into the main 8.x-3.x branch. Tobias, I hope that's alright for the moment. I'd normally hold off pending some additional reviews, but I was noticing that this was getting a bit tangled-up with a couple other pending issues. Since it seems like we have a consensus on the general concept, and since nearly all of the new pieces are pretty well encapsulated, I figured this would be a safe merge. I did open-up a follow-up however to ensure this is still flagged confirmation (notably related to the test addition... as I'm not sure I'm being as eloquent as I can with that part of the change):

#2848254: Confirm tests for global locators.

I'm happy to continue refining if needed :)

Status: Fixed » Closed (fixed)

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