Feature ? ? PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
8 May 2023

Summary of Changes

Introduces a new MailerInterface which does contain the most common functions which are needed to send a mail. A factory which is served through the container and the corresponding aware interface and trait are also shipped with this pr.

It also fixes an inconsistency, getting a mailer with the global configuration settings. Mail::getInstance() delivers the same object, but with a different configuration, either if Factory::getMailer() is called before or not. Here is an example:

Factory::getConfig()->set('mailfrom', 'test@example.com');
echo Mail::getInstance()->From; // Prints an empty string
echo Factory::getMailer()->From; // Prints test@example.com
echo Mail::getInstance()->From; // Prints now test@example.com

With this pr all calls do return as From value the EMail address from the global configuration.

Testing Instructions

Send a test mail while editing the global Joomla configuration.

Actual result BEFORE applying this Pull Request

Test mail is sent.

Expected result AFTER applying this Pull Request

Test mail is sent.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org: joomla/Manual#116

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2023
Category Administration com_config Libraries Unit Tests
avatar laoneo laoneo - open - 8 May 2023
avatar laoneo laoneo - change - 8 May 2023
Status New Pending
avatar laoneo laoneo - change - 8 May 2023
The description was changed
avatar laoneo laoneo - edited - 8 May 2023
aa930d8 8 May 2023 avatar laoneo cs
avatar laoneo laoneo - change - 8 May 2023
Labels Added: ? PR-4.4-dev
avatar wilsonge
wilsonge - comment - 8 May 2023

I don't think this is the correct implementation for this.

Note that factory doesn't actually set the mailer into it's list of statics - all it's doing is populating the data from JApplication. Funnily enough as things stand if all applications only called Mail::getInstance instead of Factory::getMailer then they wouldn't have this data populated despite the doc blocks. Now all instances of the mailer will have this information injected if they are using the factory which is a big behaviour change.

This is a much simpler case than the mvc classes. I think you just need an interface on the mail class and on top of that a new method in the application that bootstraps a mail instance that can be injected. This will also make the application dependent code much easier to unit test and from the models perspective it makes no diference anyhow whether that bootstrap of the from sender etc has actually taken place or not when the mail object is being injected.

avatar laoneo
laoneo - comment - 9 May 2023

More or less I was there at the beginning. Then I though that mostly you want to have the settings in the mailer instance from the global config.

Note that factory doesn't actually set the mailer into it's list of statics - all it's doing is populating the data from JApplication

This is not correct, it calls Mail::getInstance() which is cached, so it basically sets that config on the cached instance. A subsequent call on Mail::getInstance() delivers a mailer with the global settings. So actually this is a pretty inconsistent behaviour as you can get two different mailer, depending if Factory::getMailer() is called already or not. I'v updated the pr description to illustrate the current problem.

a new method in the application

Please no more functions which do make the application instance an even bigger god object.

avatar laoneo laoneo - change - 9 May 2023
The description was changed
avatar laoneo laoneo - edited - 9 May 2023
932a4f6 9 May 2023 avatar laoneo cs
fdc3a37 9 May 2023 avatar laoneo cs
avatar wilsonge
wilsonge - comment - 9 May 2023

This is not correct, it calls Mail::getInstance() which is cached, so it basically sets that config on the cached instance. A subsequent call on Mail::getInstance() delivers a mailer with the global settings. So actually this is a pretty inconsistent behaviour as you can get two different mailer, depending if Factory::getMailer() is called already or not. I'v updated the pr description to illustrate the current problem.

This was what I was trying to say - just badly!

More or less I was there at the beginning. Then I though that mostly you want to have the settings in the mailer instance from the global config.

OK but what is the value of having that central object stored in the container? Like the container is there to do dependency management not configuration of objects

Please no more functions which do make the application instance an even bigger god object

OK Then put it in JConfiguration as the only alternative - which is where all the variables come from. I don't see any benefit in the abstraction your providing...

avatar laoneo
laoneo - comment - 10 May 2023

OK but what is the value of having that central object stored in the container? Like the container is there to do dependency management not configuration of objects

Only the factory is in the container, not the mailer instance.

avatar ChrisHoefliger
ChrisHoefliger - comment - 24 May 2023

I have tested this unsuccessfully on localhost /w mailer set to SMPT
mailerror

avatar laoneo
laoneo - comment - 24 May 2023

Does it work without the patch? Just want to make sure that the settings itself do work.

avatar ChrisHoefliger
ChrisHoefliger - comment - 24 May 2023

Does it work without the patch? Just want to make sure that the settings itself do work.

Yeah, I tried without the patch first, that worked, then with patch applied and finally with patch removed. Only with applied patch no luck. This is the latest nighly build on PHP 8.2.1.

avatar laoneo
laoneo - comment - 24 May 2023

Did you try with the send test mail button?

avatar ChrisHoefliger
ChrisHoefliger - comment - 24 May 2023

Does it work without the patch? Just want to make sure that the settings itself do work.

It says 'Send a test mail while editing the global Joomla configuration.'
I assume that means press the send Test Mail button - that's what I did. Without the patch it says 'Test mail sent'; applying the patch, there's an error message and no mail gets sent.

avatar laoneo
laoneo - comment - 25 May 2023

@ChrisHoefliger can you delete the file /administrator/cache/autoload_psr4.php? Does it then still not work?

avatar ChrisHoefliger
ChrisHoefliger - comment - 25 May 2023

@ChrisHoefliger can you delete the file /administrator/cache/autoload_psr4.php? Does it then still not work?

After deleting autoload_psr4.php and then re-applying this patch, it still does not work. Without the patch, it works.

c2a1a51 26 May 2023 avatar laoneo docs
a4df9b9 26 May 2023 avatar laoneo cs
avatar ChrisHoefliger
ChrisHoefliger - comment - 26 May 2023

I have now re-tested this successfully on a test site, using SMTP mail on PHP 8.2

avatar ChrisHoefliger ChrisHoefliger - test_item - 26 May 2023 - Tested successfully
avatar ChrisHoefliger
ChrisHoefliger - comment - 26 May 2023

I have tested this item successfully on a4df9b9

Tested successfully with SMTP


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40560.

avatar toivo toivo - test_item - 28 May 2023 - Tested successfully
avatar toivo
toivo - comment - 28 May 2023

I have tested this item successfully on a4df9b9

Tested successfully in Joomla 4.4.0-dev of 28 May in Wampserver 3.3.1 64-bit on Windows 11 using SMTP and PHP 8.1.6.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40560.

avatar alikon alikon - change - 28 May 2023
Status Pending Ready to Commit
avatar alikon
alikon - comment - 28 May 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40560.

avatar heelc29
heelc29 - comment - 28 May 2023

ParamSignatureMismatch of MailerInterface to Joomla\CMS\Mail\Mail

libraries/src/Mail/Mail.php:134 PhanParamSignatureMismatch Declaration of function Send() : bool should be compatible with function send() : void defined in libraries/src/Mail/MailerInterface.php:34
libraries/src/Mail/Mail.php:197 PhanParamSignatureMismatch Declaration of function setSender(mixed $from, mixed|string $name = '') : \Joomla\CMS\Mail\Mail|bool should be compatible with function setSender(string $fromEmail, string $name = '') : void defined in libraries/src/Mail/MailerInterface.php:48
libraries/src/Mail/Mail.php:233 PhanParamSignatureMismatch Declaration of function setSubject(string $subject) : \Joomla\CMS\Mail\Mail should be compatible with function setSubject(string $subject) : void defined in libraries/src/Mail/MailerInterface.php:59
libraries/src/Mail/Mail.php:249 PhanParamSignatureMismatch Declaration of function setBody(string $content) : \Joomla\CMS\Mail\Mail should be compatible with function setBody(string $content) : void defined in libraries/src/Mail/MailerInterface.php:70
libraries/src/Mail/Mail.php:332 PhanParamSignatureMismatch Declaration of function addRecipient(mixed $recipient, mixed|string $name = '') : \Joomla\CMS\Mail\Mail|bool should be compatible with function addRecipient(string $recipientEmail, string $name = '') : void defined in libraries/src/Mail/MailerInterface.php:84
libraries/src/Mail/Mail.php:349 PhanParamSignatureMismatch Declaration of function addCc(mixed $cc, mixed|string $name = '') : \Joomla\CMS\Mail\Mail|bool should be compatible with function addCc(string $ccEmail, string $name = '') : void defined in libraries/src/Mail/MailerInterface.php:98
libraries/src/Mail/Mail.php:371 PhanParamSignatureMismatch Declaration of function addBcc(mixed $bcc, mixed|string $name = '') : \Joomla\CMS\Mail\Mail|bool should be compatible with function addBcc(string $bccEmail, string $name = '') : void defined in libraries/src/Mail/MailerInterface.php:112
libraries/src/Mail/Mail.php:397 PhanParamSignatureMismatch Declaration of function addAttachment(mixed $path, mixed|string $name = '', mixed|string $encoding = 'base64', mixed|string $type = 'application/octet-stream', string $disposition = 'attachment') : \Joomla\CMS\Mail\Mail|bool should be compatible with function addAttachment(string $data, string $name = '', string $encoding = 'base64', string $type = 'application/octet-stream') : void defined in libraries/src/Mail/MailerInterface.php:126
libraries/src/Mail/Mail.php:481 PhanParamSignatureMismatch Declaration of function addReplyTo(mixed $replyto, mixed|string $name = '') : \Joomla\CMS\Mail\Mail|bool should be compatible with function addReplyTo(string $replyToEmail, string $name = '') : void defined in libraries/src/Mail/MailerInterface.php:140
avatar laoneo laoneo - change - 28 May 2023
Labels Added: ?
avatar laoneo
laoneo - comment - 28 May 2023

Then phan messages can be ignored for now as the interface should be decoupled from the actual Mail implementation. Till the interface gets wider acceptance and we can change the methods in mailer we have to leave with the mismatch. PHP itself is accepting that and this is what counts for me at the moment. The target is to have an independent mailer interface without any reference to an implementation so we can exchange it on some point when there is a need for it.

avatar laoneo laoneo - change - 28 May 2023
Title
Add mailer interface and the respective factory
Introduce a mailer interface and the respective factory
avatar laoneo laoneo - edited - 28 May 2023
avatar MacJoom MacJoom - change - 14 Jun 2023
Labels Added: Feature
avatar MacJoom MacJoom - change - 14 Jun 2023
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-06-14 17:51:06
Closed_By MacJoom
avatar MacJoom MacJoom - close - 14 Jun 2023
avatar MacJoom MacJoom - merge - 14 Jun 2023

Add a Comment

Login with GitHub to post a comment