Fix: Ensure e-mails would always have a sender address set

[martin-rueegg]

Depending on the mailer used, the From-address of an e-mail would not be set (e.g. when using the test mailer from the testing framework).

Using a helper function to compose the message ensures that the sender is always set.

What kind of change does this PR introduce?

• Bugfix

Does this PR introduce a breaking change?

• No

The PR fulfills these requirements:

• It's submitted to the develop branch, not the master branch if no hotfix
• All tests are passing
• Changelog was modified

Other information:

[luke-]

@martin-rueegg Hmm, not sure if I like this approach, replace the mailer access with a helper class.
Is there any reason why we can implement this in the Mailer itself?

Maybe we could also use a different "TestMailer" in the Testing config.

[martin-rueegg]

@luke- Thanks for looking into this.

The Helper class already exists. And yes, we could move that function to the existing Mailer class instead. Might make more sense indeed. I've thought about it but then mistakenly concluded there would be a name conflict...

The current production mailer has this code implemented. However, since it's a configurable component, should the sender not be assured in any case? Even if a admin chooses to use his own implementation of the mailer?

In my personal view, it is the job of the mail client (in this case the code that is creating the mail) to make sure, the correct sender is set. As opposed to the mail service, to do this.. Likewise I'd make sure, the e-mail conforms to the minimum requirements, no matter what mailer class, service or server is used further down the road. This helper function (which I can happily move to the mailer class) is ensuring that.

[luke-]

@martin-rueegg Hmm, ok got your point.

Since we already modified and enhanced the Yii 2 mailer component, I don't think we should expect this to be configurable/replaceable with another custom class.

If we want to support this, you're right, we need some Helper/Service which handles defaults and customizations from HumHub core.

To keep it simple, I would treat this core component as non-modifiable.

I would also like to stay as close as possible to the Yii 2 standard/documentation/style and somehow it looks wrong to me now to replace the familiar Yii::$app->mailer->compose() with Mailer::composeEmail(). There would be certainly some modules that need to be changed here as well and most Yii 2 developer would still use mailer->compose() as learned.

So currently I would prefer to:

• Create a new humhub\components\mail\Mailer::setComposeDefaults() method and use this in humhub\components\mail\Mailer->compose()
• Create a new e.g. humhub\components\tests\Mailer which extends Codeception\Lib\Connector\Yii2\TestMailer and include humhub\components\mail\Mailer::setComposeDefaults() in compose there.
• Use this call in tests

What do you think?

[martin-rueegg]

@luke- Ok, sounds fair enough.

How about creating an additional interface humhub\interfaces\MailerInterface (which extends yii\mail\MailerInterface) to add to those two classes (i.e. humhub\components\mail\Mailer and humhub\components\tests\Mailer) and being required as return type for \humhub\interfaces\Application::getMailer()?

Now, of course we could also add a \humhub\interfaces\Application::setMailer($mailer) that either defines the interface on the parameter or does proper error handling in the function, refusing if a mailer would not implement our new interface.

I've made a suggestion in the current code.

[martin-rueegg]

Additionally, I've

• renamed the Application interface to ApplicationInterface, in line with code style requirements.
• merged the common properties and methods of the console and web Applications into a common trait, that now also hosts the new getMailer() method.

[luke-]

@martin-rueegg Thanks, looks really nice now! Can you take a look into the tests?

[martin-rueegg]

@luke- yes, I'm on it.

You suggested to

• Create a new e.g. humhub\components\tests\Mailer which extends Codeception\Lib\Connector\Yii2\TestMailer ...

I thought it would be nicer to have it under protected/humhub/tests/codeception/_support/TestHelper.php and in the tests\codeception\_support, rather than cluttering the main namespace with test stuff...

The current test failed because they could not load the tests\codeception\_support namespace. However, I've fixed that now locally. But there seems to be some \Codeception\Lib\Connector\Yii2\TestMailer::$callback that is now empty and hence not able to save/send the message. Not quite sure what's going on.

From index-test.php, which mailer is supposed to load?

[martin-rueegg]

OK, so \Codeception\Lib\Connector\Yii2::mockMailer which is called from \Codeception\Lib\Connector\Yii2::startApp() is overwriting the class property of the configuration.

So in order to cheat here, we could set a definition in Yii::$container ...

[martin-rueegg]

@luke-

I think I've nailed it. So it appears, that from the test server (index-test.php) the normal humhub mailer us used. And works.

The Codeception TestMailer is only used in unit tests (and maybe functional tests?). I've achieved to overwrite this by setting the configuration of the container in such a manner, that instead of the requested

• \Codeception\Lib\Connector\Yii2\TestMailer, it actually creates a
• \tests\codeception\_support\TestMailer instance!

(Yes, dem Ingeniör ist nichts zu schwör! ;-) )

Now let's see what the test suite says ...

[martin-rueegg]

Looking good! 👍

[luke-]

@martin-rueegg Thanks, is 'v1.16' ok as PR target?

[martin-rueegg]

@martin-rueegg Thanks, is 'v1.16' ok as PR target?

In theory, yes. However, we've already introduced the Application interface in 1.15. So we should at least add the name change to 1.15, maybe without enforcing the mailer interface.

I can create an other PR with these changes that we merge into develop, then I rebase this PR onto that one and we merge this into next. How does that sound?

Could you please update next with develop?

[luke-]

@martin-rueegg Yes thanks, sounds good. I've just updated next.