Code cleanup

[SamMousa]

This is a big cleanup PR that fixes a plethora of smaller issues and rewrites other parts.

Q	A
Breaks BC?	✔️ big time

[SamMousa]

Apologies for the lack of specificity but the library was so small that a quick rewrite that breaks BC was more efficient. Hope you have time to review and provide feedback.

Some things this PR does:

• Implement interfaces for message signing en encrypting, something Symfony lacks.
• Implement wrappers for Symfony's signers & encrypters that implement the new interfaces.
• Implement interface SymfonyMessageWrapperInterface which contains the getSymfonyEmail() function that our Mailer needs. This allows us to typehint against the interface not the class.
• Rewrote Logger to use the PSR provided trait, allowing us to implement just the log function. Added configurable severity mappings and configurable category.
• Removed with- functions in the Mailer, Yii components are traditionally mutable and reconfigurable so this pattern is not needed.
• Allow user to set their own TransportFactory this allows them to configure things like a custom HttpClient but also available transports and which Dispatcher, if any, to use.
• Added test coverage, almost all code is now fully covered, except Message which sits at ~80%. The proxy wrappers that just contain 2 lines of code like DkimSignerWrapper have been excluded from code coverage.

[SamMousa]

I'm gonna let this sit here for a bit while I test it in my production application.

[Krakozaber]

Is the completion of the work planned ?

[SamMousa]

Yes definitely. This branch is already in production so I'm pretty sure it works.
Needs a good review though.

[samdark]

@SamMousa shouldn't readme be adjusted?

[SamMousa]

Hmm, good point, very probably yes. Will try to make some time to finish this!

[nd4c]

I would recommend allowing them because if someone creates a new Transport that has a lot of 'options' it's much easier to see what they are rather than appending them to the 'dsn' string.

I find the symfony mailer to be kind of a backward step compared to Swift in a few areas. The configuration being one of them. He even expects the dsn string to be in a global constant. That's why he prefers the single string over an array. However, internally the DSN object parses the the string and converts it to individual settings. To me it just makes more sense to skip all that and use an array - much cleaner looking configuration. Of course, I kind of opinionated... Oh well.

[nd4c]

Also, I would recommend using 'protected' instead of 'private'. This allows the classes to be extended.

Quote from the "About Yii" section of the "Guide":
Yii is extremely extensible. You can customize or replace nearly every piece of the core's code. You can also take advantage of Yii's solid extension architecture to use or develop redistributable extensions.

And since this is an official Yii extension rather than an independent library it makes sense to me to follow that "extensible" aspect.

[SamMousa]

This allows the classes to be extended.

You keep coming back to this, but I believe it is a fundamental misunderstanding of how to write extensible code. Code being extensible does not have to mean that classes are extendable.
Anything you have mentioned wanting to do by using custom configuration or extending classes you can do better by using composition.

I would recommend allowing them because if someone creates a new Transport that has a lot of 'options' it's much easier to see what they are rather than appending them to the 'dsn' string.

The library we use uses a DSN though, and the DSN object inherently loses features like failover or roundrobin configuration. Which means that we sacrifice features from the parent library just to introduce our own configuration syntax which we then need to document and help users with.

Thinking it through a bit more I'm pretty sure that even leaving in the transport configuration option is not the best design.
Ideally you configure the factory and have it support your custom transport. The custom transport is than configured using a DSN like all other transports already supported by the library.

This is way cleaner:

[
    'class' => Mailer::class,
    'dsn' => "sendgrid+api://{$env->get('SENDGRID')}@default"
]

And if you need a custom factory implement that:

[
    'class' => Mailer::class,
    'transportFactory' => new Transport(new MyTransportFactory()),
    'dsn' => "my-custom-protocol://{$env->get('SENDGRID')}@default"
]

[nd4c]

Composition it is then. Of course, you realize that the Mailer class extends BaseMailer...

I don't see that strictly using a 'dsn' element is necessarily cleaner. When using certain Transports, yes, it's cleaner. But not for all Transports.

'transport' => [
	'scheme' => 'smtps',
	'host' => 'mail.longwinded.com',
	'port' => 2525,
	'username' => 'customersupport@mailingstation.anotherverylongurl.com',
	'password' => 'We prefertousea Verylongandunforgettablepassphrase insteadof a password',
	'options' => [
		'verify_peer' => 0,
		'local_domain' => 'example.org',
		'restart_threshold' =< 10,
		'restart_threshold_sleep' => 1,
		'ping_threshold' => 200,
	],
],

That would be a very long 'dsn' string. And, again, the DSN object just parses the string to get back to what's clearly displayed in the array.

[nd4c]

I guess if I wanted to use failover or roundrobin with such complicated transport settings I would probably need a Mailer Factory that could automatically compose the 'dsn' string from a much more readable config array. So it might make sense to just use the 'dsn' string. I just keep feeling that we'd be missing a nice feature.

[SamMousa]

Composition it is then. Of course, you realize that the Mailer class extends BaseMailer...

I do, there's no easy way around it. Also realize that the framework is +- 10 years old, its core code is not a best practice in 2022.

[Krakozaber]

samdark added this to the 3.0.0 milestone 9 days ago

does that mean never?

[samdark]

No. That means it will be a major release of this exact extension.

[machour]

@SamMousa thank you for reworking this!

I'm wondering if we could also include MailerEvents as Yii events? (definitely needed to inline CSS or stuff like this prior to sending the message)

[SamMousa]

@SamMousa thank you for reworking this!

I'm wondering if we could also include MailerEvents as Yii events? (definitely needed to inline CSS or stuff like this prior to sending the message)

Not in this pr. Let's do that after maybe

[tgerakitis]

just curious, what is the ETA on this PR?
or is it possible to require this branch using composer?
here's how to require the changes:
composer.json

{
    "name": "yiisoft/yii2-app-basic",
    "description": "Yii 2 Basic Project Template",
    ...
    "require": {
        ...,
        "yiisoft/yii2-symfonymailer": "dev-code-cleanup"
    },
    "repositories": [
        ...,
        {
            "type": "vcs",
            "url": "https://github.com/SAM-IT/yii2-symfonymailer"
        }
    ]
}

thanks for your hard work! 😊

[SamMousa]

or is it possible to require this branch using composer?

Yes. Google is your friend.

[samdark]

@SamMousa sorry it took so long. Would you please help closing issues this PR solves? Thanks.

[Krakozaber]

@samdark need a tag

[samdark]

@SamMousa, @Krakozaber is readme alright?

[SamMousa]

Yes it looks good and both configuration methods have test cases.

[nd4c]

@SamMousa, @Krakozaber is readme alright?

The readme has an error in the first configuration. The transport array can have a 'dsn" or it can have the other fields, 'scheme', etc. But it cannot have both. If the code finds a 'dsn' it will ignore the other fields.

Incorrect:

'transport' => [
                'scheme' => 'smtps',
                'host' => '',
                'username' => '',
                'password' => '',
                'port' => 465,
 =>               'dsn' => 'native://default',  <=
            ],

Correct:

'transport' => [
                'scheme' => 'smtps',
                'host' => '',
                'username' => '',
                'password' => '',
                'port' => 465,
            ],

[SamMousa]

Good catch, can you make a PR for that change @nd4c ?

[nd4c]

Also, I believe the 'scheme' settings is better to be left out.

'scheme' => 'smtps' is kind of misleading. It doesn't exactly force ssl, but rather causes the code to choose a different transport.

I forget the details, but it's the 'port' => '465' that cause the code to use ssl. And the default transport works better than the 'smtps' transport.

So the better config would be:

'transport' => [
                'host' => '',
                'username' => '',
                'password' => '',
                'port' => 465,
            ],

[nd4c]

I've never dones a PR before. Any tips?

[SamMousa]

Personally I believe you should be using DSN in all cases. The underlying library uses DSN and by not using it all you do is lose functionality.

I've never dones a PR before. Any tips?

For these simple things you can just do them inside Github and it will do most of the meta-work for you.
Go here: https://github.com/yiisoft/yii2-symfonymailer/blob/master/README.md and click the pencil top right.

Good luck!

[nd4c]

Actually the underlying library uses the inidividual fields. It will convert any DSN string to individual attributes on it's DSN object. And not all transports are easy to for a developer to "read" as a DSN string. (And for that matter, the DSN string is not technically a DSN, but rather a list of configuration settings...)

I will work on the README.md file.

[SamMousa]

Actually the underlying library uses the inidividual fields. It will convert any DSN string to individual attributes on it's DSN object. And not all transports are easy to for a developer to "read" as a DSN string. (And for that matter, the DSN string is not technically a DSN, but rather a list of configuration settings...)

All true, but the relevant part is the fact that the DSN is an opaque string.
So thinks like round-robin load balancing are not possible using the array syntax but they are supported using the DSN syntax.

In general the pattern is wrong. Any new feature added to the underlying library requires us to write and maintain new code, tests and documentation... instead we could just say: use a DSN, check the docs: https://symfony.com/doc/current/mailer.html and be done :-)

I mean I don't know based on our readme how I'd configure it to use Sendgrid.. The array syntax doesn't add any discoverability... I know however that if I just use the DSN I'm good to go.