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.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff-2358991-19-21.txt | 2.58 KB | er.pushpinderrana |
#21 | remove_usage_of-2358991-21.patch | 17.21 KB | er.pushpinderrana |
#19 | interdiff.txt | 6.85 KB | JeroenT |
#19 | remove_usage_of-2358991-19.patch | 17.09 KB | JeroenT |
#17 | remove_usage_of-2358991-17.patch | 17.05 KB | JeroenT |
Comments
Comment #1
JeroenTRemoved usages of the drupal_mail() function. Patch attached.
Comment #2
javivf CreditAttribution: javivf commentedPatch applied and the function only exist in core/includes/mail.inc
Comment #3
alexpottThe code standards says that this should be a one liner.
Should be an @see to the method on the interface. This needs to be fixed elsewhere in the patch.
Comment #4
JeroenTAddressed the changes as suggested by alexpott in #3.
Comment #5
rpayanmComment #6
Alienpruts CreditAttribution: Alienpruts commented@rpayanm : you shouldn't just set this to Reviewed and tested by the community.
Not that I doubt your skills as a developer, but usually a patch needs to be thorougly reviewed by some developers before this status is changed.
I have requested a thorough review by other developers in the #drupal-contribute IRC channel, I'm sure they will take a look at it and give their respective opinions on it. :)
Tnx for the efforts :)
EDIT : if you want to read up on the normal review process, may I recommend https://www.drupal.org/node/156119 ?
Comment #7
aspilicious CreditAttribution: aspilicious commentedThis exceeds 80 chars, hmm not sure how we can fix this.
Same story exceeds 80 chars.
And again
One more
It's tricky because the new syntax takes way more space in the docs.
Comment #8
jamesdixon CreditAttribution: jamesdixon commentedLets bust the long pieces of code down to the next line. May not be the prettiest but is very readable IMHO.
Comment #9
Ashok Negi CreditAttribution: Ashok Negi commentedChanging status to Needs Work. Please remove drupal_mail from comment sections. See attached screenshot for reference.
Comment #10
jamesdixon CreditAttribution: jamesdixon commented@Ashok, thanks for the feedback. The usages of drupal_mail inside of core/includes/mail.inc are documenting the deprecated function definition of drupal_mail.
Should we remove the drupal_mail function definition and it's documentation block above?
Comment #11
JeroenTthere is a seperate issue for removing the function drupal_mail itself: #2359457: Remove drupal_mail().
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedYou should be using the actual class interface name here.
Here too
And here
Suffice it to say, there are a few more.
Code seems fine, just the docs need some work.
Comment #13
rpayanmTrying...
Comment #14
ianthomas_ukThe relevant interface is \Drupal\Core\Mail\MailManagerInterface. Some inline documentation was updated to refer to MailManagerInterface on #2358993: Remove usage of drupal_mail_system() when in fact they should have been referring to \Drupal\Core\Mail\MailInterface.
I know it's not a change introduced by this patch, but shouldn't that be "prior to sending"? I figure we might as well correct that as we're changing this line anyway.
As there aren't standards for when to use fully qualified namespaces yet, I've just used the interface name in the interface itself and when the fully qualified name has already been used earlier in a docblock.
Comment #15
rpayanmLooks fine for me.
Comment #16
alexpottDouble line when the standard is one. I think the fact that the message is composed by the mail() method is irrelevant.
This does not make sense it will not call the interface it will call an implementation of it.
Should be on one line.
Should be on one line - also "Prepares". Also this function docblock would be improved with an @see to MailManagerInterface->mail().
Should this not be
MailManagerInterface::mail()
? I'm not 100% sure but that is the replacement for drupal_mail so...Comment #17
JeroenTI made all changes as suggested by alexpott in #16, but I'm not sure if I did 2 correct.
Patch attached.
Comment #18
ianthomas_ukThis is still over the 80 character limit (which is why the previous version was wrapped). I think we can just use "Formats a message prior to sending.", maybe with an @see at the end of the block if it doesn't have one already.
Also still over 80 chars. How about "Alter an email message created with MailManagerInterface->mail()"? The namespace is still referenced at the end of the block.
Over 80 characters. I can't see a way to shorten it, but the 'called from' section could be moved from the summary line into the first paragraph.
This hasn't been changed to MailManagerInterface.
There's something weird with your interdiff because it looks like it's being changed from drupal_mail() to MailInterface, and then back again. The patch itself is fine other than not having the word Manager in it. The same has happened in update.fetch.inc.
Your fix to point 2 looks good.
If you're unsure of coding standards, check https://www.drupal.org/coding-standards/docs. The relevant bit for 3 of the points above is
Comment #19
JeroenT@ianthomas_uk, Thank you for your feedback.
All your feedback seems valid so new patch attached!
This interdiff may also seem a little bit weird, because hook_help moved from system.api.php to help.api.php. The changed lines in system.api.php don't match but the patch still applied.
Comment #20
ianthomas_ukWe're nearly there. I've reviewed the patch rather than this interdiff, found these issues:
invoke hook_mail() can be moved up a line.
This should be MailManagerInterface (PhpMail is an implementation of MailInterface).
MailManagerInterface
This should be formatted to wrap at 80 chars.
Comment #21
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedIncorporated #20 changes.
Comment #22
ianthomas_ukLooks good, review points have been addressed.
Comment #23
alexpottCommitted 2d727c6 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.