Problem/Motivation

drupal-check results on commit hash: f479d32c

./vendor/bin/drupal-check modules/contrib/smtp/
 9/9 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------- 
  Line   smtp.install                                       
 ------ --------------------------------------------------- 
  32     Call to deprecated function drupal_set_message().  
  33     Call to deprecated function drupal_set_message().  
 ------ --------------------------------------------------- 


 [ERROR] Found 2 errors                                                                                                 

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ChaseOnTheWeb created an issue. See original summary.

joy29’s picture

Assigned: Unassigned » joy29
Issue tags: -midcamp2019 +SrijanSprintDay
joy29’s picture

Assigned: joy29 » Unassigned
Status: Active » Needs review
FileSize
1.73 KB

Status: Needs review » Needs work

The last submitted patch, 3: smtp-3042630-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Waldoswndrwrld’s picture

joy29’s picture

@Waldoswndrwrld Thanks

joy29’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: smtp-3042630-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jigish.addweb’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

@ChaseOnTheWeb, Kindly review the attached patch & let me know your views for the same.

Thanks!..

Status: Needs review » Needs work

The last submitted patch, 9: smtp-3042630-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joy29’s picture

John Cook’s picture

The strtolower() errors have already been fixed as part of #3041687: Deprecated Unicode::* methods need to be replaced., so the the current patches are now obsolete.

But there are now more errors reported by drupal-check:

./vendor/bin/drupal-check modules/contrib/smtp/
 9/9 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ --------------------------------------------------- 
  Line   smtp.install                                       
 ------ --------------------------------------------------- 
  32     Call to deprecated function drupal_set_message().  
  33     Call to deprecated function drupal_set_message().  
 ------ --------------------------------------------------- 


 [ERROR] Found 2 errors                                                                                                 

I've updated the summary, hidden the existing patches, and set the status back to needs work.

Ankush_03’s picture

Status: Needs work » Needs review
FileSize
834 bytes

Adding patch please review !

Diego_Mow’s picture

Status: Needs review » Reviewed & tested by the community

Looks Good

swatichouhan012’s picture

Assigned: joy29 » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
1.93 KB
1.03 KB

@Diego_Mow Sorry to move this again in NW, i found one more deprecated code ,

------ -------------------------------------------------------------------------------------------------------------------
Line src/Plugin/Mail/SMTPMailSystem.php
------ -------------------------------------------------------------------------------------------------------------------
488 Call to deprecated constant FILE_EXISTS_REPLACE: Deprecated in drupal:8.7.0 and is removed from drupal:9.0.0. Use
\Drupal\Core\File\FileSystemInterface::EXISTS_REPLACE.

i have created a patch to fix this kindly review,

Diego_Mow’s picture

Status: Needs review » Reviewed & tested by the community

Good catch swatichouhan012.

I checked the patch and continue to look ok for me.

Diego_Mow’s picture

rivimey’s picture

rivimey’s picture

wundo’s picture

Status: Reviewed & tested by the community » Needs work

Patch needs re-roll

Diego_Mow’s picture

Status: Needs work » Needs review
FileSize
9.98 KB

Generated new patch considering code from patches #13 and #15 and also some new CR items to guarantee everything.

Status: Needs review » Needs work

The last submitted patch, 21: drupal-9-deprecated-code-report-3042630-21.patch, failed testing. View results

rivimey’s picture

@Diego

In changing smtp_install() wouldn't it be better to cache the return value of \Drupal::messenger() and call addMessage from that?

If you're going to change Smtp_send_queue queuer please lets have a meaningful docblock, especially which indicates why the function exists. For example: "Add the outgoing mail to the SMTP Mail Queue."

Ditto for _smtp_mailer_send()

Question, not bug: for D9 compatibility, should file_save_data be rendered using a Drupal service instead?

rivimey’s picture

Fixed the cause of the test error: the tester had been changed to expect smtpConnect() but the string is still SmtpConnect() and that is correct.

Also fixed the point about smtp_install() I mentioned above.

Also fixed the text of that message, which was still stuck in D7 menu terminology.

Diego_Mow’s picture

Status: Needs work » Reviewed & tested by the community

Tested and worked fine here.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

The .info.yml file is missing the new core_version_key, see https://www.drupal.org/node/3070687.

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
9.32 KB

Added the core version key in the info.yml file. Here is the updated patch.

Berdir’s picture

+++ b/src/Plugin/Mail/SMTPMailSystem.php
@@ -485,7 +486,7 @@ class SMTPMailSystem implements MailInterface, ContainerFactoryPluginInterface {
               }
 
               $attachment_new_filename = \Drupal::service('file_system')->tempnam('temporary://', 'smtp');
-              $file_path = file_save_data($attachment, $attachment_new_filename, FILE_EXISTS_REPLACE);
+              $file_path = file_save_data($attachment, $attachment_new_filename, FileSystemInterface::EXISTS_REPLACE);
               $real_path = \Drupal::service('file_system')->realpath($file_path->uri);
 

I think this constant requires 8.7, so we should use ^8.7.7 || ^9 and remove the core: 8.x key.

Suresh Prabhu Parkala’s picture

Updated patch please review!

rivimey’s picture

Status: Needs review » Reviewed & tested by the community

  • japerry committed 3997daf on 8.x-1.x
    Issue #3042630 by japerry, joy29, Suresh Prabhu Parkala, swatichouhan012...
japerry’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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