Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for laravel 9 #103

Merged
merged 10 commits into from
Dec 20, 2022
Merged

Conversation

samtlewis
Copy link
Contributor

I saw that the composer.json was updated in master to support Laravel 9 but there are a couple of other changes necessary to support L9 since the Swift Mailer was replaced with the Symfony Mailer.

The Log driver and InboundEmail files needed updates.
I implemented to be backward compatible by checking the app()->version() in each instance.
I also added a run-tests action to test against L6-9 on php 7.3, 8 and 8.1
I confirmed that all tests failed prior to my changes and they all pass on all supported php/laravel combinations.

@cmanish049
Copy link

@samtlewis @mpociot it will be really helpful if we can merge this asap.

@toddgoates
Copy link

^ Ditto. I would love to use this on a Laravel 9 app.

@colinmackinlay
Copy link
Contributor

colinmackinlay commented Apr 23, 2022

@samtlewis have you got this working in production or staging or even local yet? I've pulled it in and all my app does is attempt to forward any email that arrives but it throws this error:
Symfony\Component\Mime\Message::setBody(): Argument #1 ($body) must be of type ?Symfony\Component\Mime\Part\AbstractPart, string given

The stacktrace ends with this:

#0 ...snip.../vendor/laravel/framework/src/Illuminate/Support/Traits/ForwardsCalls.php(23): Symfony\\Component\\Mime\\Message->setBody()
#1 ...snip.../vendor/laravel/framework/src/Illuminate/Support/Traits/ForwardsCalls.php(52): Illuminate\\Mail\\Message->forwardCallTo()
#2 ...snip.../vendor/laravel/framework/src/Illuminate/Mail/Message.php(368): Illuminate\\Mail\\Message->forwardDecoratedCallTo()
#3 ...snip.../vendor/beyondcode/laravel-mailbox/src/InboundEmail.php(171): Illuminate\\Mail\\Message->__call()
#4 ...snip.../vendor/laravel/framework/src/Illuminate/Mail/Mailer.php(267): BeyondCode\\Mailbox\\InboundEmail->BeyondCode\\Mailbox\\{closure}()
#5 ...snip.../vendor/laravel/framework/src/Illuminate/Mail/MailManager.php(505): Illuminate\\Mail\\Mailer->send()
#6 ...snip.../vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(337): Illuminate\\Mail\\MailManager->__call()
#7 ...snip.../vendor/beyondcode/laravel-mailbox/src/InboundEmail.php(172): Illuminate\\Support\\Facades\\Facade::__callStatic()
#8 ...snip.../app/Providers/AppServiceProvider.php(119): BeyondCode\\Mailbox\\InboundEmail->forward()

Scratching my head at the moment!

@colinmackinlay
Copy link
Contributor

@samtlewis - I've looked again at the changes you've made and they don't seem quite right. Apologies if I'm missing something obvious.

            if (app()->version() >= 9) {
                $mailable->withSwiftMessage(function (\Swift_Message $message) {
                    $message->getHeaders()->addIdHeader('In-Reply-To', $this->id());
                });
            } else {
                $mailable->withSymfonyMessage(function (\Swift_Message $message) {
                    $message->getHeaders()->addIdHeader('In-Reply-To', $this->id());
                });
            }
  1. Shouldn't the conditional be the other way round? For Laravel 9+ it should be Symfony not Swift?
  2. When calling withSymfonyMessage it won't be a Swift_Message that is passed?

It still doesn't explain my issue with forwarding. At the moment I think we're going to have to wait until @sschlein or @mpociot have time to give the package a Laravel 9 facelift

@samtlewis
Copy link
Contributor Author

@colinmackinlay i believe you are correct. That code is backwards. I am using the ingestion functionality but not reply/forwarding. Probably should have added some tests for that.

Copy link

@bramdevries bramdevries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested out these changes on a Laravel 9 installation and they were working correctly. We're not specifically using the reply() behaviour but the changes I suggested should do the trick.

@mpociot Could you (or anyone else at Beyondcode) take a look at this PR?. A lot of people are currently blocked from upgrading to Laravel 9 (#105) and there has been no communication on wether you want to continue maintaining this package. The only way forward for us seems to be forking this package and maintaining our own version.

@@ -19,9 +19,15 @@ public function processLog(MessageSent $event)
return;
}

if (app()->version() >= 9) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably safer to change this to use version_compare as it's specifically made for this purpose.

$message = version_compare(app()->version(), '9.0.0', '>') ? $event->sent->toString() : $event->message;

$mailable->withSwiftMessage(function (\Swift_Message $message) {
$message->getHeaders()->addIdHeader('In-Reply-To', $this->id());
});
if (app()->version() >= 9) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

This should also be reversed as withSymfonyMessage is only available in Laravel 9, and withSwiftMessage is for Laravel 8 and below.

The callback of withSymfonyMessage is also still using Swift_Message. The correct implementation should be something like:

$mailable->withSymfonyMessage(function (Symfony\Component\Mime\Email $email) {
  $email->getHeaders()->addIdHeader('In-Reply-To', $this->id());
})

@joelharkes
Copy link

@samtlewis implemented in my fork: joelharkes/laravel-mailbox version 3.0.0

purposebuiltscott added a commit to PurposeBuilt/laravel-mailbox that referenced this pull request Aug 22, 2022
@mpociot mpociot merged commit b925d81 into beyondcode:master Dec 20, 2022
@mpociot
Copy link
Member

mpociot commented Dec 20, 2022

Thank you, and sorry that it took so long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants