Validate library dependencies when extensions are installed and more gracefully deal with library-specific errors at runtime

This sounds like the parent for several smaller sub-features, IMHO. I'm wondering if we could consider which pieces of this are alpha-blockers and break those off? It seems to me that at a minimum we need to ensure that a missing definition or lib source is not fatal (maybe with some simple exception handling and logging?). For asset libraries this should be reasonable safe, I'm not sure about PHP libs however. The behavior could potentially be type-specific.

Treating libraries are truly enforced dependencies at install-time is definitely good, I just figure we can be a little lax about this for asset libs... at least before a fully stable release.

I started looking into this a bit in hopes that we can catch an fatal error on installation and possibly look at some other bits in a follow-up.

The hard part, however, is finding the right way to enforce this check. I.e. to intercept the module installation somehow

I think we have hook_modules_installed() and hook_themes_installed() as possible options. From what I can see these would be universally invoked (I quickly confirmed that it fires via both the UI install and a drush install). The tricky part is that the only "context" that we get is a list of names of modules or themes being installed. So we would need to manually call system_get_info() (and possibly system_rebuild_module_data()) to get the details from the info files. I suppose this could be a workable starting point though.

Then if the library is installed can be checked with

$library = \Drupal::service('libraries.manager')->getLibrary($library_name);
if ($library instanceof LocalLibraryInterface) {
$library->isInstalled();
}

For an uninstalled library it looks like getLibrary() throws errors before $library->isInstalled() can be checked. I think this is because of the version detection that is triggered (while getting the lib) in LibraryTypeBase::onLibraryCreate(). Hopefully sorting that out will be fairly minor.

Edit: I see now that something like hook_modules_installed() won't let us validate an installation before it completes (e.g. the "abort installation" part). Hummm. I guess that in the meantime we could at least use those hooks to alert the admin to missing steps in the installation process.

Depending on how layered we want to go before considering which parts could be broken off into separate issues, I figured we might as well work in a new branch. Linked issue to "2774313-validate-dependencies" branch.

The last branch commit bypasses the version check if a local library is not installed. This should avoid exceptions on a call like \Drupal::service('libraries.manager')->getLibrary($library_name); when a local library is not yet installed. This allows the definition data to still be fetched, and isInstalled() to be checked programmatically, etc.. I figure that getLibrary() should not throw an exception in such cases given that it contains methods that should be usable even if the library sources are not yet available.

It seems to me that a major part of all this is gracefully dealing with errors while an external library is being registered with core in hook_library_info_build(). This hook seems to be invoked whenever the core library cache is cold and a library (any library) is requested. This could happen at almost any time, so we probably need to be careful about exception handling to ensure that a simple problem with one library does not bring down the whole process.

I think it would be generally useful to be able to target broad groups of exceptions as specific to this module, or at least specific to broad categories of library errors (definition detection vs installation detection, etc.). Exceptions could extend a common module-specific class for this, but I see that we already have a LibraryIdAccessorTrait and LibraryAccessorTrait associated with exceptions. These traits seems to denote which exceptions are lib-specific and furthermore with are related to definition processing vs library processing. So I guess just associating them with an interface gives us the ability to target groups of exceptions easily in "upper" API-using layers (like hook_library_info_build()).

The last branch commit leverages this idea and adds some nice exception-based logging to libraries_library_info_build(). This should keep simple library not detected/installed errors from being fatal. Of course this still needs to be complimented with some form of detection/warning process at extention install time, but I figured this would be a good start. In fact this may be the minimum needed to address the "Critial" parts of this issue, so I'll go ahead and flag as "Needs Review".

I just want to bump this. As far as I can see the work in the current issue branch may be enough to address the "more gracefully deal with library-specific errors at runtime" part of the issue. This may also be one of the last (if not the last) blocker to an alpha release (see #2821282: Consolidate and Finalize Alpha-Blockers for 8.x-3.x). Just want to see if anyone has any comments.

I went ahead a merged the changes from #9 and #11 into 8.x-3.x.

At this stage in the 8.x-3.x lifecycle I figure it's best to get something in place to deals with the fatal errors (linked to common lib definition and src problems) even while work on this issue continues. I'm hopeful this also addresses the "critical" part of the issue and will move the priority down a notch as a result. This may also remove this from the alpha-blockers list, but some additional confirmation on that might be warranted.

So branch micro-commit #9 introduced a test failure that somehow I overlooked. It turns out that it's not so straightforward to calculate when to skip version detection (in Drupal\libraries\ExternalLibrary\Type\LibraryTypeBase::onLibraryCreate()) as the logic there needs to be compatible with libs of all types and interfaces (local, remote, asset, php, etc) and some library objects share interfaces and traits that are specific to both local and remote libs. Anyway, to make a long story short, #9 introduced a test failure for a remote library test case, and at this point it's probably easiest to just revert that bit. The most important micro-commit (#11) is still in place so the state of the issue is unchanged.

At some point we need to decide if it makes sense to allow a library object to be loaded without an exception even when the library sources are not available (and the version detection cannot be run, etc.). This could be dealt with later in a separate issue.

I also closed the 2774313-validate-dependencies branch since it had already been merged (squashed) into 8.x-3.x. I suppose that a squash merge only makes sense if work on the feature branch is complete, otherwise perhaps we should just be using a no-ff merge? The best-practice branch workflow for d.o. projects is not yet clear to me. Anyway, a new branch could certainly be opened for this issue if that makes sense moving forward, I just wanted to sort of "reset" things back to 8.x-3.x for now.

Hi, any news about this?

Using hook_modules_installed() seems to make sense, has anyone experimented with it?

The attached patch forces re-registering external asset libraries when new modules are installed, this means basically that libraries_library_info_build() gets called again and does its thing.

This may not be ideal because all the modules with library_dependencies in the info file get reprocessed, but since installing new modules is a rare event, I think we can live with the overhead.

One thing I noticed is that the library definitions are saved into the public://library-definitions dir, but the actual external library (the zip file, for example) is not downloaded and extracted automatically. Should I open another issue for that?