Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the hook documentation uses the wrong function signature
Issue priority Normal because there are no big changes.
Unfrozen changes Unfrozen because it only changes documentation.
Prioritized changes Probably not a prioritized change.
Disruption No disruption at all.

Problem/Motivation

hook_library_info_alter() refers to $module, but when the alter hook is invoked $extension is passed. This makes more sense because both modules and themes can define libraries.

Code from Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo():

    // Allow modules to alter the module's registered libraries.
    $this->moduleHandler->alter('library_info', $libraries, $extension);
    $this->themeManager->alter('library_info', $libraries, $extension);

Proposed resolution

Update $module to $extension and change the documentation accordingly.

Remaining tasks

Patch
Patch review

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#26 2490936-26.patch2.71 KBcbanman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,135 pass(es). View
#24 2490936-24.patch2.71 KBcbanman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,133 pass(es). View
#21 2490936-21.patch2.71 KBcbanman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,135 pass(es). View
#15 2490936-15.patch2.73 KBashutoshsngh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,712 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding where the alter hook is invoked for reference.

joshi.rohit100’s picture

Status: Active » Needs review
FileSize
904 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,505 pass(es). View
sqndr’s picture

  1. +++ b/core/modules/system/theme.api.php
    @@ -848,10 +848,10 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + *   The name of the extension that registered a library.
    

    Here it says the library.

  2. +++ b/core/modules/system/theme.api.php
    @@ -848,10 +848,10 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    +function hook_library_info_alter(&$libraries, $extension) {
    

    While here it says the libraries.

joshi.rohit100’s picture

FileSize
1.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,508 pass(es). View
908 bytes
sqndr’s picture

@joshi.rohit100: I think you did an interdiff of the new patch vs the old patch, the lines you added are marked red. Thanks for fixing this so quickly.

I'll let Cottser review the patch as well. Looks good to me now.

Cottser’s picture

Title: hook_library_info_alter() docs are slightly out of date » hook_library_info_alter() docs and function signature are slightly out of date
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

Thanks @joshi.rohit100 @sqndr, looks good!

webchick’s picture

Assigned: Unassigned » jhodgdon

Makes sense to me, but passing to Jennifer just in case I'm missing something.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

This does make sense to me too, but maybe we should document that it can also be 'core', as in the example code? Because 'core' is not really an extension, per se.

Also to be very very clear, maybe it should say the "machine name"?

jhodgdon’s picture

By the way, I verified from looking at https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Asset!LibraryDisc... and functions it calls that indeed, the extension name passed into this hook is either 'core' or a module or theme machine name. So this change is good, maybe just not quite complete.

Cottser’s picture

Status: Needs review » Needs work

Great point, let's clarify that!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
andypost’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,491 pass(es). View

Hook should mention about modules and themes because both able to use this hook.
Also core use notion of asset for libraries so I changed a description.
The related protected method that calls that hook should be referenced.
No interdiff because patch totally different

PS: please check my grammar ;(

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This is looking better, and the grammar is pretty close. ;)

Still a couple of issues to address in the documentation:

  1. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,25 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * Alters assets (JavaScript/CSS) registry.
    

    Why are we calling this the "assets registry"? The terminology in the rest of the code, function names, etc. is "library" so let's keep it at that -- don't introduce new terms. So this should be:

    Alter JavaScript/CSS library information.

    [note: verb should be "Alter" not "Alters"]

  2. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,25 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * update library to newer version while ensuring backwards compatibility. In
    

    Several grammar/punctuation/typo fixes needed in this line. Should be:

    update a library to a newer version, while ensuring backward compatibility.

  3. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,25 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * general, such manipulations should only be done by designated modules and
    + * themes, since most modules and themes that integrate with a certain library
    + * also depend on the API of a certain library version.
    + *
    

    What is a "designated" module or theme? I don't understand what this is trying to say, exactly.

  4. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,25 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + *   The name of the extension that registered the libraries.
    

    For clarity, should say "machine name" not just "name" here.

    Also, since "core" is not an extension, that should be mentioned in this documentation.

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,665 pass(es). View
ashutoshsngh’s picture

FileSize
2.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,712 pass(es). View

Attached wrong patch in #14.

ashutoshsngh’s picture

Issue tags: +SrijanSprintNight
jhodgdon’s picture

Status: Needs review » Needs work

When you upload a patch on an issue that already had a patch, please make an Interdiff file. Thanks!

Anyway... thanks for the new patch, which is an improvement. But it still needs some work:

  1. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * update a library to a newer version, while ensuring backward compatibility.In
    

    There needs to be a space after the . here -- and then the line will be over 80 characters so you will need to rewrap the paragraph.

  2. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * update a library to a newer version, while ensuring backward compatibility.In
    + * general, such manipulations should only be done by the modules and themes
    + * that will extend this functionality, since most modules and themes that
    + * integrate with a certain library also depend on the API of a certain library
    + * version.
    + *
    

    Hm. The wording here is a bit confusing, but at least I see what the intent is this time. ;)

    How about something like this:

    In general, such manipulations should only be done to extend the library's functionality in a backward-compatible way, to avoid breaking other modules and themes that may be using the library.

  3. +++ b/core/modules/system/theme.api.php
    @@ -838,22 +838,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * @param string $extension
    + *   The machine name of the extension that registered the libraries.
    

    Again, please say something about core here, which is not an extension. See several of my previous reviews, which made the same comment, and which still hasn't been addressed... and just to point out, that was the whole point of this issue actually!

andypost’s picture

The related issue adds another way to override libraries so probably needs to be mentioned here somehow

andypost’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review

I'd like to point that render api separates assets and attachments so better to get feedback from @Wim Leers at least

+++ b/core/modules/system/theme.api.php
@@ -838,22 +838,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
+ * Alter JavaScript/CSS library information.

That should point that alter happens on extension's libraries (multiple)

jhodgdon’s picture

That other issue mentioned in #18 has not been finalized. It should be updating the related documentation when it is finalized, right? We can't document something here that isn't yet part of Core.

cbanman’s picture

FileSize
2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,135 pass(es). View
1.33 KB

Made suggested changes mentioned in #17 to most recent patch.

cbanman’s picture

andypost’s picture

+++ b/core/modules/system/theme.api.php
@@ -847,22 +847,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
+ * Alter JavaScript/CSS library information.

and again, Alter libraries provided by extension....

cbanman’s picture

FileSize
2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,133 pass(es). View
442 bytes

Right, I missed that. How's this then?
- Made change #23 suggested.

jhodgdon’s picture

Status: Needs review » Needs work

Looks good, thanks!

Two very minor grammar/punctuation/typo things to fix:

  1. +++ b/core/modules/system/theme.api.php
    @@ -847,22 +847,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * Alter libraries provided by extension.
    

    Should be "an extension" or "extensions".

  2. +++ b/core/modules/system/theme.api.php
    @@ -847,22 +847,26 @@ function hook_js_settings_alter(array &$settings, \Drupal\Core\Asset\AttachedAss
    + * Allows modules and themes to change libraries' definitions, mostly used to
    + * update a library to a newer version, while ensuring backward compatibility.
    + * In general, such manipulations should only be done to extend the library's
    

    the comma in this first line should be a ;

cbanman’s picture

FileSize
2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,135 pass(es). View
680 bytes

Made changes as per #25.

cbanman’s picture

Status: Needs work » Needs review
andypost’s picture

+1 rtbc

trwad’s picture

Issue summary: View changes

Added Beta Evaluation

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, as long as we have a glossary somewhere saying what an extension is. That's my only reservation. RTBC though, improvements all around.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

+1 for RTBC.

No, actually I'll just commit this. :) Thanks everyone!

  • jhodgdon committed a2fc985 on 8.0.x
    Issue #2490936 by cbanman, joshi.rohit100, ashutoshsngh, andypost,...
Cottser’s picture

Issue summary: View changes

(Cross-posting with commit, but for future reference…)

Updating the beta evaluation to be more accurate :)

andypost’s picture

Assigned: Wim Leers » Unassigned

Status: Fixed » Closed (fixed)

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