s-ichikawa / laravel-sendgrid-driver

This library can add sendgrid driver into the laravel's mail configure.
MIT License
390 stars 91 forks source link

Laravel9 support #160

Closed s-ichikawa closed 2 years ago

s-ichikawa commented 2 years ago

I'm working on making support the laravel v9 now. https://github.com/s-ichikawa/laravel-sendgrid-driver/pull/159

There was a big change in the mail component from laravel9. https://github.com/laravel/framework/pull/38481

But I'm not using the laravel recently. so please try the for_laravel9 branch and create an issue if you face some problems.

If there is no critical issue until 2/22/2022, I'll merge into master branch and tag 4.0.0.

kustomrtr commented 2 years ago

Hey, thanks for the update. I've been testing the for_laravel9 branch and so far it's been working great!

Just a few things I encountered while setting this up, the new readme asks for adding these lines into the app.php under boostrap directory:

$app->configure('mail');
$app->configure('services');
$app->register(Sichikawa\LaravelSendgridDriver\MailServiceProvider::class);

unset($app->availableBindings['mailer']);

image image

I get the errors above. If I try to run the app, I get: Fatal error: Uncaught Error: Call to undefined method Illuminate\Foundation\Application::configure() in C:\git\project\bootstrap\app.php:55 Stack trace: #0 C:\git\project\public\index.php(47): require_once() #1 {main} thrown in C:\git\project\bootstrap\app.php on line 55

If I remove the "configure" lines, then I get this

Fatal error: Uncaught ReflectionException: Class "availableBindings" does not exist in C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php:877 Stack trace: #0 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(877): ReflectionClass->__construct('availableBindin...') #1 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(758): Illuminate\Container\Container->build('availableBindin...') #2 C:\git\project\vendor\laravel\framework\src\Illuminate\Foundation\Application.php(855): Illuminate\Container\Container->resolve('availableBindin...', Array, true) #3 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(694): Illuminate\Foundation\Application->resolve('availableBindin...', Array) #4 C:\git\project\vendor\laravel\framework\src\Illuminate\Foundation\Application.php(840): Illuminate\Container\Container->make('availableBindin...', Array) #5 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(1417): Illuminate\Foundation\Application->make('availableBindin...') #6 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(1453): Illuminate\Container\Container->offsetGet('availableBindin...') #7 C:\git\project\bootstrap\app.php(57): Illuminate\Container\Container->__get('availableBindin...') #8 C:\git\project\public\index.php(47): require_once('C:\\git\\govassis...') #9 {main} Next Illuminate\Contracts\Container\BindingResolutionException: Target class [availableBindings] does not exist. in C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php:879 Stack trace: #0 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(758): Illuminate\Container\Container->build('availableBindin...') #1 C:\git\project\vendor\laravel\framework\src\Illuminate\Foundation\Application.php(855): Illuminate\Container\Container->resolve('availableBindin...', Array, true) #2 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(694): Illuminate\Foundation\Application->resolve('availableBindin...', Array) #3 C:\git\project\vendor\laravel\framework\src\Illuminate\Foundation\Application.php(840): Illuminate\Container\Container->make('availableBindin...', Array) #4 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(1417): Illuminate\Foundation\Application->make('availableBindin...') #5 C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php(1453): Illuminate\Container\Container->offsetGet('availableBindin...') #6 C:\git\project\bootstrap\app.php(57): Illuminate\Container\Container->__get('availableBindin...') #7 C:\git\project\public\index.php(47): require_once('C:\\git\\govassis...') #8 {main} thrown in C:\git\project\vendor\laravel\framework\src\Illuminate\Container\Container.php on line 879

If I remove the unset line then all goes OK.

s-ichikawa commented 2 years ago

@kustomrtr Thank you for your trial and reporting. I'd like to make sure, that's when you try to install it on Laravel instead of Lumen, right? These configure are for the installation of Lumen.

kustomrtr commented 2 years ago

@s-ichikawa omg I feel stupid now, I re-checked the readme and you're in fact right. I was adding those Lumen settings on a Laravel installation, I'm blind hahaha. Thanks again!

s-ichikawa commented 2 years ago

OK, Thank you for your trying!

ghost commented 2 years ago

Also going to test this out! I did notice a small issue while upgrading:

We're using the SendgridTransport::SMTP_API_NAME constant, which doesn't exist anymore. Changed it to SendgridTransport::REQUEST_BODY_PARAMETER

s-ichikawa commented 2 years ago

@rjp-thijs Ahh, yes. I changed that const name. It might have been a bit rough as a way to change the const name. For the time being, leave SMTP_API_NAME as well and deprecate it. Thank you!

ghost commented 2 years ago

Just wanted to report back that we've been using this in production for about a week now and havent noticed any issues :)

Koozza commented 2 years ago

@s-ichikawa (FYI @rjp-thijs is my other account ;)) Migtht have cheered a bit too early. It seems that something messes up with our attachments. I'm going to dive into this issue now, but I just wanted to pre-warn you :) PDF and XML attachments get attached as they should, but our CSV's are added as plain text below the mail.

Koozza commented 2 years ago

Figured it out. Left a fix at the PR: https://github.com/s-ichikawa/laravel-sendgrid-driver/pull/159

s-ichikawa commented 2 years ago

@Koozza Fixed it! Thank you very much!

Koozza commented 2 years ago

I would be fine with releasing this as 1.4 :) Haven't noticed any other bugs!

s-ichikawa commented 2 years ago

I merged #159 and tagged 4.0.0. @kustomrtr @rjp-thijs @Koozza Thank you very much for your contributions!