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.
Problem/Motivation
In the class composer, file core/lib/Drupal/Core/Composer/Composer.php, Wrong @param doc for the method vendorTestCodeCleanup
Comment | File | Size | Author |
---|---|---|---|
#25 | 2614764-24.patch | 727 bytes | snehi |
#25 | interdiff.txt | 771 bytes | snehi |
#23 | 2614764-23.patch | 727 bytes | snehi |
#23 | interdiff.txt | 777 bytes | snehi |
#19 | interdiff.txt | 500 bytes | priya.chat |
Comments
Comment #2
rakesh.gectcrComment #3
jhodgdonThanks for the patch! Sorry for the delay in reviewing -- I've been on vacation and no one else, unfortunately, ever reviews docs patches. :(
This also needs a documentation line added.
Comment #4
rakesh.gectcrComment #5
jhodgdonThanks!
Hm. OK, not sure what this means, but presumably "composer" should be capitalized at least?
And if this is of class PackageEvent, why is it documented as 'a script event" object? It might be right, but please check. I would think it's a "package" event, whatever that is.
Comment #6
amit.drupal CreditAttribution: amit.drupal at gai Technologies Pvt Ltd commentedComment #7
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedcomposer is still not capitalized here. Where is the interdiff ?
Please work on the assigned issue this issue is already assigned to rakesh.
Comment #8
rakesh.gectcr@snehi
Well said.
@amit.drupal
before giving patch, Please understand the community workflow.
Comment #9
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAssigning to myself due to inactivity.
Comment #10
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedPlease review the attached patch. I just started from #4
Comment #11
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #12
jhodgdonThanks for the new patch!
Hm...
So this @param documentation is probably correct, but it doesn't tell me anything that I cannot read from the function signature. I have no idea what a PackageEvent object is, or why you would need to pass one into this function, or what it is used for.
So... although it is not incorrect, I don't think this is very good documentation. If we're going to fix up this @param, let's figure out what this really is, why it's needed, and what it's used for, and put that into the documentation. OK? Thanks!
Comment #13
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedSo you want me to change @param documentation as well as function signature's work ?
Comment #14
jhodgdonThe issue title is "Wrong @param doc". Yes, change the param doc. :) I do not think we are changing the function signature.
Comment #15
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi All,
I have created a patch, though its not perfect, its a start and open for others to further enhance it.
About the PackageEvent, its a class that extends InstallerEvent class, which is An event for all installer. It passed any event related to installer as an argument. Kindly provide more suggestions for this.
Comment #16
jhodgdonFormatting: The lines after @param need to be wrapped into a nice paragraph, as close to 80 character lines as possible.
Documentation... So when you are documenting a parameter, the aim is to document what the parameter is used for in the method, or how it affects the outcome of the method.
This documentation doesn't tell me that. It is just trying to tell me what the PackageEvent class is, which I could/should get from reading the class documentation, not from reading a @param documentation line.
So what we need to document here is what $event is used for in the function.
This function's one-line intro says:
Remove possibly problematic test files from vendored projects.
So look at the code and figure out how $event is related to that process, and put that into the documentation.
Thanks!
Comment #17
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedHello,
I have tried to add some documentation for the event object, Please review my patch.
Comment #19
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedSorry for the previous attachments Please ignore that. I am uploading corrected patch and interdiff files. I have taken the last patch and added my changes according to that.
Comment #20
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedComment #21
jhodgdonAh, OK, that is better -- it actually tells me what $event is being used for. Thank you!
Now, let's fix the standards/formatting:
The documentation lines need to be indented by 2 spaces.
Also... maybe a bit of rewriting... I was able to figure out what you were trying to say, but it doesn't really make sense as it is written.
Maybe something like:
A PackageEvent object to get the configured composer vendor directories from.
??? And I didn't read the rest of the code in the module to see if $event is used for anything other than that first line.
Comment #22
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #23
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedLong time, i am back with new energy from DrupalCon Asia.
Here is the patch and interdiff.
Comment #24
jhodgdonThanks! Almost there, but:
Wrapping nitpick: The word "directories" will fit on the previous line.
Comment #25
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedSorry for silly mistake.
Here is the final patch with interdiff.
Comment #26
jhodgdonLooks good now, thanks! We all (even me, frequently!) make wrapping mistakes from time to time -- this is why we have reviews. ;)
Comment #27
snehi CreditAttribution: snehi at Publicis Sapient for Publicis Sapient commentedThanks for patience and time.
Comment #31
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x and 8.0.x. Thanks!
Comment #33
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented