Problem/Motivation
It is currently up to any user of the library.discovery service (\Drupal\Core\Asset\LibraryDiscovery) to invoke hook_library-alter().
(Not to be confused with hook_library_info_alter(), which does get invoked by the responsible service.)
This is bad for:
- DX — it's annoying to do, unlike anything else, and fragile (if it's forgotten, then
hook_library_alter()won't be invoked, with bad consequences…) - performance — the results of
hook_library_alter()are not statically cached as they should be and could hence result in the same hook being invoked multiple times (it should be called by any consumer of thelibrary.discoveryservice)
On top of that, there currently is zero test coverage for hook_library_alter()!
Proposed resolution
Let LibraryDiscovery call it.
Remaining tasks
Review.
User interface changes
None.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | fix_hook_library_alter-2378263-10.patch | 10.52 KB | wim leers |
Comments
Comment #1
wim leersComment #3
wim leersBasically every test is failing on testbot… but not locally. Something fishy is going on with testbot. Retesting.
Comment #4
dawehnerPatch looks fine!
HAHA
I know its "out of scope" but can we think about not executing an alter hook for each library but rather once for all? What bout a follow up for both the static and dynamic alter hook?
Comment #7
hampercm commentedI attempted to test the patch submitted in #1 to see what was up. Applying the patch to an existing install of 8.0.x immediately broke the site. My attempts to recover by following the procedure at https://www.drupal.org/documentation/rebuild were unsuccessful, as well as attempts to perform a new install after zapping the database, settings.php, etc.
Browsing to /core/install.php at this point gave the following output:
Running the patch through simplytest.me produced similar output. I'm guessing the patch that was submitted is in some way incomplete/incorrect.
Comment #8
wim leers#4
#7: thanks for confirming the bug :)
Fixed the patch. I forgot to include the hunk that updates
core.services.yml… /facepalm!Comment #10
wim leersOk, so thanks to the implicit test coverage of
hook_library_alter()for Color module and Locale module (which has a test forhook_library_alter(), but pretends it forhook_library_alter()… fixed!), I found a pretty crucial oversight in the patch, causing those 3 failures!Comment #11
catchBlocks #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests, bumping to critical.
Comment #12
dawehnerAlright!
Comment #13
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7d36a7c and pushed to 8.0.x. Thanks!