User tests: Successful: Unsuccessful:
The MailTemplate class should work with an injected MailerInterface and not concrete implementation to be more flexible. Additionally this pr deprecates the optional parameter to be able to remove the deprecated Factory usage.
Mail is sent.
Mail is sent.

Mail is sent with attachment in HTML format.
Mail is sent with attachment in HTML format.
Please select:
Documentation link for guide.joomla.org:
No documentation changes for guide.joomla.org needed
Pull Request link for manual.joomla.org: joomla/Manual#620
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
| Category | ⇒ | Libraries |
I have tested this item ✅ successfully on a59506a
I followed the instructions, everything works fine.
I have tested this item ✅ successfully on a59506a
I have tested this successfully! Thanks @laoneo!
I have tested this item ✅ successfully on a59506a
I have tested this successfully! Thanks @laoneo!
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
Feature
PR-6.2-dev
|
||
RTC
RTC
| Labels |
Added:
RTC
|
||
I would question about the valid of this PR. Maybe someone with stronger OPP background should look at this more carefully
MailerInterface but in the code of MailTemplate class, you have code to check for a concrete implement. Is MailerInterface incomplete ?MailerInterface, any implementation which is not Mail class will loose the functions provided by MailTemplate (because you hard code the check with a concrete implement before calling certain code):All it does is to support the interface and doesn't restrict to the Mail class as we should work with interfaces. If you need these parameters like isHTML, then extend from Mail class. But there is no point to not work with interfaces. If I have a plugin which redirects everything to a custom Mailer service or whatever, there is no need to configure SMTP, etc. This brings a lot of flexibility while not loosing any settings from before.
I think it just a fake support to interface because if you pass any implement of the interface which is not a Mail instance, you will loose the features of MailTemplate which I mentioned earlier. Plus the support interface while having to check for concrete implement seems not right to me.
That's what I see from looking at the change in this PR. I will leave it to other maintainers and release managers to look at it further and make final decision.
Due all respect, but your comment doesn't make sense at all. This is how support for the interface should be implemented in a backwards compatible way. Mailer instances which come from the factory doesn't have to be always a Mail class. If a plugin would implement it's own mailer (which is not a Mail class) and set it in the global container, then this code would badly break. When this gets merged, then we can convert the rest of the code away from Factory::getMailer, otherwise we will have to life forever with it.
Sorry, I do not see the point of supporting interface causing loosing features of MailTemplate. I have expressed my concerns in my earlier comments, it's normal that you do not agree or I am wrong. Let's wait for feedbacks from others.
Sorry, I do not see the point of supporting interface if it causes loosing features of MailTemplate. I have expressed my concerns in my earlier comments, it's normal that you do not agree or I am wrong. Let's wait for feedbacks from others.
No problem, don't expect it either.
I would like to have two new tests - with Html tested.
| Status | Ready to Commit | ⇒ | Pending |
Back to pending. See previous comment.
Back to pending. See previous comment.
Please test attachments too.
I'v updated the testing instructions with HTML and attachment test.
| Labels |
Removed:
RTC
|
||
| Category | Libraries | ⇒ | Libraries Front End Plugins |
| Category | Libraries Front End Plugins | ⇒ | Administration com_config Libraries Front End Plugins |
Hi @laoneo, Thanks for the PR.
This is the testing I've done but I am not sure about the conclusion.
C:\laragon\bin\sendmail\outputResult BEFORE PR: Mail is sent from only the main email (first user) to the sender who sent the test email (first or second user). The sender is constant (first user). Either it's first or second user who sent the mail, the sender is always the first user's email.
Result AFTER PR: Same behavior - mail is sent successfully.
https://downloads.joomla.org/api/v1/latest/cmsResult BEFORE PR:
Result AFTER PR:
Both the normal branch and this PR have the same functionalities. The task success notification always uses the first user who created the task as both sender and receiver, ignoring the configured user group in the notification settings.
I get this attachment in the email:
{"branches":[{"branch":"Joomla! 1.0","version":"1.0.15"},{"branch":"Joomla! 1.5","version":"1.5.26"},{"branch":"Joomla! 2.5","version":"2.5.28"},{"branch":"Joomla! 3","version":"3.10.12"},{"branch":"Joomla! 4","version":"4.4.14"},{"branch":"Joomla! 5","version":"5.4.5"},{"branch":"Joomla! 6","version":"6.1.0"}]}Is this the expected output?
If this is the intended behavior for task success notifications (always going to the main user from the main user regardless of who sent/create it, not to configured user groups), then I will mark this test as successful as there are no regressions introduced by this PR.
Note: Receiver changes in the Test 1: Basic Mail Functionality
Could you please clarify?
Is the behavior the same as without the patch? If yes, then it looks like you probably found a bug.
Is the behavior the same as without the patch? If yes, then it looks like you probably found a bug.
It's the same behavior before and after the patch.
Mail system might be not accurate due to testing on local host. Maybe It needs a real host.
If it is the same as before the patch then it would be good if you can open an issue and mark your test as successful.
I have tested this item ✅ successfully on 6d52d72
Sorry for late reply, didn't notice the message.
I have tested this item ✅ successfully on 6d52d72
Sorry for late reply, didn't notice the message.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
| Labels |
Added:
Conflicting Files
|
||
I have tested this item ✅ successfully on b180180
tested plain, html and html with attachments
I have tested this item ✅ successfully on b180180
tested plain, html and html with attachments
I have tested this item ✅ successfully on a59506a
I followed the instructions, everything works fine.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47677.