InnoGE / laravel-msgraph-mail

Laravel Mail driver for Microsoft Office 365 using the MSGraph API
MIT License
29 stars 8 forks source link

[Bug]: Issue after updating to 1.3.0 #27

Closed galabl closed 6 months ago

galabl commented 6 months ago

What happened?

Error: Configuration key tenant_id for microso ft-graph mailer is missing. at /httpdocs/vendor/innoge/laravel-msgraph-mail/src/LaravelMsGraphMailServiceProvider.php:53) PHP: 8.2 Laravel: 9

I believe the issue is in this commit in LaravelMsGraphMailServiceProvider.php:

The requireConfigString method reports missing configuration because of $value = $config[$key]; so for example it looks for $config['tenant_id']; but this is how $config looks at least in my app

[
  'driver' => 'microsoft-graph',
  'host' => '',
  'port' => 587,
  'from' => [
    'address' => 'mail@example.com',
    'name' => 'Mail',
  ],
  'encryption' => '',
  'username' => '',
  'password' => '',
  'sendmail' => '/usr/sbin/sendmail -bs',
  'markdown' => [
    'theme' => 'default',
    'paths' => [
      0 => '/var/www/vhosts/example.com/httpdocs/resources/views/vendor/mail',
    ],
  ],
  'log_channel' => NULL,
  'mailers' => [
    'microsoft-graph' => [
      'transport' => 'microsoft-graph',
      'client_id' => 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx',
      'client_secret' => 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx',
      'tenant_id' => 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx',
      'from' => [
        'address' => 'mail@example.com',
        'name' => 'Mail',
      ],
    ],
  ],
]

It used to pull the config this way config('mail.mailers.microsoft-graph.tenant_id', '')

How to reproduce the bug

Update to 1.3.0

Package Version

1.3.0

PHP Version

8.2

Laravel Version

9.0

Which operating systems does with happen with?

Linux

Notes

No response

geisi commented 6 months ago

Hey @galabl thank you for reporting this issue. Unfortunately I'm currently unable to reproduce that. Can you dd() the $config array within the requireConfigString() method in the LaravelMsGraphMailServiceProvider and provide me the result?

galabl commented 6 months ago

het @geisi here's the screenshot of dd from requireConfigString(). requested-dd

Below is mail.php config too

<?php

return [

    /*
    |--------------------------------------------------------------------------
    | Mail Driver
    |--------------------------------------------------------------------------
    |
    | Laravel supports both SMTP and PHP's "mail" function as drivers for the
    | sending of e-mail. You may specify which one you're using throughout
    | your application here. By default, Laravel is setup for SMTP mail.
    |
    | Supported: "smtp", "sendmail", "mailgun", "mandrill", "ses",
    |            "sparkpost", "log", "array"
    |
    */

    'driver' => env('MAIL_DRIVER', 'microsoft-graph'),

    /*
    |--------------------------------------------------------------------------
    | SMTP Host Address
    |--------------------------------------------------------------------------
    |
    | Here you may provide the host address of the SMTP server used by your
    | applications. A default option is provided that is compatible with
    | the Mailgun mail service which will provide reliable deliveries.
    |
    */

    'host' => env('MAIL_HOST', 'smtp.mailgun.org'),

    /*
    |--------------------------------------------------------------------------
    | SMTP Host Port
    |--------------------------------------------------------------------------
    |
    | This is the SMTP port used by your application to deliver e-mails to
    | users of the application. Like the host we have set this value to
    | stay compatible with the Mailgun e-mail application by default.
    |
    */

    'port' => env('MAIL_PORT', 587),

    /*
    |--------------------------------------------------------------------------
    | Global "From" Address
    |--------------------------------------------------------------------------
    |
    | You may wish for all e-mails sent by your application to be sent from
    | the same address. Here, you may specify a name and address that is
    | used globally for all e-mails that are sent by your application.
    |
    */

    'from' => [
        'address' => env('MAIL_FROM_ADDRESS', 'hello@example.com'),
        'name' => env('MAIL_FROM_NAME', 'Example'),
    ],

    /*
    |--------------------------------------------------------------------------
    | E-Mail Encryption Protocol
    |--------------------------------------------------------------------------
    |
    | Here you may specify the encryption protocol that should be used when
    | the application send e-mail messages. A sensible default using the
    | transport layer security protocol should provide great security.
    |
    */

    'encryption' => env('MAIL_ENCRYPTION', 'tls'),

    /*
    |--------------------------------------------------------------------------
    | SMTP Server Username
    |--------------------------------------------------------------------------
    |
    | If your SMTP server requires a username for authentication, you should
    | set it here. This will get used to authenticate with your server on
    | connection. You may also set the "password" value below this one.
    |
    */

    'username' => env('MAIL_USERNAME'),

    'password' => env('MAIL_PASSWORD'),

    /*
    |--------------------------------------------------------------------------
    | Sendmail System Path
    |--------------------------------------------------------------------------
    |
    | When using the "sendmail" driver to send e-mails, we will need to know
    | the path to where Sendmail lives on this server. A default path has
    | been provided here, which will work well on most of your systems.
    |
    */

    'sendmail' => '/usr/sbin/sendmail -bs',

    /*
    |--------------------------------------------------------------------------
    | Markdown Mail Settings
    |--------------------------------------------------------------------------
    |
    | If you are using Markdown based email rendering, you may configure your
    | theme and component paths here, allowing you to customize the design
    | of the emails. Or, you may simply stick with the Laravel defaults!
    |
    */

    'markdown' => [
        'theme' => 'default',

        'paths' => [
            resource_path('views/vendor/mail'),
        ],
    ],

    /*
    |--------------------------------------------------------------------------
    | Log Channel
    |--------------------------------------------------------------------------
    |
    | If you are using the "log" driver, you may specify the logging channel
    | if you prefer to keep mail messages separate from other log entries
    | for simpler reading. Otherwise, the default channel will be used.
    |
    */

    'log_channel' => env('MAIL_LOG_CHANNEL'),

    'mailers' => [
        'microsoft-graph' => [
            'transport' => 'microsoft-graph',
            'client_id' => env('MICROSOFT_GRAPH_CLIENT_ID'),
            'client_secret' => env('MICROSOFT_GRAPH_CLIENT_SECRET'),
            'tenant_id' => env('MICROSOFT_GRAPH_TENANT_ID'),
            'from' => [
                'address' => env('MAIL_FROM_ADDRESS'),
                'name' => env('MAIL_FROM_NAME'),
            ],
        ],
    ]

];
galabl commented 6 months ago

FYI:

I tried it on new Laravel project and it works fine, as Laravel 9 reached EOL, I guess this is not at issue at all.

geisi commented 6 months ago

@galabl This package should be compatible with Laravel 9 and I tested Laravel 9 Projects which work fine with the 1.3.0 version so the issue probably has something to do with your app. From the information provided I do not see any issues with your config. So this should be correct.

galabl commented 6 months ago

@geisi If I remember correctly, my project started as laravel 6 and was updated last time to version 9.

The issue is within mail.php config which I confirmed right now, but my knowledge is not that good to know why is it like that.

As you can see at the start of my mail.php config there's a 'driver' => env('MAIL_DRIVER', 'microsoft-graph'),, I compared it to a fresh laravel mail.php config, and driver doesn't exist so I deleted it and added 'default' => env('MAIL_MAILER', 'log'),(copied from the fresh config file).

After that, this is the dd result from requireConfigString() new-dd

geisi commented 6 months ago

Yes this dd is correct and how it should be. Now your problem seem to be solved?

galabl commented 6 months ago

It is, it's just weird that this was solved by renaming driver to default in mail config

JohanFredrikVaren commented 4 weeks ago

Why was this closed?

A "^1.1" dependency on this package proved fatal, as 1.3.0 contains breaking changes, due to commit 2336ce20c which completely changes the supported configuration.

It went from this:

return new MicrosoftGraphApiService(
    tenantId: config('mail.mailers.microsoft-graph.tenant_id', ''),
    clientId: config('mail.mailers.microsoft-graph.client_id', ''),
    clientSecret: config('mail.mailers.microsoft-graph.client_secret', ''),
    accessTokenTtl: config('mail.mailers.microsoft-graph.access_token_ttl', 3000),
);

to this:

return new MicrosoftGraphTransport(
    new MicrosoftGraphApiService(
        tenantId: $this->requireConfigString($config, 'tenant_id'),
        clientId: $this->requireConfigString($config, 'client_id'),
        clientSecret: $this->requireConfigString($config, 'client_secret'),
        accessTokenTtl: $accessTokenTtl,
    ),
);

An outdated $config value such as the following would work just fine up until 1.3.0:

[
    "driver" => "microsoft-graph",
    "mailers" => [
        "microsoft-graph" => [
            "client_id" => "foo",
            "client_secret" => "bar",
            "tentant_id" => "baz",
            "from" => [
                "address" => "foo@bar.test",
                "name" => "Foo Bar Baz"
            ]
        ]
    ],
    "host" => "",
    "port" => "",
    ...
    "log_channel" => null
]

So while pre-1.3.0 was a bit specific in its settings access, that's what worked, and legacy Laravel installations that were set up with the "mailers" entry would still work, despite their legacy config format. Now they crash, meaning that while LaravelMsGraphMailServiceProvider now more correctly adheres to Laravel's config system, 1.3.0 was a serious semver violation, and should have been released as a new major version. Upgrading within a major version should be crash safe, period! Composer doesn't care about your READMEs ;-)