From 6769a49af718516825fab248745efd5b7f608c78 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Feb 2022 17:08:54 +0100 Subject: [PATCH] CI: switch to GitHub Actions - step 3: linting This commit: * Adds a GH Actions workflow for the PHP lint CI check. * Removes all references to that action from the `.travis.yml` configuration. * Add a "Build Status" badge in the Readme to use the results from the GH `Lint` Action runs. * Adds two small tweaks to the `lint` script in the `composer.json`. Notes: 1. Builds will run on: - Select pushes using branch filtering similar to before. - All pull requests. - When manually triggered. Note: manual triggering of builds has to be [explicitly allowed](https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/). This is not a feature which is enabled by default. 2. If a previous GH actions run for the same branch hadn't finished yet when the same branch is pushed again, the previous run will be cancelled. In Travis, this was an option on the "Settings" page - "Auto cancellation" -, which was turned on for most, if not all, Yoast repos. The `concurrency` configuration in the GHA script emulates the same behaviour. 3. The default ini settings used by the `setup-php` action are based on the `php.ini-production` configuration. This means that `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT`, `display_errors` is set to `Off` and `zend.assertions` is set to `-1` (= do not compile). For the purposes of CI, especially for linting and testing code, I'd recommend running with `zend.assertions=-1`, `error_reporting=-1` and `display_errors=On` to ensure **all** PHP notices are shown. Note: this only needs to be done for the PHP version used for the actual linting. Note: the defaults will be changed in the next major of `setup-php` to be based on the `php.ini-develop` configuration, but that may still be a while off. Refs: * https://github.com/shivammathur/setup-php/issues/450 * https://github.com/shivammathur/setup-php/issues/469 4. As the prefixing of the dependencies needs a higher PHP version, the `setup-php` action is used twice within this workflow. The first time, it sets up PHP 7.2 to allow for creating the `vendor_prefixed` directory. The second time, it sets up the PHP version on which we want to do the actual linting. Note: last time I checked, I found that within a workflow you can only switch PHP versions once, but that is sufficient for our purposes. (may have changed in the mean time) 5. As the `composer.lock` file for this plugin contains dependencies which are locked at versions which are incompatible with the full range of PHP versions supported by the plugin, we need to do some juggling to allow the linting to work (on all supported PHP versions). Previously in Travis, an uncontrolled `composer update` would be run to get round this. Uncontrolled updates are generally not a good idea in CI as you may end up with passing tests based on updated dependencies, while in reality things would fail. So, for this workflow, I've chosen to approach this slightly differently (compared to Travis), but still tried to keep things as simple as possible. - First a full install is done on PHP 7.2. This will generate the `vendor_prefixed` directory, as well as set up all dependencies. - Next, the dependencies which are only needed for the prefixing are removed. This will remove a couple of dependencies which would automatically be loaded by Composer on loading of the autoload file (due to the use of `autoload.files` in those dependencies) and would trigger a parse error on PHP < 7.2 as they contain non-cross-version compatible code. - Now those incompatible dependencies are removed, we **_don't_** actually need to do a re-`install` of the dependencies on the target PHP version as all we'll be using is PHP-Parallel-Lint and PHP-Parallel-Lint is compatible with PHP 5.3 and higher, so the version installed on PHP 7.2 will work on all PHP versions on which we need to do the linting. It may be a bit counter-intuitive to do it like that, but it works and keeps the workflow as simple as possible, while still achieving the goal of the workflow. And we also avoid the uncontrolled update. 6. While the `setup-php` action offers the ability to [install the PHAR file for Parallel Lint](https://github.com/shivammathur/setup-php#wrench-tools-support), I've elected not to use that option as it would mean that we would not be able to use the `composer lint` script in the workflow, which would mean that the CLI arguments would have to be duplicated between the `composer.json` file and the `lint.yml` file. IMO, that would make this a typical point of failure where updates would be done in one, but not the other. If, at some point in the future, the Parallel Lint tool would start to support a config file for the CLI arguments, removing this point of failure, this choice can be (and should be) revisited. 7. Composer dependency downloads will be cached for faster builds using a [predefined GH action](https://github.com/marketplace/actions/install-composer-dependencies) specifically created for this purpose. The alternative would be to handle the caching manually, which would add three extra steps to the script. Note: Caching works differently between Travis and GH Actions. On GH Actions, once a cache has been created, it can't be updated. It can only be replaced by a new cache with a different key. As the PHP version, the `composer.json` and a potential `composer.lock` hash are all part of the key used by the above mentioned action, this difference should not have a significant impact. Ref: https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows 8. The Linting check will display the results inline in the GitHub code view using the [cs2pr](https://github.com/staabm/annotate-pull-request-from-checkstyle) tool. Differences with the Travis implementation: * There is a minor difference in the branch filtering being done for `push` events. Travis accepted PCRE regexes for the filtering. GH Actions uses glob patterns. The glob patterns now in place match the regexes as closely as possible, but are not an exact match to the "old" patterns. They should be sufficient for our purposes though. * The tag-based branch filter has not been added as that filter was only intended for use with the `deploy` stage. * Linting will now also be executed against PHP 8.1 and 8.2 (nightly). Linting runs against PHP 8.2 will be "allowed to fail" for the time being. Note: if any of the "allowed to fail" jobs actually fail, the workflow will show as successful, but there will still be a red `x` next to a PR. This is a known issue in GHA: https://github.com/actions/toolkit/issues/399 There are work-arounds possible for this, however, the work-arounds would hide failures even more, meaning that chances are they won't be seen until they actually become a problem (no longer allowed to fail), which is too late. Regarding the changes to the Composer `lint` scripts: * For the generic `lint` command, the `vendor_prefixed` directory is no longer excluded from the linting as the adjusted code **does** need to be cross-version compatible and safeguarding that PHP_Scoper doesn't introduce cross-version compatibility issues is a good thing. --- .github/workflows/lint.yml | 75 ++++++++++++++++++++++++++++++++++++++ .travis.yml | 16 ++------ README.md | 1 + composer.json | 4 +- 4 files changed, 82 insertions(+), 14 deletions(-) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 00000000000..1e80c17d06e --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,75 @@ +name: Lint + +on: + # Run on pushes to select branches and on all pull requests. + push: + branches: + - master + - trunk + - 'release/**' + - 'hotfix/[0-9]+.[0-9]+*' + - 'feature/**' + pull_request: + # Allow manually triggering the workflow. + workflow_dispatch: + +# Cancels all previous workflow runs for the same branch that have not yet completed. +concurrency: + # The concurrency group contains the workflow name and the branch name. + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + lint: + runs-on: ubuntu-latest + + strategy: + matrix: + # Lint against the highest/lowest supported versions of each PHP major. + # And also do a run against "nightly" (the current dev version of PHP). + php_version: ['5.6', '7.0', '7.4', '8.0', '8.1', '8.2'] + + name: "Lint: PHP ${{ matrix.php_version }}" + + # Allow builds to fail on as-of-yet unreleased PHP versions. + continue-on-error: ${{ matrix.php_version == '8.2' }} + + steps: + - name: Checkout code + uses: actions/checkout@v3 + + # For the purpose of linting the code, we need the `vendor-prefixed` directory to + # be created as the prefixed code should be linted to ensure there are no parse errors + # in the generated code against any of the supported PHP versions. + # The prefix-dependencies task makes use of reflection-based PHP code that only works on PHP > 7.2. + - name: Install PHP 7.x for generating the vendor_prefixed directory + uses: shivammathur/setup-php@v2 + with: + php-version: 7.2 + coverage: none + + - name: Install Composer dependencies and generate vendor_prefixed directory + uses: ramsey/composer-install@v2 + + # Remove packages which are not PHP cross-version compatible and only used for the prefixing. + # - humbug/php-scoper is only needed to actually do the prefixing, so won't be shipped anyway. + # - league/oauth2-client and its dependencies *are* the packages being prefixed, so linting the + # prefixed versions is sufficient. + - name: Delete dependencies which are not cross-version compatible + run: composer remove --dev --no-scripts humbug/php-scoper league/oauth2-client + + - name: Install PHP for the actual linting + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php_version }} + ini-values: zend.assertions=1, error_reporting=-1, display_errors=On + coverage: none + tools: cs2pr + + # We don't need to remove and reinstall the Composer dependencies as + # the only thing we'll use is the PHP-Parallel-Lint package (PHP 5.3+) + # and that means the package is compatible with all supported PHP versions, + # so we can just use the version installed on PHP 7.2. + + - name: Lint against parse errors + run: composer lint -- --checkstyle | cs2pr diff --git a/.travis.yml b/.travis.yml index e830edb65d0..783da7912bd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,17 +29,16 @@ jobs: - php: 7.3.24 env: WP_VERSION=latest WP_MULTISITE=1 COVERAGE=1 - php: 5.6 - env: WP_VERSION=5.8 WP_MULTISITE=1 PHPLINT=1 PHPUNIT=1 + env: WP_VERSION=5.8 WP_MULTISITE=1 PHPUNIT=1 # Use 'trusty' to test against MySQL 5.6, 'xenial' contains 5.7 by default. dist: trusty - php: 7.3 env: WP_VERSION=trunk PHPUNIT=1 - php: 7.4 - env: WP_VERSION=latest PHPUNIT=1 PHPLINT=1 + env: WP_VERSION=latest PHPUNIT=1 - php: 8.0 - env: WP_VERSION=latest PHPUNIT=1 PHPLINT=1 + env: WP_VERSION=latest PHPUNIT=1 - php: "nightly" - env: PHPLINT=1 - stage: 🚀 deployment name: "Deploy to S3" if: branch = deploy # Only build when on the `deploy` branch, this functionality is not used yet and is taking a long time to complete. @@ -143,7 +142,7 @@ install: - | if [[ ${TRAVIS_PHP_VERSION:0:1} == "8" || $TRAVIS_PHP_VERSION == "nightly" ]]; then travis_retry composer install --no-interaction --ignore-platform-reqs --no-scripts --no-suggest - elif [[ "$PHPUNIT" == "1" || "$COVERAGE" == "1" || "$PHPLINT" == "1" ]]; then + elif [[ "$PHPUNIT" == "1" || "$COVERAGE" == "1" ]]; then # Run composer update as we have dev dependencies locked at PHP ^7.0 versions. travis_retry composer update --no-interaction --no-scripts travis_retry composer install --no-interaction --no-scripts @@ -226,13 +225,6 @@ script: yarn test travis_time_finish && travis_fold end "JavaScript.tests" fi - # PHP Linting - - | - if [[ "$PHPLINT" == "1" ]]; then - travis_fold start "PHP.check" && travis_time_start - composer lint - travis_time_finish && travis_fold end "PHP.check" - fi # PHP Unit - | if [[ "$PHPUNIT" == "1" && ${TRAVIS_PHP_VERSION:0:1} != "8" && $TRAVIS_PHP_VERSION != "nightly" ]]; then diff --git a/README.md b/README.md index b158e7b3070..9732b80ee00 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,7 @@ # Yoast SEO [![CS](https://github.com/Yoast/wordpress-seo/actions/workflows/cs.yml/badge.svg)](https://github.com/Yoast/wordpress-seo/actions/workflows/cs.yml) +[![Lint](https://github.com/Yoast/wordpress-seo/actions/workflows/lint.yml/badge.svg)](https://github.com/Yoast/wordpress-seo/actions/workflows/lint.yml) [![Build Status](https://api.travis-ci.com/Yoast/wordpress-seo.svg?branch=master)](https://travis-ci.com/Yoast/wordpress-seo) [![Stable Version](https://poser.pugx.org/yoast/wordpress-seo/v/stable.svg)](https://packagist.org/packages/yoast/wordpress-seo) [![License](https://poser.pugx.org/yoast/wordpress-seo/license.svg)](https://packagist.org/packages/yoast/wordpress-seo) diff --git a/composer.json b/composer.json index 2286e52c9f3..22abc2e5e94 100644 --- a/composer.json +++ b/composer.json @@ -64,10 +64,10 @@ "@php ./vendor/phpunit/phpunit/phpunit -c phpunit-integration.xml.dist" ], "lint": [ - "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint . -e php --show-deprecated --exclude vendor --exclude vendor_prefixed --exclude node_modules --exclude .git" + "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint . -e php --show-deprecated --exclude vendor --exclude node_modules --exclude .git" ], "lint-files": [ - "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint --show-deprecated" + "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint -e php --show-deprecated" ], "lint-branch": [ "Yoast\\WP\\SEO\\Composer\\Actions::lint_branch"