- Issue created by @dpi
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 1:46pm 18 December 2023 - 🇦🇺Australia dpi Perth, Australia
If you want to test the functionality as demoed in the gif, you'll need:
- https://www.drupal.org/project/sm →
- https://www.drupal.org/project/sm_transport_doctrine →
- The patches, as mentioned on the SM project-page/readme
- Status changed to Needs work
12 months ago 4:23pm 20 December 2023 - 🇺🇸United States zengenuity
I tested this today, and there are two issues that I see.
First, these changes make the module incompatible with D9. If we're going to do that, we should create a new major version. I'm okay with that. We can have a 2.0.x branch that is D10 only. But I don't want to merge this into 1.0.x and break D9 compatibility for sites that might still be on D9. Is there a specific thing you require that is D10 only? Please let me know. If so, I will create a new branch, and we can move this issue to that branch.
Second, in testing, I found that embedded images do not work when messenger is engaged. They work fine with the patch when messenger is not active. But when you configure the site to use the doctrine messenger queue, the CIDs are wrong in the emails. The images are still attached, but they don't display inline because of the CID mismatch. You can test this by sending yourself an email with HTML in it like this:
<img src="image:/sites/default/files/some-existing-image-file.png" />Can you see if this is something that can be fixed? I'm not sure if it needs to be fixed in this module or in your other modules.
- 🇦🇺Australia dpi Perth, Australia
First, these changes make the module incompatible with D9.
How so?
Since the minimum supported version of Drupal core is 10.1, we could just as well create a new minor. I dont think a new major is necessary.
On images:
I tried two methods of testing, and I believe its SML is at fault.
SML Test Mail
I used
admin/config/system/symfony-mailer-lite/testto test. I modifiedsymfony_mailer_lite_mailby appending:$text[] = '<img src="image:/sites/default/files/media-icons/generic/audio.png" />';Out of the box I found when I dispatched the mail, found Mailhog refused to render the image. After debugging I found the name seemed to be mismatched. Altering
\Drupal\symfony_mailer_lite\Plugin\Mail\SymfonyMailer::embedImagesby modifying the second to last line to$html = preg_replace('/cid:' . $image['cid'] . '/', 'cid:' . $image['filename'], $html);resolved the issue. I'm not saying this is a long term solution, but it did resolve sending mails.Before
After
Manual dispatch with Mailer
I also tried sending through Mailer directly, first creating a service like so:
mailer.mailer: class: Symfony\Component\Mailer\Mailer arguments: - '@mailer.transports' - '@?messenger.default_bus' - '@event_dispatcher' public: false Symfony\Component\Mailer\MailerInterface: '@mailer.mailer'Then dispatching with:
$cid = 'blah'; $email = (new \Symfony\Component\Mime\Email()) ->to('foo@bar') ->from('john@cardholder.com') ->subject('Hello world!') ->text('Some plain text') ->addPart((new DataPart(fopen('/data/app/sites/default/files/media-icons/generic/audio.png', 'r'), $cid, 'image/png'))->asInline()) ->html(<<<HTML <p>Some <strong>rich</strong> text</p> <img src="cid:$cid"> HTML); /** @var \Symfony\Component\Mailer\MailerInterface $mailer */ $mailer = \Drupal::service(MailerInterface::class); $mailer->send($email);This method works without issue. So I think the problem is purely with SML
- 🇦🇺Australia dpi Perth, Australia
Still looking into it but your CID issue is the right direction to investigate
- 🇦🇺Australia dpi Perth, Australia
The
preg_replacesuggestion above seems to work with and without messenger. So I'm thinking we should just do that. I'll push that change to the MR for review.I'll also add the Mailer service as I created above, and switch
\Drupal\symfony_mailer_lite\Plugin\Mail\SymfonyMailerto use it. Its entirely up to you if you want to use it however. - Status changed to Needs review
12 months ago 5:23am 21 December 2023 - 🇺🇸United States zengenuity
Your change to the $cid variable breaks an edge case, so I reverted that commit. If you have two images embedded with the same file name from different directories, the same image is displayed twice instead of the two different images. This doesn't happen with the original $cid variable.
I also tested again to confirm that embedding images does not work when messenger is enabled, and I found that to be true. But, I'm not really that worried about it, as long as it doesn't break the use of this module without messenger. So, with my reversion of the $cid change, I think we can go ahead with committing this.
I figured out the required changes to make your additions run on Drupal 9. I didn't test messenger on D9. My goal was only to make sure that we don't break the non-messenger use of this module on D9, and with my changes, that seems to be the case.
Before I commit this, can you check that my changes have not broken things for your intended use case with messenger? I did run it with messenger as shown in your GIF, and it seems to be fine (other than the embedded images issue), but you know more about it and might notice something I missed.
Once I hear back from you that the changes are working, I will commit this.
- 🇦🇺Australia dpi Perth, Australia
Thanks for taking a look.
I think we can keep the service aliases → , even though they may not work with older D9. I pushed that revert, though I wont argue if you dont want it.
The doc changes for the class props are no longer FQN's/documented, not sure if thats intentional
-
zengenuity →
committed f1066b11 on 1.0.x authored by
dpi →
Issue #3409475: Add Symfony Messenger support for async messages (emails...
-
zengenuity →
committed f1066b11 on 1.0.x authored by
dpi →
- Status changed to Fixed
12 months ago 2:45pm 5 January 2024 - 🇺🇸United States zengenuity
Looks good. I tested the service alias change on a recent D9, and it worked, so I think that is fine. I merged the MR. Thanks for this!
Automatically closed - issue fixed for 2 weeks with no activity.
