Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
hook_library_alter()
is bad because:
- it causes performance problems
- it encourages (or at least allows) for crazy things that actually belong in
drupalSettings
- it's oh so similar, yet subtly different, from
hook_library_info_alter()
— hence the DX is not great at all
Proposed resolution
Remove hook_library_alter()
implementations.
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#3 | rm_hook_library_alter-2390707-3.patch | 8.66 KB | Wim Leers |
Comments
Comment #1
Wim LeersThere are 3 implementations in core, for
color
,locale
andquickedit
. I replaced all of them in this patch.Comment #3
Wim LeersHah!
Comment #4
Fabianx CreditAttribution: Fabianx commentedGreat work, Wim!
This also directly affects cache-ability.
Comment #5
Wim LeersLogically, this has a >=2.5% performance improvement, because #2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all, which removed less work, led to a 2.5% improvement.
Comment #6
Wim LeersComment #7
dawehnerThats the exactly and only way to specify that recursive anonymous function reference: See http://3v4l.org/GsXYf
Comment #8
Wim LeersComment #9
ianthomas_ukThis will need a change record before commit
Comment #10
Wim LeersNo, it does not, because this does not remove the hook, it only removes the implementations of the hook. Actually removing the hook is for a follow-up issue.
I already clarified the IS in my previous comment, but I had not yet updated the issue title, did that now.
(dawehner preferred this, and I'm fine with it.)
Comment #11
ianthomas_ukGenerally they've been being required for the remove usages issues. It's a good time to write them, because you're doing what the change record needs to tell people to do, and it means the removal issue is simple. For functions that are deprecated for removal in D9, it means we actually have documentation about the recommended API (we should write one whenever we deprecate an API).
Comment #12
webchickI don't know what the specific policy there is (we have way too many policies imo), but I went ahead and wrote a change record since I wanted to understand what this issue was about in order to review/commit it. See https://www.drupal.org/node/2391981. Most of that is taken from a brief convo with Wim, but still needs confirmation before publishing.
So in essence, what this patch is doing is bringing the libraries system inline with the current reality, which is that all forms of JS, including (more recently) JS settings, are all piped through the central libraries system. Therefore, a run-time version of this hook is no longer required, and it has great performance benefits to not do that.
So since this patch is mostly moving code around, and since it helps resolve a performance critical, I think this is fine to go in. The only change I made is I marked hook_library_alter() deprecated, to be removed before 8.0.0, because shipping with two ways to do the same thing that act in subtly different ways seems like a recipe for major confusion, and it helps performance:
(If that's not correct, I can always revert that change later.)
Apart from that, this looks fine, so committed and pushed to 8.0.x. Thanks!
Comment #14
Wim LeersFurther refined the CR a bit, and published it.
Comment #16
mermentau CreditAttribution: mermentau commentedThe replacement hook_library_info_alter() is causing issue with Mayo contrib theme as seen https://www.drupal.org/node/2404253