Sending mail without using getServices() in MODX 3

Development
1

Since getServices() is deprecated, I spent some time trying to send mail without it.

The code below works, and with real emails, the email arrives, but there’s a strange quirk:

use MODX\Revolution\Mail\modPHPMailer;

$sender = 'somebody@somewhere.com';
$recipient = 'somebodyElse@somewhere.com';
if (!$modx->services->has('mail')) {
    $modx->services->add('mail', new modPHPMailer($modx));
}
$mail = $modx->services->get('mail');
$mail->set(modMail::MAIL_BODY, 'Testing Mail Service again');
$mail->set(modMail::MAIL_FROM, $sender);
$mail->set(modMail::MAIL_FROM_NAME, 'Bob');
$mail->set(modMail::MAIL_SENDER, $sender);
$mail->set(modMail::MAIL_SUBJECT, 'TESTING');
$mail->address('to', $recipient);
$mail->address('reply-to', $sender);
$mail->setHTML(true);
if (!$sent = $mail->send()) {
    $err = 'Error: ' . $mail->mailer->ErrorInfo;
    $this->modx->log(xPDO::LOG_LEVEL_ERROR, $err);
    $mail->reset();
    return 'Failure';
} else {
    $mail->reset();
    return "Mail sent";
}

The quirk is that the mail class in the Pimple container is:
PHPMailer\PHPMailer\PHPMailer#22 (the number varies). I tried stepping through the code in XDebug, but couldn’t find where this happens.

This doesn’t happen with any of the other services. So is the code correct, why does this happen, and is it an issue?

As a side note, as far as I can tell, the mail service is never initialized anywhere in MODX 3. All the places that send mail (e.g., modUser) are using getService(), and all the MODX 3 docs I could find advise using getService().

I also found that calling $mail->reset() threw a 500 error on MODX cloud. Commenting it out made the code work, and uncommenting it produced the 500 error (did this cycle several times). That seems to have gone away after manually clearing the site cache, so maybe it’s not an issue. It’s pretty mysterious, though, since reset() is quite innocuous. It just empties the mail fields.

2

Why not initialize $mail as $mail = new modPHPMailer($modx) and ignore the services part?

3

I was reviewing my implementation and yours, and the only major difference is instead of:

$mail = $modx->services->get('mail');

I just call

$modx->services->get('mail');

then in the rest I call

$modx->mail->set(...
4

You should do it the way @bobray is doing it. :sweat_smile:

2 Likes
5

I’m not touching code that works :stuck_out_tongue:

6

You will have to when it breaks. :sweat_smile:

7

Wouldn’t it be better to name the service “mailers instead of “mail”? Furthermore I agree with @opengeek

8

Would be nice to get the mailer into the container in 3.1 so this becomes a non-issue.

9

mail is the legacy service name.

10

We need a bootstrap that configures the container by default and can be customized by the user.

1 Like
11

Sounds like a very good idea

12

Are you thinking of a System Setting with a comma-separated list of services to load? That seems like it would be easy to implement.

We should re-factor the modUser class sendEmail() method to use it, but maybe that should wait until the bootstrap part is done.

Any clue why the classname in the container is PHPMailer\PHPMailer\PHPMailer#22?

13

No. Not a system setting. This needs to be a PHP file that is loaded after the context is initialized and configures all the services in the container, based on any relevant system/context settings.

Yes. After this bootstrap is implemented, you can just get the service from the container when needed and not worry about checking to make sure it exists/configuring the container if it doesn’t.

I don’t think it is the class name in the container. I think that is just a representation of the instance that is stored in the container.

14

I not clear on why it needs to be a file. Why not put the code at the end of $modx->initialize()?

It sounds like you’re thinking of a separate setting for each service, wouldn’t a single setting be more convenient for changing the list of services to be loaded?

15

Settings can be used by any of the services and in some cases were used to define what class you might want to have configured for a particular service in MODX 2.x. I do not want to get into another situation like extension_packages (which are now loaded in MODX 3.x via bootstrap.php files for each component “namespace”) where we have them loaded automatically from settings. Settings should contain options that can be used to configure specific options in various core services, but the services themselves should be configured in the container via PHP code. This is pretty typical with any dependency injection container. Doing it any other way will limit what can be configured in the container, as well as how it can be configured for each service.

16

I thought extension_packages might come up. :wink:

That makes sense.

BTW, about the triple class name. It’s a quirk in modPhpMailer, not Pimple:

$mail = $modx->services->get('mail');
echo "Class of \$mail: " . get_class($mail);
echo "\nClass of \$mail->mailer: " . get_class($mail->mailer);

Output:

Class of $mail: MODX\Revolution\Mail\modPHPMailer
Class of $mail->mailer: PHPMailer\PHPMailer\PHPMailer

It’s still weird, imo.

1 Like
17

Sorry, I misunderstood what you meant regarding the class name. I thought you were referring to the number on the end.

18

I should have made that clearer. I was wrong anyway, just not for that reason. :wink:

19

@opengeek do you have any idea on the implementation of this bootstrap file? We could do it in a similar approach as we do for the config file.

E.g; generate the default bootstrap.php during setup and place it in core/config. Maybe we should also consider having a MODX_BOOTSTRAP_KEY so we can use load different bootstrap files (like how it works for the config file).

20

I thought it would work using the core namespace, as that code already looks for a bootstrap.php file in the core namespace path. However, I just followed some code and realize now that the core namespace path is hardcoded as manager_path in the generateNamespaceCache method in order to use it to locate the core controllers. I’m going to need some time to ponder the best way to clean up this mess.

But yes, I was wanting to have a default bootstrap.php file generated into the appropriate location. We will need to just resolve this improper use of the core namespace path to figure out the best way to do that. I don’t think there is a need for a MODX_BOOTSTRAP_KEY, however.