This is a follow-up to #2815189: Library dependency checking can fail for instal profile (and also related to #2090623: Turn hardcoded library_load_files() into a more modern, flexible system). It's not urgent, but I wanted to capture some notes on it before I forget.

We are currently implementing our own Extension Hander service along with a custom Extension wrapper, which introduces a couple new layers of abstraction. As far as I can see these classes are currently only used to fetch some data from *.info.yml files that declare library_dependencies. I suppose that having things setup this way makes perfect sense as it encapsulates an extension-specific calculation with the Extension object itself. However, it also looks like the best way to traverse the *.info.yml data is with a static call to system_get_info(), which is not extension-specific in scope. So we end-up building custom Extension wrappers that have to then use global functionality to do their job. I'm wonder if it might make sense to refactor a bit and centralize the *.info.yml lookup within the Library Manager service instead.

Of course this suggestion could easily be misguided, specifically if:

  • There was an alternative to system_get_info() that fetches *.info.yml data, preferably something that could be injected into the Extension object itself (unit testable).
  • There are plans to further develop the custom Extension Hander service and Extension wrapper to do more than fetch library_dependencies data.

If any of those are true, then don't hesitate to close this :)

CommentFileSizeAuthor
#5 2816155-kill-extension-manager.patch9.97 KBtstoeckler
#5 interdiff.txt1.63 KBtstoeckler
#3 2816115-3.patch9.99 KBrjacobs

Comments

rjacobs created an issue. See original summary.

rjacobs’s picture

rjacobs commented
Related issues: +#2815189: Library dependency checking can fail for instal profile

Adding relation to #2815189: Library dependency checking can fail for instal profile.

Note also that I don't mean to nit-pick existing architecture... this is mostly just a follow-up to make sure that I understand the design intentions of this part of the module.

rjacobs’s picture

rjacobs commented
Status: Active » Needs review
StatusFileSize
new 2816115-3.patch9.99 KB

Here's a go at the idea... for the testbots.

rjacobs’s picture

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

This is also architecturally related to #2090623: Turn hardcoded library_load_files() into a more modern, flexible system.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new interdiff.txt1.63 KB
new 2816155-kill-extension-manager.patch9.97 KB

Yes, you are 100% correct. system_get_info() is kind of a shitty tool, but is precisely the correct tool for this job. I completely agree with this issue.

There are plans to further develop the custom Extension Hander service and Extension wrapper to do more than fetch library_dependencies data.

I always had in mind to provide something like a library_suggestions or something like that for "optional dependencies", but even that probably wouldn't justify this abstraction I guess.

Some notes on the actual patch:

  1. +++ b/src/ExternalLibrary/LibraryManager.php
@@ -72,9 +60,12 @@ class LibraryManager implements LibraryManagerInterface {
+        assert('!empty($info)');

    Any reason for this, I don't really understand. 

  2. +++ b/src/ExternalLibrary/LibraryManager.php
@@ -72,9 +60,12 @@ class LibraryManager implements LibraryManagerInterface {
+        $library_ids += isset($info['library_dependencies']) ? $info['library_dependencies'] : [];

    += won't work here because these are indexed arrays, not associative arrays. 

  3. +++ b/tests/src/Kernel/ExternalLibrary/Asset/AssetLibraryTestBase.php
@@ -12,7 +12,7 @@ abstract class AssetLibraryTestBase extends LibraryTypeKernelTestBase {
+   * \Drupal\libraries\ExternalLibrary requires system_get_info() which is in

    It's actually LibraryManager.

Since I had the patch applied locally anyway to take a look at it in context, took the liberty of quickly updating it for the above. Feel free to improve on that (i.e. bring back the assert() which I removed, because I don't understand it, but there of course might be a reason for it, that I am simply missing) and commit some form of this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Priority: Minor » Normal
Status: Needs review » Fixed

Went ahead and committed this. As always, we can improve on this if you disagree with #5 or have other idea.

Thanks again, this is really nice!

rjacobs’s picture

rjacobs commented

Thanks! It looks like the commit didn't get attached to the issue correctly, but it is in there, so that's great.

I'm coming-back to this a bit later than I'd like so I suppose it might be a bit moot to get to your questions now :) ... but here's what I can say on #5:

+ assert('!empty($info)');

Any reason for this, I don't really understand.

The assertion was preserved from the (now removed) Drupal\libraries\Extension\Extension\getLibraryDependencies(). It was actually this assertion that tipped-me-off to #2815189: Library dependency checking can fail for instal profile as $info for profile-based extentions was coming up empty. Anyway, there is probably no harm in removing it now.

+ $library_ids += isset($info['library_dependencies']) ? $info['library_dependencies'] : [];

+= won't work here because these are indexed arrays, not associative arrays.

Oh yes, you are right. I'm not sure how I missed that.

+ * \Drupal\libraries\ExternalLibrary requires system_get_info() which is in

It's actually LibraryManager.

True true... especially now that this change is committed :)

Thanks!

Status: Fixed » Closed (fixed)

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