Skip to content

Fix failing CI checks - #7

Open
jjroelofs wants to merge 5 commits into
1.xfrom
jur/1.x/fix-ci-checks
Open

jjroelofs wants to merge 5 commits into
1.xfrom
jur/1.x/fix-ci-checks

Conversation

@jjroelofs

@jjroelofs jjroelofs commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

What was broken

The drupal-lint job failed before phpcs ran. scripts/prepare-drupal-lint.sh installs the lint tools with two composer global require calls:

  • drupal/coder 9.0.1 (latest) requires PHP_CodeSniffer 4.
  • phpcompatibility/php-compatibility 9.3.5 (latest stable) only accepts PHP_CodeSniffer 2 or 3.

The second call cannot resolve against the first, so Composer exits with code 2 and the Drupal, DrupalPractice and PHPCompatibility checks never run.

What changed

  • scripts/prepare-drupal-lint.sh: the lint tools are installed in one composer global require on the stable line: drupal/coder:^8.3, phpcompatibility/php-compatibility:^9.3 and squizlabs/php_codesniffer:^3.13. This replaces the 10.0.0-alpha2 PHPCompatibility pin. Coder 8.3 is used because coder 9 needs PHP_CodeSniffer 4, which PHPCompatibility 9.3 does not accept.
  • scripts/prepare-drupal-lint.sh: drupal_core_version is set from $TARGET_DRUPAL_CORE_VERSION instead of the literal 11. The script's default of 11 is unchanged.
  • scripts/prepare-drupal-lint.sh: set -e is removed, and both composer global require lines end with || exit 1, so a failed install still stops the run.
  • README.md: the intro blockquote is re-wrapped. Only the line breaks changed.
  • rl_sorting.install: the unused $display_id key is dropped from the foreach in rl_sorting_install().

phpcs configuration and check levels are unchanged.

Checks addressed

  • drupal-lint

Decisions for review

  • Decided: set -e is removed. scripts/run-drupal-lint.sh sources the prepare script and records each standard's exit code itself, so set -e ended the run at the first failing standard. dxpr/rl 1.x and dxpr/analyze_posthog 1.x still keep it.
  • phpcs.xml still sets drupal_core_version to the literal 11. XML cannot read the variable, so it is not changed here.
  • Blocked: commit a608d69 is titled "Drop unused display key in rl_sorting_update loop", but its change is in rl_sorting_install(). Fixing the title needs a force push, which is not allowed here. A squash merge would clean it up.

drupal/coder 9.x requires PHP_CodeSniffer 4, but the latest stable
PHPCompatibility (9.3.5) only accepts PHP_CodeSniffer 2 or 3. Composer
therefore failed before phpcs ran. PHPCompatibility 10.0.0-alpha2 is the
first release that accepts PHP_CodeSniffer 4.
The Drupal coding standard flags line 3 of README.md at 83 characters.
The warning made phpcs exit non-zero, which stopped the drupal-lint
script before the DrupalPractice check ran.
DrupalPractice reports $display_id as an unused variable at line 76 of
rl_sorting.install. The loop only needs the display value by reference.
The value was 11 plus TARGET_DRUPAL_CORE_VERSION (11), giving 1111.

@jjroelofs jjroelofs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: works (CI green), but I would change the approach.

  1. Prefer the stable toolchain over php-compatibility:10.0.0-alpha2. The description lists the stable route (drupal/coder 8.3 with PHPCompatibility 9.3.5) as "not tested here". It is now tested: toast_image_editor #9 and local_tts #4 both get green CI with the same line: drupal/coder:^8.3, phpcompatibility/php-compatibility:^9.3 and squizlabs/php_codesniffer:^3. Pinning an alpha means the lint job can break on the next alpha. Using the same line in all repos also settles the "maintainer needs to choose" blocker.
  2. The description is incomplete. "What changed" lists only the PHPCompatibility pin. The diff also reflows the README, drops the unused $display_id in rl_sorting_install(), and changes drupal_core_version. Add those.
  3. drupal_core_version 11 is correct (the old form gave 1111). Since this script's default is already 11, $TARGET_DRUPAL_CORE_VERSION would work and match analyze and its siblings.
  4. Commit message: agreed that a squash merge is the right fix for rl_sorting_update, given no force push.
  5. set -e stays here, so the first failing standard still ends the run (see the note on analyze #39).

Install drupal/coder ^8.3, PHPCompatibility ^9.3 and PHP_CodeSniffer ^3.13 in one composer solve, replacing the 10.0.0-alpha2 pin. Pass TARGET_DRUPAL_CORE_VERSION to drupal_core_version instead of the literal 11. Drop set -e and end the composer installs with || exit 1.
@jjroelofs

Copy link
Copy Markdown
Contributor Author

Follow-up on the review:

  • Replaced the php-compatibility:10.0.0-alpha2 pin with the stable line: drupal/coder:^8.3, php-compatibility:^9.3, php_codesniffer:^3.13 (5c21d22).
  • drupal_core_version now reads $TARGET_DRUPAL_CORE_VERSION; set -e removed and || exit 1 added.
  • The description lists every change and drops the stale 'not tested here' wording.

Pushed back on: only the rl_sorting_update commit title. Rewording a pushed commit needs a force push, so use a squash merge.

All checks pass on the new head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant