... which is expected to be released this Thursday.
* Builds against PHP 8.5 are no longer allowed to fail.
* Update PHP version on which code coverage is run (high should now be 8.5).
* Add _allowed to fail_ build against PHP 8.6.
* Update the README.
Note: for some jobs I use "nightly" for the "next" PHP version, for some `8.6`. While it may appear there is no difference and this is true for the better part of the year, there is a difference for about two months.
To illustrate, consider PHP 8.5:
* PHP "nightly" refers to the PHP `master` branch, so was PHP 8.5 until the PHP 8.5 was branched off when the first RC was cut in September.
* As of that moment, "nightly" basically became PHP 8.6, so to test against PHP 8.5, one would need to explicitly request `8.5`.
* As of the release of PHP 8.5, it is expected for "nightly" to be PHP 8.6, so the difference is moot again.
For that reason, the unit test workflow uses the explicit `8.6` version for PHP "next".
As things were, test runs on forks would always fail on the "upload code coverage reports" step, as forks (justifiably) don't have access to the `CODECOV_TOKEN`.
Fixed now by updating the conditions to run that step.
> Users frequently over-scope their workflow and job permissions, or set broad workflow-level permissions without realizing that all jobs inherit those permissions.
>
> Furthermore, users often don't realize that the _default_ `GITHUB_TOKEN` permissions can be very broad, meaning that workflows that don't configure any permissions at all can _still_ provide excessive credentials to their individual jobs.
>
> **Remediation**
> In general, permissions should be declared as minimally as possible, and as close to their usage site as possible.
>
> In practice, this means that workflows should almost always set `permissions: {}` at the workflow level to disable all permissions by default, and then set specific job-level permissions as needed.
This was already addressed for the other two workflows, just not for the `tests` one.
As far as I can see, the jobs here do not need the `GITHUB_TOKEN` secret and even if they do, only for `content: read`, which for public repos does not need to be set explicitly, though it doesn't do any harm to have that set anyway.
Refs:
* https://docs.zizmor.sh/audits/#excessive-permissions
> By default, using `actions/checkout` causes a credential to be persisted in the checked-out repo's `.git/config`, so that subsequent `git` operations can be authenticated.
>
> Subsequent steps may accidentally publicly persist `.git/config`, e.g. by including it in a publicly accessible artifact via `actions/upload-artifact`.
>
> However, even without this, persisting the credential in the `.git/config` is non-ideal unless actually needed.
>
> **Remediation**
>
> Unless needed for `git` operations, `actions/checkout` should be used with `persist-credentials: false`.
>
> If the persisted credential is needed, it should be made explicit with `persist-credentials: true`.
This has now been addressed in all workflows.
Refs:
* https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
* https://docs.zizmor.sh/audits/#artipacked
Recently there has been more and more focus on securing GH Actions workflows - in part due to some incidents.
The problem with "unpinned" action runners is as follows:
* Tags are mutable, which means that a tag could point to a safe commit today, but to a malicious commit tomorrow.
Note that GitHub is currently beta-testing a new "immutable releases" feature (= tags and release artifacts can not be changed anymore once the release is published), but whether that has much effect depends on the ecosystem of the packages using the feature.
Aside from that, it will likely take years before all projects adopt _immutable releases_.
* Action runners often don't even point to a tag, but to a branch, making the used action runner a moving target.
_Note: this type of "floating major" for action runners used to be promoted as good practice when the ecosystem was "young". Insights have since changed._
While it is convenient to use "floating majors" of action runners, as this means you only need to update the workflows on a new major release of the action runner, the price is higher risk of malicious code being executed in workflows.
Dependabot, by now, can automatically submit PRs to update pinned action runners too, as long as the commit-hash pinned runner is followed by a comment listing the released version the commit is pointing to.
So, what with Dependabot being capable of updating workflows with pinned action runners, I believe it is time to update the workflows to the _current_ best practice of using commit-hash pinned action runners.
The downside of this change is that there will be more frequent Dependabot PRs.
If this would become a burden/irritating, the following mitigations can be implemented:
1. Updating the Dependabot config to group updates instead of sending individual PRs per action runner.
2. A workflow to automatically merge Dependabot PRs as long as CI passes.
Includes updating the version for `ossf/scorecard-action` as it was a couple of version behind.
Ref: https://docs.github.com/en/actions/reference/security/secure-use#using-third-party-actions
PHP 8.4 removes the IMAP extension (moved to PECL).
With this in mind, I've reviewed how the tests are being run versus the extension requirements and recommendations.
As things are, the tests are currently run in the "ideal" environment, i.e. with all required and optional extensions available.
However, the codebase also contains fall-backs for when certain extensions are **_not_** available and for at least some of those fallbacks, there are dedicated tests available, but in an ideal environment those tests will not run and the fall-backs are not tested, which is the case with the current CI setup.
To improve this situation, I'm proposing to keep running the tests against all PHP versions with the "ideal" extension set, but to also have additional test runs with a far more limited set of PHP extensions.
To determine which extensions should be in each set, I've looked at the following:
* `@requires` tags found in the test suite and the conditions for calls to `markTestSkipped()`.
This brought to light that the `openssl` extension was currently not listed in the "ideal" extension set. This has now been fixed.
* The required extensions of PHPUnit - `dom, json, libxml, mbstring, tokenizer, xml, xmlwriter`.
* The required extensions of PHPMailer itself - `ctype, filter, hash`.
* Not strictly required, but more for convenience/workflow speed: `curl` for Composer.
* And `xdebug` will still be enabled/disabled based on the `coverage` setting.
Note: while some tests would benefit from being run _without_ the `mbstring` extension, that's unfortunately not an option as `mbstring` is a requirement of PHPUnit 🤷
Also note, the tests with the "minimal" extension setup needs to run `composer install` with an `--ignore-platform-req` flag to prevent running into the following issue:
```
Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.
```
As this extension "requirement" is for a dependency which is not used in the test run, the extension requirement can be safely ignored.
Caches used in GH Actions do not get updated, they can only be replaced by a different cache with a different cache key.
Now the predefined Composer install action this repo is using already creates a pretty comprehensive cache key:
> `ramsey/composer-install` will auto-generate a cache key which is composed of
the following elements:
> * The OS image name, like `ubuntu-latest`.
> * The exact PHP version, like `8.1.11`.
> * The options passed via `composer-options`.
> * The dependency version setting as per `dependency-versions`.
> * The working directory as per `working-directory`.
> * A hash of the `composer.json` and/or `composer.lock` files.
This means that aside from other factors, the cache will always be busted when changes are made to the (committed) `composer.json` or the `composer.lock` file (if the latter exists in the repo).
For packages running on recent versions of PHP, it also means that the cache will automatically be busted once a month when a new PHP version comes out.
### The problem
For runs on older PHP versions which don't receive updates anymore, the cache will not be busted via new PHP version releases, so effectively, the cache will only be busted when a change is made to the `composer.json`/`composer.lock` file - which may not happen that frequently on low-traffic repos.
But... packages _in use_ on those older PHP versions - especially dependencies of declared dependencies - may still release new versions and those new versions will not exist in the cache and will need to be downloaded each time the action is run and over time the cache gets less and less relevant as more and more packages will need to be downloaded for each run.
### The solution
To combat this issue, a new `custom-cache-suffix` option has been added to the Composer install action in version 2.2.0.
This new option allows for providing some extra information to add to the cache key, which allows for busting the cache based on your own additional criteria.
This commit implements the use of this `custom-cache-suffix` option for all relevant workflows in this repo.
Refs:
* https://github.com/ramsey/composer-install/#custom-cache-suffix
* https://github.com/ramsey/composer-install/releases/tag/2.2.0
If there is a ruleset error, the `cs2pr` action doesn't receive an `xml` report and exits with a `0` error code, even though the PHPCS run failed (though not on CS errors, but on a ruleset error).
This changes the GH Actions workflow to allow for that situation and still fail the build in that case.
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 %.
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
jrfnl
Marcus Bointon
dependabot[bot]
Alex