Currently there are 3 lanaguage loaders in the libraries module. css, js and php all being called form the libraries_load_files

What would be really good is that these are converted into Drupal plugins which handle the loading of these file types.

This would mean that if there were other file types that people what to load they can create their own without having to modify libraries.

1 example of this would be for the Sassy module to add a SCSS loader so instead of using CSS you can load SASS directly from within a library.

or

if you want server side js using the v8js extension someone can create this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

I totally agree that this makes sense. I think the next step for D8, however, is to move libraries_info() into a proper LibraryManager (i.e. PluginManager) then we have a basis for moving code into classes and utilizing the new concepts.

tstoeckler’s picture

Title: Convert Language Loaders into plugins » Turn hardcoded library_load_files() into a more modern, flexible system
Issue summary: View changes

Retitling. Working on an implementation now and it turns out that plugins are in fact not necessarily the way to go here. We'll see.

The problem is that different types of libraries need different integration points. Asset libraries in D8 no longer require an explicit 'load' step, but they need to be registered in the (core) hook_library_info_build().

  • b6d1d8d committed on 8.x-3.x
    Issue
    Issue #2090623 by tstoeckler: Provide a modern, flexible library...
tstoeckler’s picture

So this first commit allows modules to specify
library_dependencies: in their .info.yml files and then it adds those libraries in hook_libraries_info_build(), so they can be used in render arrays with #attached.

A lot of pieces are still missing, but it's a start.

  • 9d50a33 committed on 8.x-3.x
    Issue #2090623 by tstoeckler: Add a test for PHP file loading
    
rjacobs’s picture

Status: Active » Needs review
FileSize
551 bytes

I don't have any comments (yet) on the main issue, though I noticed that the test added in #5 is missing a @group annotation. This kills local automated testing as well as d.o. testbot tests for any projects declaring Libraries API as a dependency. I'm attaching a patch for that here, but I'd be happy to submit it in a separate issue if that makes more sense (using this issue seems the most logical as it looks like it will contain an iterative set of commits).

Edit: For reference here's an example testbot failure on a separate project that declares a dependency on Libraries API (note the error at the end of the review log): https://qa.drupal.org/pifr/test/719133

rjacobs’s picture

Opps, patch in #6 had accidental whitespace.

  • dc772ca committed on 8.x-3.x
    Issue #2090623 by tstoeckler: Add an external library registry
    
rjacobs’s picture

Status: Needs review » Active

Thank you. It looks like the commit in #8 also addressed my issue from #6. I'll return the status back to "active" given that no patch is currently pending review.

The last submitted patch, 6: libraries-missing_test_group-2090623-6.patch, failed testing.

Status: Active » Needs work

The last submitted patch, 7: libraries-missing_test_group-2090623-7.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Active
rjacobs’s picture

It looks like these base structures are shaping-up in a really decoupled way, and everything looks really flexible. There are several layers of abstraction that I need to wrap my head around here before I could be all that useful, but this really looks like some foundational work. I suppose it's safe to say that this issue is really a base for just about every other major issues in the 8.x-3.x queue.

Using a stream wrapper for the library definitions will make things a whole lot more future-proof. It seems like currently the intent is to have a single (configurable) store for the definitions. If/when there is a canonical remote location for this (i.e. #773508: [meta] Provide a central repository of library information (i.e. libraries.drupal.org)) then things will be easy. However, I wonder what the interim or alternative approach might look like? Would we want to encourage a singular local path (e.g. /sites/all/libraries/definitions/) where users could place the definition files? If this were the case then module developers would need to offer users the option, and additional install step, to download a lib definition (sort of in lieu of the data formally passed in hook_libraries_info()) until that info could be registered in a more central way. Another approach could be to support an alternative location within each extension folder (lazily detected and cached) and then use that info as a fallback if no entry is found in the canonical registry. I don't want to get too wrapped-up in contingency considerations at this early stage, but it seems like some bridge may be needed from the old model to the new one. Or is the intent to make a totally clean break from the "old"?

For Asset libraries I think I follow what's going in libraries_library_info_build() (thanks for getting hook_library_info_build()) into core). What I'm not sure about is where the local library detection, and mapping with definitions, would live (logic from libraries_detect(), libraries_get_libraries(), etc.). Would that be part of the Asset loader itself or decoupled into some other component of some sort?

tstoeckler’s picture

Thanks for your feedback @rjacobs. It's great to have someone on board. These ideas have been spinning around in my head for quite some time now, and it's a good thing I'm finally putting them into some code, but it's even better - and in general invaluable - to have someone to bounce the ideas back and forth with. These are exactly the kinds of discussions we need to be having right now, IMO. Yay! :-)

Regarding the location of the library files. The idea was that if we do at some point want to have a canonical repository of library definitions in the future we probably want it to work like translations are now being downloaded in Drupal 8 core: Download them all in a single directory inside of the files directory. Just like the translations:// stream wrapper, we use a stream wrapper so that people can put the directory outside of their web root if they want, for security.

Libraries API in Drupal 7 already supported library files, and there it worked by scanning the sites/all/libraries, and sites/example.com/libraries directories, etc. for library definition files. The upside there is that it allowed sharing library definitions for people in a multi-site installation, while still allowing site-specific library-definitions at the same time. So I think we could consider allowing a similar thing in Drupal 8 as well, except we probably wouldn't be using the sites/all/libraries directory, because that will be not be the canonical place to put all libraries anyway (core went with core/assets for asset libraries, so I think we should do sites/all/assets as well, and if (!) we ever do anything with Composer libraries, we will want sites/all/vendor) so probably something like sites/all/library_definitions. So we could have a stream wrapper that checks if a file exists in sites/example.com/library_definitions, if not checks sites/all/library_definitions, and so forth. That might also make it easier for people to control their library definitions on custom sites. I.e. on custom D8 projects I currently have the problem that Drupal *always* checks for translation files, even if I have them in the repo directly, and that's something that using a different stream wrapper could avoid.

But I think we *definitely* want to have something like the current stream wrapper for people who do want to use the (eventual) central registry.

Re: libraries_detect(), libraries_get_libraries(): Yes, a lot of the parts are still missing at the moment. I am just about to commit an update here, which introduces LocalLibraryInterface which PHP file libraries use which has a getLibraryPath(), which replaces *a part of* libraries_detect(). The libraries itself are also discovered using stream wrappers, but now instead of lumping all of them in a single sites/all/libraries directory, now only the PHP file libraries use the sites/all/libraries directory and local asset libraries will use an assets directory to be inline with core. (See also above.) So that somewhat replaces libraries_get_libraries(). A big piece that is still completely missing is the version detection, that is the major missing piece right now I think. I think we can rip out the current "variant" handling and provide proper support for minified/source libraries for asset libraries, in any case I don't think that particular bit is super high on the todo list.

  • tstoeckler committed 73269e2 on 8.x-3.x
    Issue #2090623 by tstoeckler: Add support for local libraries and make...
amaria’s picture

Is there a reason to put the files in sites/all instead of a base libraries (or assets) folder? Modules such as webprofiler use libraries directly in the base directory.

rjacobs’s picture

@amaria, based on tstoeckler notes in #14 it would seem like leveraging /sites/*/assets as opposed to /assets would ultimately be more adaptable to multisite features of core. It would allow lib definitions, and libraries themselves, to be shared or overridden on a site-by-site basis. I suppose that for many situations this may not be so important, but when considering a common API such a feature would likely be an asset (no pun intended).

rjacobs’s picture

@tstoeckler, thanks for the notes in #14. The way you have described (and started to setup) the various stream wrappers seems quite solid. I don't yet follow fully how the connection to the canonical remote registry could leverage the same stream wrapper, but the specifics around this may be getting a bit off-topic (I'm assuming that a central remote connection handler could simply use the stream wrapper's local storage as a local cache of sorts?) Anyway I can take a look at the the analogous structures of Locale for more on that as well.

I've recently been pondering some of the topical CX and site builder perspectives on all this, specifically the ways the 3.x branch of the API could best be used/documented even before a central remote registry is available. I know that a release sans central remote registry is not necessarily a goal for the 3.x branch, but given that such a registry might be a little ways off, and given that we likely want maintainers who are currently porting projects to continue leveraging Libraries API, it might be something to consider early on. I suppose it could be as simple as defining best practices around the manual maintenance of instance-specific local registries. It seems that the current architecture could support several forward-compatible possibilities here while still keeping definitions decoupled from modules. Having these practices defined may also be useful for some use cases where a central registry connection is not possible (even after the registry exists). Perhaps a child issue, specific to some considerations here and any related conditional logic for the registry class would be warranted?

It sounds like you already have some very well-formed ideas around all this, and I'd be happy to help get them documented.

amaria’s picture

@rjacobs, Yeah I realized that a couple days after my comment. Also, the dev version works regardless of whether or not the library path is in the root or sites/all. Thanks.

  • tstoeckler committed 0268e6a on 8.x-3.x
    Issue #2090623 by tstoeckler: Fix path prepending for asset libraries
    
  • tstoeckler committed 1b5f539 on 8.x-3.x
    Issue #2090623 by tstoeckler: Introduce the concept of locators
    
  • tstoeckler committed 57bae40 on 8.x-3.x
    Issue #2090623 by tstoeckler: Introduce a stream wrapper for asset...
  • tstoeckler committed 9fb27dd on 8.x-3.x
    Issue #2090623 by tstoeckler: Provide support for remote asset libraries
    
  • tstoeckler committed ea27414 on 8.x-3.x
    Issue #2090623 by tstoeckler: Introduce the notion of library types
    
tstoeckler’s picture

Just pushed a few more updates to 8.x-3.x, including version detection.

Will try to update the IS here and in #1704734: [master] Libraries API 8.x-3.x soon, but I think what is missing now from actual functionality is proper dependency support, then Libraries API is actually somewhat usable in a way that's similar to D7. Then we can A) add lots of in-code documentation and B) hash out a battle-plan for the next steps. So this quick update is all for now, let's see...

tstoeckler’s picture

Status: Active » Fixed

Closing this one as well, I think the foundation of this is done, and we can track the remaining items in separate issues linking directly from #1704734: [master] Libraries API 8.x-3.x. Currently there are just too many issues to keep track of IMO.

Status: Fixed » Closed (fixed)

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