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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
8.66 KB

There are 3 implementations in core, for color, locale and quickedit. I replaced all of them in this patch.

Status: Needs review » Needs work

The last submitted patch, 1: rm_hook_library_alter-2390707-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.66 KB
837 bytes

Hah!

Fabianx’s picture

Issue tags: +D8 cacheability

Great work, Wim!

This also directly affects cache-ability.

Wim Leers’s picture

Logically, 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.

Wim Leers’s picture

Issue tags: +Ghent DA sprint
dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/quickedit/quickedit.module
@@ -70,43 +70,28 @@ function quickedit_page_attachments(array &$page) {
+    $alter_library = function(&$library, $theme) use (&$alter_library) {

Thats the exactly and only way to specify that recursive anonymous function reference: See http://3v4l.org/GsXYf

Wim Leers’s picture

Issue summary: View changes
ianthomas_uk’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Novice

This will need a change record before commit

Wim Leers’s picture

Title: Remove hook_library_alter() » Remove hook_library_alter() implementations
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record, -Novice

No, 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.)

ianthomas_uk’s picture

Generally 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).

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I 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:

 * @deprecated in Drupal 8.0.x, will be removed before Drupal 8.0.0
 *   Use hook_library_info_alter() and hook_js_settings_alter().

(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!

  • webchick committed ff20a6f on 8.0.x
    Issue #2390707 by Wim Leers: Remove hook_library_alter() implementations
    
Wim Leers’s picture

Further refined the CR a bit, and published it.

Status: Fixed » Closed (fixed)

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

mermentau’s picture

The replacement hook_library_info_alter() is causing issue with Mayo contrib theme as seen https://www.drupal.org/node/2404253