Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pazhyn created an issue. See original summary.

jorgik’s picture

Assigned: Unassigned » jorgik
jorgik’s picture

jorgik’s picture

Status: Needs work » Needs review
dpovshed’s picture

Status: Needs review » Needs work

@jorgik, there are 2 minor issues with your patch:

1) this line
\Drupal::logger('smtp')->error( 'There is no submitted from address.');
contains an extra space

2) In this line
drupal_set_message(t('The submitted from address @from is not valid.', array('@from' => $from)), 'error');
I believe no need to remove parenthesis around @from. Or at least we should change it to %from to emphasise core element of the message.

pazhyn’s picture

Issue tags: +DrupalCamp Kyiv, +CodeSprintUA
pazhyn’s picture

jorgik’s picture

dpovshed’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, #9 looks nice!

estoyausente’s picture

It looks nice but maybe we can inject the logger in place of the procedural way. I think that mail system are very relevant in all sites and it's useful make it configurable in the class constructor.

Review this link https://www.drupal.org/node/2595985 for checking it.

(But anyway, the patch apply correctly and is correct).

jorgik’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.04 KB

OK, I changed patch, added logger in constructor, needs review

estoyausente’s picture

Status: Needs review » Needs work
+++ b/src/Plugin/Mail/SMTPMailSystem.php
@@ -10,27 +10,45 @@ namespace Drupal\smtp\Plugin\Mail;
-use Drupal\smtp\PHPMailer\PHPMailer;
...
+use PHPMailer;

@@ -74,7 +92,7 @@ class SMTPMailSystem implements MailInterface {
-    // Create a new PHPMailer object - autoloaded from registry.
+    // Create a new PHPMailer object - autoloaded from composer.

I think that you has mixed two different issues in the patch ;)

(The other part seems nice, please fix it and I will review after it)

jorgik’s picture

Status: Needs review » Needs work

The last submitted patch, 15: deprecated_watchdog_function-2742773-15.patch, failed testing.

The last submitted patch, 15: deprecated_watchdog_function-2742773-15.patch, failed testing.

pazhyn’s picture

Provided patch #15 is not working for me.
Errors in log:

...

class_exists() ... /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php:96, referer: ... /admin/config/system/smtp
spl_autoload_call() ... /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php:96, referer: ... /admin/config/system/smtp
Composer\\Autoload\\ClassLoader->loadClass() ... /core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php:0, referer: .. /web/admin/config/system/smtp
Composer\\Autoload\\includeFile() ... /vendor/composer/ClassLoader.php:301, referer: .. /admin/config/system/smtp

Needs work.

jorgik’s picture

estoyausente’s picture

Status: Needs review » Needs work
+++ b/src/Plugin/Mail/SMTPMailSystem.php
@@ -90,10 +104,10 @@ class SMTPMailSystem implements MailInterface {

-        $from = $this->smtpConfig->get('smtp_from');
-        $headers['Sender'] = $from;
-        $headers['Return-Path'] = $from;
-        $headers['Reply-To'] = $from;

+      $from = $this->smtpConfig->get('smtp_from');
+      $headers['Sender'] = $from;
+      $headers['Return-Path'] = $from;
+      $headers['Reply-To'] = $from;

@@ -159,25 +173,25 @@ class SMTPMailSystem implements MailInterface {
-            break;
+              break;
...
-            break;
+              break;
...
-            break;
+              break;

This isn't related with the current issue, are only indentation changes. Is good idea resolve one task in each issue. If you want to refactor the code to use the coding standard, please open another issue.

estoyausente’s picture

Status: Needs review » Needs work

I tried to send an email, and watchdog throw me this message:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "smtp.logger.channel". en Drupal\Component\DependencyInjection\Container->get() (linea 157 de /home/rrqun/www/core/lib/Drupal/Component/DependencyInjection/Container.php).

Can you test it again, please?

jorgik’s picture

estoyausente’s picture

Status: Needs review » Reviewed & tested by the community

Patch work perfect, messages save in watchdog correctly. I think that all is ok.

But please, in order to ease the review, make an interdiff in this kind of patchs with little changes between versions.
https://www.drupal.org/documentation/git/interdiff

wundo’s picture

  • wundo committed 43cde40 on 8.x-1.x authored by jorgik
    Issue #2742773 by jorgik, estoyausente, pazhyn, dpovshed: Deprecated...
wundo’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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