Problem/Motivation

hook_library_alter() was previously deprecated, and was "removed" in #2392717: Remove hook_library_alter() from theme.api.php, but it was not actually removed. The actual invocation of the hook was left in and only the documentation was changed.

Proposed solution

Finish removing it.

API Changes

Deprecated hook_library_alter() will be removed (for real this time).

Notes

All the instances of hook_library_alter() has been removed in #2390707: Remove hook_library_alter() implementations
hook_library_alter() has been removed already from theme.api.php in #2392717: Remove hook_library_alter() from theme.api.php
There is already a change record addressing its deprecation.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it contains only removal of deprecated code.
Issue priority Normal because it contains only removal of deprecated code.
Prioritized changes The main goal of this issue is removing code already deprecated for 8.0.0.
Disruption Non-disruptive for core/contributed and custom modules because the code has been already marked deprecated (and documentation of it already removed) so it shouldn't be used anywhere.
CommentFileSizeAuthor
#23 interdiff.txt831 byteslauriii
#23 remove_deprecated-2472547-23.patch4.99 KBlauriii
#12 interdiff.txt916 byteslauriii
#12 remove_deprecated-2472547-12.patch4.39 KBlauriii
#8 interdiff.txt1.93 KBlauriii
#8 remove_deprecated-2472547-8.patch3.73 KBlauriii
#3 2472547-3.patch1.83 KBlauriii
#3 interdiff.txt987 byteslauriii
#1 2472547-1.patch747 byteslauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Active » Needs review
FileSize
2472547-1.patch747 bytes

Status: Needs review » Needs work

The last submitted patch, 1: 2472547-1.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Needs review
FileSize
interdiff.txt987 bytes
2472547-3.patch1.83 KB

There was still tests for alter.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Title: Remove unused hook_library_alter » Remove deprecated hook_library_alter()
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Related issues: +#2390707: Remove hook_library_alter() implementations, +#2392717: Remove hook_library_alter() from theme.api.php
Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Reviewed & tested by the community

RTBC, thanks Laurii!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Needs work

Right this was the runtime hook, let's drop it.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -85,8 +85,6 @@ public function getLibrariesByExtension($extension) {
-        $this->moduleHandler->alter('library', $definition, $library_name);
-        $this->themeManager->alter('library', $definition, $library_name);

We can remove the module handler and theme manager from the class itself now.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Needs review
FileSize
remove_deprecated-2472547-8.patch3.73 KB
interdiff.txt1.93 KB

Good catch!

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC, changes look good.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Needs work

Can we please cleanup the test to not longer create mock objects for the module handler / theme handler?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs beta evaluation

Also, since this is a normal task, let's add a beta evaluation and examine how the change fits during the Drupal 8 beta.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
remove_deprecated-2472547-12.patch4.39 KB
interdiff.txt916 bytes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Issue tags: -Needs beta evaluation
Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC ...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: remove_deprecated-2472547-12.patch, failed testing.

lauriii queued 12: remove_deprecated-2472547-12.patch for re-testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Reviewed & tested by the community

Setting back to RTBC because testbot was a little drunk.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Assigned: Unassigned » xjm

Reviewing this.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Issue tags: +drupaldevdays

So this confused me at first, because the hook did not appear to be marked deprecated -- it just appeared to be undocumented. What actually happened is that #2392717: Remove hook_library_alter() from theme.api.php removed the documentation of the hook... but not the test coverage or the invocation. Oops! So this change is allowed during the beta.

I updated the summary to explain this. (Note that removing deprecated code is not unfrozen; that's only strings, markup, migrate, docs, etc.) I also updated the title of the change record: https://www.drupal.org/node/2391981/revisions/view/8368695/8371373

Now reviewing the patch itself. :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Assigned: xjm » Unassigned

Actually @dawehner has more feedback.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work

We can drop 

  /**
   * The mocked module handler.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
   */
  protected $moduleHandler;

  /**
   * The mocked theme manager.
   *
   * @var \Drupal\Core\Theme\ThemeManagerInterface|\PHPUnit_Framework_MockObject_MockObject
   */
  protected $themeManager;

as well in that test file.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Reviewed & tested by the community
FileSize
remove_deprecated-2472547-23.patch4.99 KB
interdiff.txt831 bytes

Fixed the documentation :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Somewhat related #2472119: Extension installers allow extensions with duplicate names to be enabled.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Issue tags: +D8 Accelerate Dev Days

Tagging

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -D8 Accelerate Dev Days

Not really a part of the criticals and performance sprint, so removing that tag. :)

Per #19, this is okay to go in since the hook was already deprecated for removal 8.0.0 and we just missed a bit of the patch previously. Committed and pushed to 8.0.x

  • xjm committed 3f3ebc7 on 8.0.x 
    Issue #2472547 by lauriii, dawehner: Remove deprecated...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Performance, +D8 Accelerate Dev Days
Related issues: +#2050269: hook_library_info_alter() is not called for themes

This should have fixed the performance regression that #2050269: hook_library_info_alter() is not called for themes caused! :)

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

We need a quick follow-up issue to fix the comments:

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -85,8 +69,6 @@ public function getLibrariesByExtension($extension) {
         // Allow modules and themes to dynamically attach request and context
         // specific data for this library; e.g., localization.

nit - We forgot to remove this comment, which is no longer true.

Status: Fixed » Closed (fixed)

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