I am working in on a project of Drupal 8 beta 14, then I needed implement SMTP project, since I installed it doesn't work out of the box reason why I overwrite it and I am provide a hot-fix to make compatible with my current Drupal 8 version. I am aware right now Drupal is one minor upper than me patch, however I guess it would match with last Drupal 8 version (Beta 15)

CommentFileSizeAuthor
#19 2567303-19.patch1.92 KBPradnya Pingat
#13 smtp-hotfix-2567303-13.patch2.59 KBchishah92
#2 smtp-hotfix_d8_beta14-2567303-1-8.patch15.52 KBkeboca

Comments

keboca created an issue. See original summary.

keboca’s picture

keboca commented
StatusFileSize
new smtp-hotfix_d8_beta14-2567303-1-8.patch15.52 KB
asrob’s picture

asrob
he/him
Primary language Hungarian
Location Hungary 🇭🇺 🇪🇺
commented
Status: Patch (to be ported) » Postponed (maintainer needs more info)

@keboca, can you give us more information about this patch? As I can see there are some fixes due to coding standards.

keboca’s picture

keboca commented

@asrob, I list what I've done in this patch:

  • 1.hook_install()
  • 2.hook_uninstall()
  • 3.hook_menu()
  • 4.SMTPConfigForm.php

1.hook_install()

When module is installed, it saves settings into "smtp.settings" configuration, somehow the follow "smtp_allowhtml" key doesn't work as expected if it is defined as empty string, then I modified to an integer (I setup as zero).
Then I added a new settings "smtp_queue" to be store into "smtp.settings" by default when module is installed.
Also I am saving current system mail interface into settings to "prev_mail_system" key.

2.hook_uninstall()

When module is uninstalled, I restore system mail interface to value that Drupal had before install SMTP module, it means, I retrieve value stored at "prev_mail_system" key in "smtp.settings".

3.hook_menu()

I just removed it since we're using YAML file to discover all routes into Drupal 8.

4.SMTPConfigForm.php

When SMTPConfigForm call "buildForm" method, I expose "smtp_queue" email option into form emails,

$form['email_options']['smtp_queue'] = array(
    '#type' => 'checkbox',
    '#title' => t('Enable SMTP Drupal Queue system'),
    '#default_value' => $config->get('smtp_queue'),
    '#description' => t('Checking this box will allow Drupal Queue system handle send e-mails using Cron execution.'),
);

It is because when "SMTPMailSystem" class invokes "mail" method, it tries to retrieve this value from settings at line 522:
if ($this->smtpConfig->get('smtp_queue')) {
But it wasn't available into "smtp.settings" settings reason why I exposed.

Then when "submitForm" method is called, I load current system mail interface setting into a temp variable ("$mail_interface") and I setup "SMTPMailSystem" as default interface in order to send test email using this interface.

After that I check that current SMTP option is already enabled and SMTP interface is different than system mail interface saved into $mail_interface, if so I save "SMTPMailSystem" as default interface, otherwise I will restore system mail interface.

Basically it is what I am doing there, also a bunch of code standards from Drupal, BTW I haven't check current module version, then I don't know if it had change already, since I uploaded this patch around 9 months ago.

I hope, it helps and you could apply it to this module,
Have a ball,

pazhyn’s picture

pazhyn commented
Status: Postponed (maintainer needs more info) » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: smtp-hotfix_d8_beta14-2567303-1-8.patch, failed testing.

The last submitted patch, 2: smtp-hotfix_d8_beta14-2567303-1-8.patch, failed testing.

estoyausente’s picture

estoyausente
Primary language Spanish
commented
Issue tags: +Needs reroll

Anyway, this patch need a reroll, I can't apply it.

chishah92’s picture

chishah92 commented
Assigned: Unassigned » chishah92
chishah92’s picture

chishah92 commented

I tried to reroll the patch , but i am not even able to apply the patch as it says the no such file or directory inspite of creating a contrib folder inside modules folder for the patch to get applied.

Please Advice

estoyausente’s picture

estoyausente
Primary language Spanish
commented

@chishah92 have you try folowwing this guide?

https://www.drupal.org/patch/reroll

I don't think that it is a easy reroll. Some help:

  1. +++ b/modules/contrib/smtp/smtp.install
@@ -9,6 +9,10 @@
+  // Retrieve current system mail interface.
+  $factory = \Drupal::configFactory();
+  $mail_interface = $factory->getEditable('system.mail')
+    ->get('interface');

    This already did in https://www.drupal.org/node/2698513 

  2. +++ b/modules/contrib/smtp/smtp.module
@@ -26,18 +26,6 @@ function smtp_help($path, $arg) {
- * Implements hook_menu().
- */
-function smtp_menu() {
-  $items['admin/config/system/smtp'] = array(
-    'title' => 'SMTP Authentication Support',
-    'description' => 'Allow for site emails to be sent through an SMTP server of your choice.',
-    'route_name' => 'smtp.config',
-  );
-  return $items;
-}

    It's ok and necessary. 

  3. +++ b/modules/contrib/smtp/src/Form/SMTPConfigForm.php
@@ -37,7 +37,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#type'  => 'fieldset',
+      '#type' => 'fieldset',

    This code standar refactor is nice, but... it is the correct issue?

I think that we can divide this issue in different issues (remove hook_menu, coding standards, set default mail system at uninstall...) and close it after that. It's a little bit chaotic issue.

chishah92’s picture

chishah92 commented

Yes i had referred that guide , but due to the issue which i am facing i am not even able to start fixing things.

Thanks!
~chirag

chishah92’s picture

chishah92 commented
Status: Needs work » Needs review
StatusFileSize
new smtp-hotfix-2567303-13.patch2.59 KB

@estoyausente , adding the new patch where i have done following things:

  1. Removing hook_menu
  2. Applied the patch from https://www.drupal.org/node/2698513 to set default mail system at uninstall
  3. Coding standard fixes

I hope this is at least a good start.

Thanks!
~Chirag

estoyausente’s picture

estoyausente
Primary language Spanish
commented

Yeah it's a nice start.

  • The patch now apply correctly.
  • The module is installed correctly.
  • The module sends emails correctly
  • The module is uninstalled correctly
  • System send mail after uninstal correctly using default mail system.

But I think that is better put in different commits the hook_menu removal and the uninstall actions because the are different issues and problems and can be debugged and tested better separatelly.

Anyway, its look nice!

Status: Needs review » Needs work

The last submitted patch, 13: smtp-hotfix-2567303-13.patch, failed testing.

The last submitted patch, 13: smtp-hotfix-2567303-13.patch, failed testing.

ashishdalvi’s picture

ashishdalvi
Primary language English
Location Mumbai
commented
Issue tags: +DrupalMumbaiCodeSprint15

Unable to apply patch. Patch needs to be rerolled.

We will work on this issue in DrupalMumbai Code sprint #15.

Pradnya Pingat’s picture

Pradnya Pingat commented
Assigned: chishah92 » Pradnya Pingat

I am working on it

Pradnya Pingat’s picture

Pradnya Pingat commented
Assigned: Pradnya Pingat » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2567303-19.patch1.92 KB

Rerolled patch with 8.x-1.x

wundo’s picture

wundo commented
Priority: Major » Normal
Status: Needs review » Needs work

@Pradnya Pingat, looks like your patch is including a few code style changes not directly related with the issue itself, could you please submit a patch with only the necessary changes and submit the code styling changes as their own issue?

wundo’s picture

wundo commented
pazhyn’s picture

pazhyn commented

Can we suggest changing this issue subject and description?
I think it looks confusing now and differs from the discussion and provided patches.
@Pradnya Pingat, can you specify at least correct subject for this issue, please?

Pradnya Pingat’s picture

Pradnya Pingat commented

@wundo I think thats because my IDE is not configured according to drupal standards. I fixed that now .Thanks for help

wundo’s picture

wundo commented
Status: Needs work » Closed (works as designed)