Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Beta phase evaluation
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
Comment | File | Size | Author |
---|---|---|---|
#26 | 2490936-26.patch | 2.71 KB | cbanman |
#24 | 2490936-24.patch | 2.71 KB | cbanman |
#21 | 2490936-21.patch | 2.71 KB | cbanman |
#15 | 2490936-15.patch | 2.73 KB | ashutoshsngh |
Comments
Comment #1
star-szrAdding where the alter hook is invoked for reference.
Comment #2
joshi.rohit100Comment #3
sqndr CreditAttribution: sqndr as a volunteer commentedHere it says the library.
While here it says the libraries.
Comment #4
joshi.rohit100Comment #5
sqndr CreditAttribution: sqndr as a volunteer commented@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.
Comment #6
star-szrThanks @joshi.rohit100 @sqndr, looks good!
Comment #7
webchickMakes sense to me, but passing to Jennifer just in case I'm missing something.
Comment #8
jhodgdonThis 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"?
Comment #9
jhodgdonBy 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.
Comment #10
star-szrGreat point, let's clarify that!
Comment #11
jhodgdonComment #12
andypostHook 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 ;(
Comment #13
jhodgdonThanks! This is looking better, and the grammar is pretty close. ;)
Still a couple of issues to address in the documentation:
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"]
Several grammar/punctuation/typo fixes needed in this line. Should be:
update a library to a newer version, while ensuring backward compatibility.
What is a "designated" module or theme? I don't understand what this is trying to say, exactly.
For clarity, should say "machine name" not just "name" here.
Also, since "core" is not an extension, that should be mentioned in this documentation.
Comment #14
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedComment #15
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedAttached wrong patch in #14.
Comment #16
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedComment #17
jhodgdonWhen 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:
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.
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.
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!
Comment #18
andypostThe related issue adds another way to override libraries so probably needs to be mentioned here somehow
Comment #19
andypostI'd like to point that render api separates assets and attachments so better to get feedback from @Wim Leers at least
That should point that alter happens on extension's libraries (multiple)
Comment #20
jhodgdonThat 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.
Comment #21
cbanman CreditAttribution: cbanman at Acro Commerce commentedMade suggested changes mentioned in #17 to most recent patch.
Comment #22
cbanman CreditAttribution: cbanman at Acro Commerce commentedComment #23
andypostand again, Alter libraries provided by extension....
Comment #24
cbanman CreditAttribution: cbanman at Acro Commerce commentedRight, I missed that. How's this then?
- Made change #23 suggested.
Comment #25
jhodgdonLooks good, thanks!
Two very minor grammar/punctuation/typo things to fix:
Should be "an extension" or "extensions".
the comma in this first line should be a ;
Comment #26
cbanman CreditAttribution: cbanman at Acro Commerce commentedMade changes as per #25.
Comment #27
cbanman CreditAttribution: cbanman at Acro Commerce commentedComment #28
andypost+1 rtbc
Comment #29
chananapeeyush CreditAttribution: chananapeeyush as a volunteer and commentedAdded Beta Evaluation
Comment #30
star-szrThis 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.
Comment #31
jhodgdon+1 for RTBC.
No, actually I'll just commit this. :) Thanks everyone!
Comment #33
star-szr(Cross-posting with commit, but for future reference…)
Updating the beta evaluation to be more accurate :)
Comment #34
andypost