Can be counted as BC break.

Can be counted as BC break.

@laoneo Not sure how this could be counted as BC break? The method always expect false returned when invalid email address passed, not an Exception. You want to keep the current code to have Exception throws here?

Just for information, I noticed this issue because in my extensions, I use MailHelper::isEmailAddress to validate if an email address is valid before sending emails. In case customer enter a typo in the address, for example test@domain,com, the method throws Exception and cause the whole process stopped, not something anyone would expect.

It doesn't matter about the return value, important is that the behavior changes in a library class, then it is a bc break. It is legit to throw exceptions in a function. When I have the code:

try {
\Joomla\CMS\Mail\MailHelper::isEmailAddress($email);
} catch (Exception $e) {
// Send here a message to the admin
}

It will not work anymore. I don't say that the pr is wrong, but it can be counted as bc break.

Is that really a real life use-case ? I would say that the method never throws Exception (it is not mentioned in doc block). I think the Exception was introduced by mistake when we start using PunycodeHelper::toPunycode() in the method but no one noticed about it

Serious, no developer would want to use try catch to check if an email is valid.

Technically it is a bc break, doesn't really matter if it is used or not. Anyway, I just wanted to highlight it, at the end of the day it is up to the RM to decide if they want to add it to the release or not.

I would agree if the Exception is included in the doc block of the method, but it is not in this case. Does not catch the Exception and have it thrown will make developer's life harder, with ugly code. Anyway, I will leave it to RM to decide

I think the fix is fine.
yea someone can have a try/catch work around, but they also should handle false case, no b/c for me.

Thanks @Fedik

I have tested this item ✅ successfully on d385651

I was able to test this successfully! Thanks @joomdonation! :)

I have tested this item ✅ successfully on d385651

Tested successfully, The PR does what the description says.

RTC

From reading the code in the MailHelper and the PunycodeHelper I would agree with @joomdonation and @Fedik that obviously the method shall return false in case of the email address not being valid, and that the PunycodeHelper helper throws an exception now might be the result of a former update of the "Algo26\IdnaConvert" dependency.

So if extension developers have caught the exception, they still had to check the result for being true in addition in their code, so this PR here will not break anything for them except of the try catch being obsolete.

✅ final test before merge with JBT

• Tested before PR
  • user@example.com - true
  • user.example.com - false
  • user@exa mple.com - false
  • user@exam!ple.com - false
  • test@domain,com - false
  • user@exam\xC2\xA0ple.com - exception Prohibited input U+000000A0
• Applied PR with Patch Tester
  • Same results, unless no exception on prohibited input, it results also in false

I would agree with @richard67 and the others. This is not a B/C break, because checking the return value of isEmailAddress() (true/false) was already required before. Throwing an exception here is actually a bug.

Thank you @joomdonation for your contribution. Thank you @laoneo for supporting this issue. Thank you @exlemor and @krishnagandhicode for testing. Thank you @richard67 for review.