This commit adds an initial Dependabot configuration to:
* Submit pull requests for security updates and version updates for GH Action runner dependencies.
At a later point in time, it could be considered to enable it for Composer dependencies as well.
The configuration has been set up to:
* Run weekly.
* Submit a maximum of 5 pull requests at a time.
If additional pull requests are needed, these will subsequently be submitted the next time Dependabot runs after one or more of the open pull requests have been merged.
* The commit messages for PRs submitted by Dependabot will be prefixed with "GH Actions:" similar to previous PRs I've submitted manually for the same.
Yet another predefined action has had a major release.
This is, again, mostly just a change of the Node version used by the action itself (from Node 12 to Node 16).
Refs:
* https://github.com/codecov/codecov-action/releases
A number of predefined actions have had major release, which warrant an update the workflow(s).
These updates don't actually contain any changed functionality, they are mostly just a change of the Node version used by the action itself (from Node 14 to Node 16).
Refs:
* https://github.com/actions/checkout/releases
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
The postfix installation step fails regularly, resulting in failed CI builds which have to be restarted, while the failure is not due to anything in the PR.
This commit introduces a new action runner for the postfix install, which will automatically retry the install up to 3 times.
If it works as I expect it to, this should eliminate failed CI builds due to postfix installs erroring out.
Ref: https://github.com/marketplace/actions/retry-step
The Codecov service is a way to monitor test vs code coverage of a project over time and allows for the code coverage % + delta to be reported in each PR.
This commits:
* Adds a Codecov configuration.
* Adds a convenience script to the `composer.json` file to run the tests with or without code coverage.
* Adds a new matrix variable to the GH Actions `test` workflow to run the tests with code coverage and send the results to the Codecov service.
Notes:
- This disables the code coverage reporting in the "normal" test runs, including disabling `xdebug` for those runs which should make them slightly faster.
- This splits the test runs into two sets:
* High/low PHP are being run with code coverage (and have been removed from the "normal" test run matrix).
* For all other PHP versions, the tests are being run without code coverage.
* Adds a badge to the README to show the current code coverage %.
PR 2373 changed the CI in such a way that coding standards errors will now be shown inline in the code of PRs.
With that change in place, having the information about running PHPCS in the pull request template has become redundant.
This commit:
* Add a new dependency on the PHP Parallel Lint package for fast PHP linting.
The PHP Parallel Lint package, in combination with the PHP Console Highlighter provides the following advantages in comparison with "plain" PHP linting:
- Higher speed due to the parallel processes.
- Improved usability by providing color coded syntax highlighting of found errors on the command-line.
- Integration with the `cs2pr` tool, allowing for the results of the lint command to be shown in-line in PRs.
* Adds a Composer `lint` script for easy access to the tool for devs, while making sure the correct command line parameters will be used.
The linting command as currently set up, will also check the example files for linting errors.
* Adds a GH Actions job for linting the code on the high/low supported PHP versions, one arbitrary interim version + an experimental build against PHP 8.1.
The `cs2pr` tool has been enabled and will show the results of the non-experimental lint runs in-line in PRs.
**Note**: For PHP 8.1, the `cs2pr` tool is not used as there is a known issue in the Parallel Lint tool with PHP 8.1 which breaks on the checkstyle reporting. There is already a [PR open](https://github.com/php-parallel-lint/PHP-Parallel-Lint/pull/64) to fix this upstream. Once this PR has been merged and a new version of Parallel Lint has been released, the separate step for PHP 8.1 linting can be removed.
* Makes the `test` job in the GHA workflow dependent on the `lint` job having passed...
... as the tests would fail anyway if there are linting errors.
Also adjusts the name of the `test` jobs to include the word "Test" so they can be easily distinguished from the Lint jobs.
Refs:
* https://github.com/php-parallel-lint/PHP-Parallel-Lint
The `xdebug` extension is already tagged as needed via the `coverage` setting, no need to add it to the `extensions` list.
---
Note: generally speaking, I personally normally don't pass an `extensions` list and allow the `setup-php` action to run with the default extensions, which is sufficient in most cases and would be sufficient here as well.
More than anything, I use the `extensions` key to _disable_ extensions for certain test runs, rather than enable them. Just something to consider.
The below documentation should give more insight.
Refs:
* https://github.com/shivammathur/setup-php/wiki/Php-extensions-loaded-on-ubuntu-18.04
* https://github.com/shivammathur/setup-php#heavy_plus_sign-php-extension-support
It is generally speaking a good idea to cache downloaded Composer packages between runs for performance reasons.
Now, this can be set up manually and would add two more steps to the scripts, or Ben's `composer-install` action can be used which will handle it all for you. The `composer-install` action is versatile and allows for passing additional parameters, so is perfectly suitable for this.
Ref: https://github.com/marketplace/actions/install-composer-dependencies
Running against stable/lowest dependencies is relevant when a package has runtime (non-dev) dependencies.
However, PHPMailer does not have runtime dependencies.
In other words, the `dependency-version` matrix key is redundant and unused, so we may as well remove it.
* GH Actions: run on PRs and allow for manually triggering
Currently the workflow only ran on `push` events, which - as forks have to enable the workflows - means that PRs could be submitted without CI having been run and you'd only see the CI results on merge.
By adding the `pull_request` event, it is ensured that CI is always run within the main repo for pull requests. This also allows for branch protection to be enabled with "required statuses".
Additionally, triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed.
This is useful if, for instance, an external action script or composer dependency has broken.
Once a fix is available, failing builds for `master` or open PRs can be retriggered manually instead of having to be re-pushed to retrigger the workflow.
Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/
* GH Actions: report CS violations in the PR
Currently the PR template asks for people to run the CS tooling.
As the PHPCS tool is also run in the test workflow and this workflow - per the previous commit - will now also be run on pull requests, we can make life easier on contributors.
The cs2pr tool allows to display the results from an action run in checkstyle format in-line in the PR code view.
This commit enables this for PHPCS, which means that the code view will now show CS violations in the PR.
Ref: https://github.com/staabm/annotate-pull-request-from-checkstyle
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
The `docs` workflow to deploy the GH Pages website is run on pushes to `master`, but that includes pushes to `master` in forks, which obviously can't deploy to the GH Pages site.
This means that in forks (and there are nearly 9000 of them), this workflow will always fail, while in reality, it shouldn't be run in the first place.
So, I'd like to propose making this small change, which _should_ prevent the `docs` workflow from being run on forks.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
* GH Actions: start testing against PHP 8.1
The first alpha of PHP 8.1 has been released, so now seems like a good time to start running the tests against PHP 8.1.
For now, I've configured it to allow builds against PHP 8.1 to fail, while PHP 8.1 is still unstable.
Also: PHPUnit doesn't officially support PHP 8.1 yet, so to install PHPUnit 9.x on PHP 8.1, we need to use `--ignore-platform-reqs`, as otherwise PHPUnit 4.8 would be installed (last PHPUnit version without strict PHP version constraints).
* GH Actions: set error reporting to E_ALL
Turns out the default setting for `error_reporting` used by the SetupPHP action is `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT` and `display_errors` is set to `Off`.
For the purposes of CI, I'd recommend running with `E_ALL` and `display_errors=On` to ensure **all** PHP notices are shown.
Co-authored-by: jrfnl <jrfnl@users.noreply.github.com>
