while i wasn't successfully able to get the solution in #1855080: is possible to "switch off" MigrateMailIgnore?? working, i'm not sure that re-enabling the mail system for an entire migration instance is needed.

in my use case, i needed mail enabled solely within a single instance's postImport() method. it would be good to have a bit more flexibility to the timing of re-enabling (and re-disabling) mail system.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bdone’s picture

Status: Active » Needs review
FileSize
2.13 KB

this patch saves the mail system, prior to the MigrationBase constructor disabling it. It also adds two public methods to toggle the original mail system.

usage example...

protected function postImport() {

  parent::postImport();

  // Restore system emails for custom migrate notifications.
  $this->restoreMailSystem();

  // custom drupal_mail() code here...

  // Restore system emails for custom migrate notifications.
  $this->disableMailSystem();

}
bdone’s picture

Issue summary: View changes

Updated issue summary.

mikeryan’s picture

Issue summary: View changes
Status: Needs review » Needs work
  1. +++ b/includes/base.inc
    @@ -1340,6 +1335,46 @@ abstract class MigrationBase {
    +      $this->mailSystem = $conf['mail_system'];
    

    Please explicitly declare mailSystem in the class.

  2. +++ b/includes/base.inc
    @@ -1340,6 +1335,46 @@ abstract class MigrationBase {
    +      if ($conf['mail_system']['default-system'] == 'MigrateMailIgnore') {
    +        unset($conf['mail_system']);
    

    Consider if we originally had two keys in $conf['mail_system'], default-system and secondary-system. It looks like this will result in removing mail_system entirely, rather than restoring the original values.

mikeryan’s picture

Also, see https://drupal.org/comment/8592031#comment-8592031 - I think this feature should make the mail_system saving in the drush import/rollback commands obsolete, that should be removed in this patch.

Thanks.

bdone’s picture

Status: Needs work » Needs review
FileSize
4.17 KB

with fixes #2, and updates to drush commands from #3, here's an updated patch for review.

mikeryan’s picture

Status: Needs review » Needs work
+++ b/includes/base.inc
@@ -1357,6 +1357,46 @@ abstract class MigrationBase {
+    if (!empty($conf['mail_system'])) {

The if statement here doesn't actually do anything ($system is local to the loop, unsetting it has no effect on anything else). It appears you mean to unset $conf['mail_system'][$system], but even that won't have an effect when all of $conf['mail_system'] gets overwritten in the last line. For that matter, it seems $class rather than $system should be used in the comparison.

At any rate, it seems like just the assignment to $conf['mail_system'] should be sufficient to restore it, unless you're concerned there would have been changes to mail_system during migration that you would want to preserve?

Other than the above, the rest of the patch looks good, thanks.

bdone’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

thanks for the feedback @mikeryan. i hadn't considered mail_system changing during the migration and didn't make any changes along those lines. here's a fix dropping the unneeded unset().

  • mikeryan committed 2a8bd55 on 7.x-2.x authored by bdone
    Issue #2095841 by bdone: Abstract mail system disablement for more...
mikeryan’s picture

Status: Needs review » Fixed
Issue tags: +Migrate 2.8

Committed, thanks!

Status: Fixed » Closed (fixed)

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