PHPUnit just released version 9.5.10 and 8.5.21.
This contains a particular (IMO breaking) change:
> * PHPUnit no longer converts PHP deprecations to exceptions by default (configure `convertDeprecationsToExceptions="true"` to enable this)
Let's unpack this:
Previously (PHPUnit < 9.5.10/8.5.21), if PHPUnit would encounter a PHP native deprecation notice, it would:
1. Show a test which causes a deprecation notice to be thrown as **"errored"**,
2. Show the **first** deprecation notice it encountered and
3. PHPUnit would exit with a **non-0 exit code** (2), which will fail a CI build.
As of PHPUnit 9.5.10/8.5.21, if PHPUnit encounters a PHP native deprecation notice, it will no longer do so. Instead PHPUnit will:
1. Show a test which causes a PHP deprecation notice to be thrown as **"risky"**,
2. Show the **all** deprecation notices it encountered and
3. PHPUnit will exit with a **0 exit code**, which will show a CI build as passing.
This commit reverts PHPUnit to the previous behaviour by adding `convertDeprecationsToExceptions="true"` to the PHPUnit configuration.
Refs:
* https://github.com/sebastianbergmann/phpunit/blob/9.5/ChangeLog-8.5.md
* https://github.com/sebastianbergmann/phpunit/blob/9.5/ChangeLog-9.5.md
So far, this method did not have dedicated tests.
The test file this commit introduces, tests all aspects of the method as well as documents the current behaviour of the method for specific, outlier situations.
So far, the methods related to text localization - `PHPMailer::setLanguage()`, `PHPMailer::getTranslations()` and `PHPMailer::lang()` - did not have dedicated tests.
The test file this commit introduces, tests all aspects of these methods, including the changes introduced in response to 2418 and 2419, as well as documents the current behaviour of the methods for specific, outlier situations.
Language codes which were "language-script" based were not accepted by the regex used, which meant that the `sr_latn` script could never be loaded.
After discussion in 2418, it was decided to support "script" in a language code and to support it like so:
```
2-character language code [_] optional 4-character script code [_] optional 2-character country code
```
This combines the annotation forms of the following known standards:
* https://unicode-org.github.io/cldr-staging/charts/37/summary/root.html
* https://docs.oracle.com/cd/E23824_01/html/E26033/glset.html
* http://www.loc.gov/standards/iso639-2/php/code_list.php
This means that all of the below codes will now pass the language code validation:
```
sr
sr_latn
sr_rs
sr_latn_rs
```
But not:
```
sr_rs_latn
```
This commit applies the above change and also adjusts the "language code fall back" logic to take language codes with a script code into account.
Note: if the requested full "language-script-country" code is not available, "language-country" will take precedence over "language-script" for the fallback logic to find the appropriate translation file.
Related to 2418 - observation 4
When the `$langCode` is passed as "language code - country code" and no translation is found for the specified country variant, the localization would automatically fall back to English, even when there was a viable translation available in the "parent" language, i.e. just based on the _language code_.
This commit changes the logic in the `PHPMailer::setLanguage()` method for when a "lang-country" code is passed.
It will now try and find a "lang-country" file first, if not found, it will try to find a file for just the "lang" and only if that could also not be found, it will fall back to English.
Related to 2418 - observation 2
Includes using named subpatterns in the regex to make the regex self-documenting.
Includes removing a commented out line of code which is superseded anyway.
For "language code - country code" locale notations, it is common for the "country code" part to be provided in uppercase, like `pt_BR`.
The method currently did not allow for that correctly. The regex check would accept the language code, but then fail to find the file on *nix based systems as the file names are all in lowercase and non-Windows file systems are generally case-sensitive. Which means that in effect, the method, ended up falling back to the default language (English).
The change in this commit makes the handling of the provided `$langcode` case-tolerant.
Note: the language code used in the file names for the language files is still expected to always be lowercase, including for language files in custom paths!
Related to 2418 - observation 1
The return value of the method was inconsistent as it could return `true` even when the requested language was not loaded/set.
Previously, the method would:
* Return `false` when a `$langcode` was matched against the regex, but the file couldn't be loaded.
* Return `true` in all other cases, including when a `$langcode` other than `'en'` was passed, but it didn't match the regex, which meant that effectively the requested language was ignored and English was loaded anyway, but the function would still return `true`.
This has now been changed to:
* Return `true` when the requested language was loaded, whether `'en'` or another language.
* Return `false` when the requested language wasn't loaded and the language defaulted to English.
Related to https://github.com/PHPMailer/PHPMailer/issues/2418#issuecomment-876240778
The `mb_decode_mimeheader()` function uses the Mbstring internal encoding to decode.
In PHP 5.5, the default internal encoding was `ISO-8859-1`.
As of PHP 5.6, the default internal encoding was changed to use the value from the `default_charset` ini setting. Additionally, in UTF-8, the value for `default_charset` was changed to `UTF-8`.
This means that when the charset is not explicitly set, the `mb_decode_mimeheader()` function may return garbled nonsense if the charset used to _encode_ does not match the charset per PHP's `default_charset` or - in PHP 5.5 - the Mbstring internal_encoding default.
So far, this wasn't making tests fail because of some hard-coded ini settings being passed in the CI.
However, changing the default ini values creates an assumption that that configuration will be used on all servers on which the PHPMailer code will be run.
This assumption is undocumented (not in the Readme or mentioned elsewhere) and will in most cases be incorrect.
The non-default ini values change the behaviour of PHP and were the cause of test failures against PHP 5.5 which I've been seeing for some of the new tests I've been creating.
Removing the changes fixes those errors, but exposes failing tests in the existing tests for `PHPMailer::parseAddresses()`.
These undocumented _changes_ to the default PHP configuration were **required** for PHPMailer to be able to parse the addresses successfully. As this library is open source and used in a wide variety of environments, those kind of assumptions can not safely be made.
So.... the hard-coded ini settings in the CI configuration ought to be removed.
This then causes the tests for the `PHPMailer::parseAddresses()` function to start failing on PHP 5.5.
> Note: the tests are only failing on PHP 5.5 as the test case causing the failure uses a UTF-8 encoded name and as of PHP 5.6, the default encoding used in PHP is UTF-8, which matches.
> If a test case would be added with a name encoded in a different charset, the tests would also start failing on PHP 5.6+.
To fix those failures and to make the code PHP cross-version compatible, including with PHP installs configured to use a different `default_encoding`:
* We need to make sure that the Mbstring "internal encoding" is set correctly based on the Charset used for PHPMailer.
* And then need to _reset_ the internal encoding after the use of the `mb_decode_mimeheader()` to prevent any impact of this change on the wider application context in which PHPMailer may be used.
As the `PHPMailer::parseAddresses()` method is `static`, it does not have access to the (non-static) `PHPMailer::$charSet` variable.
Knowing that, I've elected to add an additional, optional variable to the `PHPMailer::parseAddresses()` method to allow for passing in the charset and have set the default value for the parameter to be in line with the default value of the `PHPMailer::$charSet` variable.
I have adjusted existing method calls to this method to explicitly pass the charset.
Both of the adjusted function calls are in the "postSend" part of the PHPMailer logic when the charset will be known and final, so can be safely passed.
I've also made minimal changes to the unit test file to allow for passing the charset in the tests.
This implementation is based on the assumption that names can be encoded in different charsets.
If the name encoding only happens when the charset is UTF-8, the new function parameter can be removed and the charset can be set to UTF-8 directly.
As I'm not completely read-in on the RFC specs for the address header being parsed and when encoding happens, I'd like a second opinion on the currently chosen implementation.
If this is the correct way to go, then additional tests need to be added to safeguard that things works correctly when a different encoding is used.
If the encoding only happens for UTF-8, the implementation can be simplified.
Update: the current implementation is correct and a `@todo` note has been added to add more tests with different encodings during a next iteration on these tests.
Refs:
* https://www.php.net/manual/en/migration56.deprecated.php#migration56.deprecated.iconv-mbstring-encoding
* https://php-legacy-docs.zend.com/manual/php5/en/ini.core#ini.default-charset
As noted in 2380, the addressing tests do not belong in the mail transport tests and as the `ReplyToGetSetClearTest` now covers this extensively, the assertions can be removed from the `MailTransportTest`.
The "fakefunctions" are all nice and dandy to get past the `idnSupported()` check, but if either of these functions is not _really_ available, the actual behaviour of the `PHPMailer::addReplyTo()` function is to _fail_ (on the address validation call in `PHPMailer::addAnAddress()`) and return `false`.
In other words, this test was nonsensical as it tested for something which can, and should, never happen.
With this in mind, the test has been rewritten to reflect the *real* behaviour when either Mbstring or the `idn_to_ascii` function is not available.
This commit:
* Renames the `testDuplicateIDNRemoved()` method to `testNoDuplicateReplyToAddresses()`.
* Removes the call to `clearReplyTos()` at the start of the function as it is redundant. Each test gets a fresh instance of the PHPMailer class.
* Adds a number of additional assertions to the method to test the enqueuing and duplication removal in more depth.
* Ensures that all assertions are accompanied by a "failure" message so it can more easily be determined which assertion failed (in case of failure).
Includes adding `@covers` tags for the test.
This commit:
* Renames the `testConvertEncoding()` method to `testEnqueueAndAddIdnAddress()`.
* Removes the call to `clearReplyTos()` at the start of the function as it is redundant. Each test gets a fresh instance of the PHPMailer class.
* Adds a number of additional assertions to the method to test the enqueuing in more depth.
Includes adding `@covers` tags for the test.
... to test that the header gets set correctly when multiple reply-to addresses have been set, as well as when a reply-to address has been set without a name.
This commit:
* Renames the `testLowPriority()` method to `testReplyToInMessageHeader()`.
This test method was originally basically testing two things: the priority and the reply to header being set. For this class, it now just focusses on the reply to header, so letting the name reflect that.
* Enhances the test by actually testing that the reply-to header is correctly found in the fully composed message.
* Breaks out the test case to a data provider to allow for adding additional test cases more easily.
... to test that passing an invalid email address in combination with an instance of the `PHPMailer` class which was instantiated with `$exceptions = true` results in an exception.
Includes adding `@covers` tags for this specific method.
This commit:
* Adds two extra test cases.
* Adds handling to allow the "expected" registered values (and key) being different from the original input values.
This commit:
* Splits the primary tests previously contained in the `testAddressing()` method into two distinct test methods, each using a data provider to allow for adding additional test cases more easily.
* Enhances the tests by not only testing the return value of the `addReplyTo()` method, but also verifying that the `ReplyTo property has been set correctly and contains the expected information.
Includes adding `@covers` tags for these specific methods.
