@bembelimen @ggppdk can you please test?

I have tested this item ✅ successfully on bdc9856

Would be great to have another tester so that this bug can be resolved with 3.8.4

I have tested this item ✅ successfully on bdc9856

Ready to Commit after two successful tests.

@mbabker can this be merged please

I don't think this is right. The way it looks now, this will ignore a duplicate address if exception throwing is enabled, unless there is something else internal to PHPMailer that throws an exception while sending over a duplicate address then we're ignoring an error state.

sendMail is just a shortcut for all the method calls needed to send a message just in one convenience function. It shouldn't behave differently than if I were to call addCC directly with the same parameters.

As i stated i just submitted the code proposed elsewhere. if its not correct then we should remove the rtc status.

So here's the issue. The add address stuff has a lot of different return possibilities made worse by the fact our subclass changes the return declarations of the parent class.

On a successful pass, $this->addCC() returns $this
On a failed pass because of invalid data (bad address type, bad email address, etc.) when Exception throwing is enabled then one is thrown else it returns boolean false (and $this->ErrorInfo will have the same message the thrown Exception would).
On a failed pass because the address is already enqueued, it returns boolean false with no error info.

Regardless of whether the email is invalid or duplicated, the parent API returns boolean false to indicate some form of error state because the address was not actually enqueued. We should respect that fail state and communicate it to the downstream caller. This PR changes the behavior so that the state is ignored when Exception throwing is enabled, absorbing an error.

Personally, I think there's nothing to patch here. The address wasn't added, it is correct to report the operation failed.

Regardless of whether the email is invalid or duplicated, the parent API returns boolean false to indicate some form of error state because the address was not actually enqueued. We should respect that fail state and communicate it to the downstream caller.

On a failed pass because of invalid data (bad address type, bad email address, etc.) when Exception throwing is enabled then one is thrown else it returns boolean false (and $this->ErrorInfo will have the

unfortunately that will not do it
exceptions are already enabled,
but it fails silently without an error to be caller , please see the following code

    protected function addAnAddress($kind, $address, $name = '')
    {
        if (!in_array($kind, array('to', 'cc', 'bcc', 'Reply-To'))) {
            $error_message = $this->lang('Invalid recipient kind: ') . $kind;
            $this->setError($error_message);
            $this->edebug($error_message);
            if ($this->exceptions) {
                throw new phpmailerException($error_message);
            }
            return false;
        }
        if (!$this->validateAddress($address)) {
            $error_message = $this->lang('invalid_address') . " (addAnAddress $kind): $address";
            $this->setError($error_message);
            $this->edebug($error_message);
            if ($this->exceptions) {
                throw new phpmailerException($error_message);
            }
            return false;
        }
        if ($kind != 'Reply-To') {
            if (!array_key_exists(strtolower($address), $this->all_recipients)) {
                array_push($this->$kind, array($address, $name));
                $this->all_recipients[strtolower($address)] = true;
                return true;
            }
        } else {
            if (!array_key_exists(strtolower($address), $this->ReplyTo)) {
                $this->ReplyTo[strtolower($address)] = array($address, $name);
                return true;
            }
        }
        return false;
    }

you can see that regardless of whether the exceptions are enabled,
this will fail silently, the code simply does not add the duplicate address and it reaches the final
return false;
without registering any error, anywhere !

• and failure without an actual indication of what went wrong, makes it look like a Joomla bug and a Joomla API bug

Personally i don't need this PR as i am aware of this behavour and i always check for duplicate emails inside to, cc, bcc, and i have patched my code to do the check a couple of years ago

If a better alternative to fix this is to throw somehow the real error as an exception,
then please do,

• this will force patching Joomla CMS calls and 3rd party extensions in several places
• or the check and exception throwing could be inside Joomla API before sendMail methods are called

both of the above are probably a more proper solution, than this one, but still something needs to be done as current silent failure is really bad, and as i said above it makes it look like a Joomla bug and a Joomla API bug

Yes, I am aware this specific case does NOT trigger exception throwing code, but the API is designed to return a boolean false when an address is not enqueued to the message even if it's not an error state (the address was not successfully added, it is wrong to treat it as a success case if the API returns boolean false when we expect a success state to return boolean true).

There is no better fix to this without extending more of the PHPMailer class to turn it into Joomla API. The garbage design of Joomla\CMS\Mail\Mail is exactly why I proposed #19368 which makes the internals of PHPMailer an implementation detail underneath a service layer and not directly exposed to the end user, where we can make more informed (and better) decisions about how to handle different scenarios. PHPMailer is not our code, we should not be altering the library's behavior, but fact is we have been since the day JMail has existed in the API.

File a bug with PHPMailer if you feel there is a need for a behavior change. But regardless of perception or developer error, the fact of the matter is the API is doing exactly what it is designed to do and that is alert the caller to a non-success state when performing an action and the shortcut sendMail method correctly checks for non-success states and aborts the operation when one is detected.

Continuing my answer above

i want to say that probably the authors of PHPMailer consider the duplicate address thing to be a "recoverable error"
thus it is simply skipped and email sending can continue without the duplicate address

so the behaviour they implemented of not throwing an error even when exceptions are enabled is deliberate

so i am rethinking that this PR may not be such a bad idea, we continue on a error that authors of PHPMailer consider to be ignoreable or "recoverable"

If you need it to be a recoverable error then don't use sendMail and make the individual method calls yourself. We should not assume that a duplicate email address false can be blissfully ignored.

Go back in history to 3.5.1 when we finally stopped blissfully ignoring errors in the mail API. This PR is suggesting to restore blissful ignorance of potential error states. No, just no.

@mbabker

This PR is suggesting to restore blissful ignorance of potential error states. No, just no.

there is no error to be ignored, simply because no exception was registered

the adding of address was denied, and false is returned to indicate this without an exception being thrown exactly because the authors of PHPMailer do not consider it an exception case

if we want to add more exception cases than PHPMailer, then sure we can
but this PR does not ignore the exceptions of PHPMailer

i understand what you are saying,
and i understand that Joomla does not need to treat duplicate address the same way as PHPMailer

the bottom line is that now we have a silent failure that looks really bad when it happens
it really looks as Joomla bug and argueably it is a Joomla bug

Personally i do not care of merging this PR, it can be closed

A boolean false means that the operation you requested to perform did NOT complete successfully. That's the bottom line. The address wasn't added, period. It might not be a "fatal" error, but it is most assuredly not a success state.

Even if I were to think this PR is a good idea, it introduces inconsistent behavior.

Add duplicate address and exception throwing enabled - the boolean false is ignored, send process continues

Add duplicate address and exception throwing disabled - the boolean false is caught and treated as an error, send process fails

We are respecting the return of the parent class we chose to extend by treating a boolean false as an error state. As this is a shortcut method, it is safer to err on the side of caution and not bypass any false return regardless of reason.

An extension developer can choose to safely ignore this state by not using the sendMail shortcut method and calling all required methods to send the message on their own.

I agree with what you are saying

just the bad thing is that PHPMailer does not register anywhere the reason for returning false ...
we have no way to know why the false was returned, no other way than guessing

I meant from API perspective we are guessing

if we look at PHPMailer code, then we know why

If the user knew it failed then there would be no need to do what this pr proposes.

At the extension level, you can make an informed decision and tell the user.

if ($mail->addCC($email) === false) {
    if (!$mail->ErrorInfo) {
        // This means it was a duplicate address, decide for yourself how to proceed
    } else {
        // This means there was a validation error, you probably shouldn't proceed
    }
}

But, that behavior is NOT something we should put into sendMail. The method is a convenient shortcut, but if you need more intricate handling of things then you need to make all the API calls yourself and not rely on the shortcut.

yes or no

lets not just leave this open forever please.

Personally I'm still no for reasons said before.

Closed