Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT’s picture

Status: Active » Needs review
FileSize
15.46 KB

Removed usages of the drupal_mail() function. Patch attached.

javivf’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied and the function only exist in core/includes/mail.inc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,8 @@
    +   * Formats a message composed by
    +   * \Drupal::service('plugin.manager.mail')->mail() prior sending.
    
    @@ -36,7 +37,8 @@
    +   * Sends a message composed by
    +   * \Drupal::service('plugin.manager.mail')->mail().
    
    +++ b/core/modules/system/system.api.php
    @@ -757,23 +757,27 @@ function hook_form_BASE_FORM_ID_alter(&$form, \Drupal\Core\Form\FormStateInterfa
    - * Alter an email message created with the drupal_mail() function.
    + * Alter an email message created with the
    + * \Drupal::service('plugin.manager.mail')->mail function.
    
    @@ -1144,33 +1149,40 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
    + * Prepare a message based on parameters; called from
    + * \Drupal::service('plugin.manager.mail')->mail().
    

    The code standards says that this should be a one liner.

  2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
    @@ -51,7 +51,7 @@ public function format(array $message) {
    +   * @see \Drupal::service('plugin.manager.mail')->mail()
    

    Should be an @see to the method on the interface. This needs to be fixed elsewhere in the patch.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
15.41 KB
3.58 KB

Addressed the changes as suggested by alexpott in #3.

rpayanm’s picture

Status: Needs review » Reviewed & tested by the community
Alienpruts’s picture

Status: Reviewed & tested by the community » Needs review

@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 ?

aspilicious’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,7 @@
    +   * Formats a message composed by \Drupal::service('plugin.manager.mail')->mail() prior sending.
    

    This exceeds 80 chars, hmm not sure how we can fix this.

  2. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -36,7 +36,7 @@
    +   * Sends a message composed by \Drupal::service('plugin.manager.mail')->mail().
    

    Same story exceeds 80 chars.

  3. +++ b/core/modules/system/system.api.php
    @@ -757,23 +757,26 @@ function hook_form_BASE_FORM_ID_alter(&$form, \Drupal\Core\Form\FormStateInterfa
    + * Alter an email message created with the \Drupal::service('plugin.manager.mail')->mail function.
    

    And again

  4. +++ b/core/modules/system/system.api.php
    @@ -1144,33 +1148,39 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
    + * Prepare a message based on parameters; called from \Drupal::service('plugin.manager.mail')->mail().
    

    One more

It's tricky because the new syntax takes way more space in the docs.

jamesdixon’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
12.42 KB

Lets bust the long pieces of code down to the next line. May not be the prettiest but is very readable IMHO.

Ashok Negi’s picture

Status: Needs review » Needs work
FileSize
98.26 KB

Changing status to Needs Work. Please remove drupal_mail from comment sections. See attached screenshot for reference.

jamesdixon’s picture

@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?

JeroenT’s picture

Status: Needs work » Needs review

there is a seperate issue for removing the function drupal_mail itself: #2359457: Remove drupal_mail().

EclipseGc’s picture

  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,8 @@
    +   * \Drupal::service('plugin.manager.mail')->mail() prior sending.
    
    @@ -36,7 +37,8 @@
    +   * \Drupal::service('plugin.manager.mail')->mail().
    
    +++ b/core/lib/Drupal/Core/Mail/MailManager.php
    @@ -117,8 +117,8 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
    +   *     \Drupal::service('plugin.manager.mail')->mail() to invoke hook_mail().
    

    You should be using the actual class interface name here.

  2. +++ b/core/modules/system/system.api.php
    @@ -297,23 +297,27 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + * \Drupal::service('plugin.manager.mail')->mail function.
    

    Here too

  3. +++ b/core/modules/system/system.api.php
    @@ -297,23 +297,27 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + * with \Drupal::service('plugin.manager.mail')->mail(). Usage examples include
    

    And here

  4. +++ b/core/modules/system/system.api.php
    @@ -297,23 +297,27 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + * \Drupal::service('plugin.manager.mail')->mail() will not invoke
    ...
    + * \Drupal::service('plugin.manager.mail')->mail() for messaging, it is best
    ...
    + *     The \Drupal::service('plugin.manager.mail')->mail() id of the message.
    ...
    + *     \Drupal::service('plugin.manager.mail')->mail() for possible id values.
    
    @@ -331,15 +335,16 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + *     \Drupal::service('plugin.manager.mail')->mail() that is used to build
    
    @@ -419,33 +424,40 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
    + * \Drupal::service('plugin.manager.mail')->mail().
    ...
    + * \Drupal::service('plugin.manager.mail')->mail(), not all modules.
    ...
    + *     \Drupal::service('plugin.manager.mail')->mail() for possible id values.
    ...
    + *     \Drupal::service('plugin.manager.mail')->mail() sets this to an empty
    ...
    + *     \Drupal::service('plugin.manager.mail')->mail() sets this to an
    ...
    + *     set by \Drupal::service('plugin.manager.mail')->mail() to either a
    ...
    + *     \Drupal::service('plugin.manager.mail')->mail() pre-fills several
    ...
    + *   \Drupal::service('plugin.manager.mail')->mail().
    

    Suffice it to say, there are a few more.

Code seems fine, just the docs need some work.

rpayanm’s picture

ianthomas_uk’s picture

FileSize
15.8 KB
17.16 KB
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,8 @@
    +   * \Drupal\Component\Plugin\PluginManagerInterface->mail() prior sending.
    

    The 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.

rpayanm’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine for me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,8 @@
    +   * Formats a message composed by
    +   * \Drupal\Core\Mail\MailManagerInterface->mail() prior to sending.
    

    Double line when the standard is one. I think the fact that the message is composed by the mail() method is irrelevant.

  2. +++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
    @@ -108,11 +109,10 @@
    +   *   If TRUE, call \Drupal\Core\Mail\MailInterface->mail() to deliver the
    

    This does not make sense it will not call the interface it will call an implementation of it.

  3. +++ b/core/modules/system/system.api.php
    @@ -297,23 +297,24 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + * Alter an email message created with the
    + * \Drupal\Core\Mail\MailManagerInterface->mail() function.
    

    Should be on one line.

  4. +++ b/core/modules/system/system.api.php
    @@ -419,33 +421,36 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
    + * \Drupal\Core\Mail\MailManagerInterface->mail().
    

    Should be on one line - also "Prepares". Also this function docblock would be improved with an @see to MailManagerInterface->mail().

  5. +++ b/core/modules/update/update.module
    @@ -442,7 +442,7 @@ function update_fetch_data_finished($success, $results) {
    - * @see drupal_mail()
    + * @see \Drupal\Core\Mail\MailInterface::mail()
    
    +++ b/core/modules/user/user.module
    @@ -1267,7 +1267,7 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
    - * @see drupal_mail()
    + * @see \Drupal\Core\Mail\MailInterface::mail()
    

    Should this not be MailManagerInterface::mail()? I'm not 100% sure but that is the replacement for drupal_mail so...

JeroenT’s picture

Status: Needs work » Needs review
FileSize
17.05 KB
4.93 KB

I made all changes as suggested by alexpott in #16, but I'm not sure if I did 2 correct.

Patch attached.

ianthomas_uk’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -17,7 +17,7 @@
    -   * Formats a message composed by drupal_mail() prior sending.
    +   * Formats a message composed by \Drupal\Core\Mail\MailManagerInterface prior to sending.
    

    This 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.

  2. +++ b/core/modules/system/system.api.php
    @@ -297,23 +297,23 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    - * Alter an email message created with the drupal_mail() function.
    

    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.

  3. +++ b/core/modules/system/system.api.php
    @@ -419,33 +420,37 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
    - * Prepare a message based on parameters; called from drupal_mail().
    + * Prepares a message based on parameters; called from \Drupal\Core\Mail\MailManagerInterface->mail().
    

    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.

  4. +++ b/core/modules/user/user.module
    @@ -1257,7 +1257,7 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
    - * @see drupal_mail()
    + * @see \Drupal\Core\Mail\MailInterface::mail()
    

    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

"All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc."
JeroenT’s picture

Status: Needs work » Needs review
FileSize
17.09 KB
6.85 KB

@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.

ianthomas_uk’s picture

Status: Needs review » Needs work

We're nearly there. I've reviewed the patch rather than this interdiff, found these issues:

  1. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
    @@ -117,7 +117,8 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
    +   *     \Drupal\Core\Mail\MailManagerInterface->mail() to
        *     invoke hook_mail().
    

    invoke hook_mail() can be moved up a line.

  2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
    @@ -53,7 +53,7 @@ public function format(array $message) {
    -   * @see drupal_mail()
    +   * @see \Drupal\Core\Mail\MailInterface::mail()
    

    This should be MailManagerInterface (PhpMail is an implementation of MailInterface).

  3. +++ b/core/modules/system/system.api.php
    @@ -331,15 +331,16 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    - * @see drupal_mail()
    + * @see \Drupal\Core\Mail\MailInterface::mail()
    

    MailManagerInterface

  4. +++ b/core/modules/system/system.api.php
    @@ -376,33 +377,38 @@ function hook_system_breadcrumb_alter(array &$breadcrumb, \Drupal\Core\Routing\R
    + * This hook is called from MailManagerInterface->mail().
      * Note that hook_mail(), unlike hook_mail_alter(), is only called on the
    - * $module argument to drupal_mail(), not all modules.
    + * $module argument to MailManagerInterface->mail(), not all modules.
    

    This should be formatted to wrap at 80 chars.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
17.21 KB
2.58 KB

Incorporated #20 changes.

ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, review points have been addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2d727c6 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 2d727c6 on 8.0.x
    Issue #2358991 by JeroenT, ianthomas_uk, rpayanm, jamesdixon, er....

Status: Fixed » Closed (fixed)

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