Skip to content

fix(#3622262): require PHP 8.2 for the readonly classes in rc2 - #38

Open
Decipher wants to merge 2 commits into
8.x-1.xfrom
feature/3622262-php-parse-error
Open

fix(#3622262): require PHP 8.2 for the readonly classes in rc2#38
Decipher wants to merge 2 commits into
8.x-1.xfrom
feature/3622262-php-parse-error

Conversation

@Decipher

@Decipher Decipher commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Fixes https://www.drupal.org/i/3622262

Problem

8.x-1.0-rc2 does not parse on PHP 8.1. It declares ImageStyleTemporaryAccessCheck, EntityWithFileField, FieldWidgetSingleElementForm, FileFieldPathsProcessFileLegacy and FileUrlHooks as final readonly class, and readonly classes need PHP 8.2. Rector runs with the php82 set, and that set adds the modifier. rc1 had none. The first three changed in 5be2c0d, which adopted the scaffold and added the Rector config. https://www.drupal.org/i/3121826 added the other two in 74954f3.

Nothing in rc2 says it needs PHP 8.2. The module allows ^10.3, and Drupal 10 still supports PHP 8.1, so Composer installs rc2 on a PHP 8.1 site and the next drush cr stops with a parse error in src/Hook/EntityWithFileField.php.

Neither CI caught it. GitHub tests Drupal 10 on PHP 8.2 and up. The Drupal.org Drupal 10 leg runs PHP 8.1, and it ran by itself up to rc1, green on Drupal 10.5.6. Every 8.x-1.x pipeline since 18 June has left it manual, so nothing ran PHP 8.1 for rc2.

Fix

This builds on cicciobat's merge request, https://git.drupalcode.org/project/filefield_paths/-/merge_requests/80, and their commit stays theirs. One commit on top of it declares PHP 8.2 everywhere the module states a requirement:

  • composer.json requires php >=8.2. This is cicciobat's commit.
  • filefield_paths.info.yml declares php: 8.2, for sites that install without Composer. On PHP 8.1 the Extend page will not enable the module.
  • The README lists PHP 8.2 under Requirements. CHANGELOG.md gets an Unreleased entry that tells PHP 8.1 sites to pin rc1.
  • PhpRequirementTest checks that the declared minimum covers the code, and that composer.json and the info file agree.
  • .gitlab-ci.yml runs the Drupal 10 jobs on PHP 8.2. They ran on PHP 8.1, and the new constraint stops them at the Composer step. The override sets PHP_VERSION on the composer (previous major) job. CORE_PREVIOUS_PHP_MIN comes from the CI/CD settings, and a settings variable wins over any value in this file.
  • .gitlab-ci.yml also gives phpunit (previous major) an absolute path to phpunit.gitlab-ci.xml. Drupal 10's test bootstrap changes directory into web/, and PHPUnit 9 then reads the configuration again from there, so the relative path failed. The Drupal 10 leg had not run a test since the scaffold was adopted on 18 June.

The issue also offers moving readonly from the classes to their properties, which would keep PHP 8.1. Rector would undo that. withPhpSets(php82: TRUE) puts the class modifier back on the next make lint-fix, and make lint fails until it does. PHP 8.1 has had no security support since the end of 2025.

rc2 is already published without a PHP constraint, so Composer on PHP 8.1 will still pick rc2 after the next release is out. The changelog entry tells PHP 8.1 sites to pin rc1.

Tests

PhpRequirementTest is a unit test with three cases. It works out the minimum PHP version the code in src/ needs, which is 8.2 while any class is readonly, and checks the info file and composer.json against it. On rc2's composer.json and info file all three fail, the first with filefield_paths.info.yml does not declare a minimum PHP version. On this branch they pass.

Check 8.x-1.0-rc2 This branch
PhpRequirementTest 3 failures Passes
php -l on PHP 8.1.34 5 of 30 files fail with unexpected token "readonly" Unchanged, the code still needs 8.2
php -l on PHP 8.2.32 All 30 files parse All 30 files parse
Composer on platform PHP 8.1.34 Installs Refused, requires php >=8.2
Composer on platform PHP 8.2.0 Installs Installs

CI on the floor guards Drupal 10. GitHub's test-php-min-d10-stable runs Drupal 10 on PHP 8.2, and the gitlab.local Drupal 10 jobs now run the full suite there too. phpstan (previous major) still fails, as an allowed failure, with 30 errors about classes missing from the Drupal 10 build, such as the #[Hook] attribute.

No code in src/ changes, so codecov has nothing new to cover. make lint is green.

Not in this MR

The Drupal.org phpunit job fails on cicciobat's merge request for a reason unrelated to this change. gitlab_templates 1.17.0 switched _PHPUNIT_CONCURRENT to 1 on 8 September (https://www.drupal.org/node/3620894). The template now hands our --configuration=phpunit.gitlab-ci.xml to run-tests.sh, which resolves it from web/ and cannot find it. The gitlab.local template mirror predates 1.17.0, so this pipeline cannot show that failure. It needs its own fix before this goes to Drupal.org.

Summary by CodeRabbit

  • Requirements

    • PHP 8.2 or later is now required to install and run the module.
    • Composer and Drupal will reject PHP 8.1 and earlier.
  • Documentation

    • Updated the README and changelog with the new PHP requirement.
    • Users remaining on PHP 8.1 should use version 8.x-1.0-rc1.
  • Quality

    • Added automated checks to keep the declared PHP requirements consistent across project configuration.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: bbb50385-d4b4-4172-84b4-d74540606a58

📥 Commits

Reviewing files that changed from the base of the PR and between 25e2a5c and 0f99b39.

📒 Files selected for processing (3)
  • .gitlab-ci.yml
  • CHANGELOG.md
  • tests/src/Unit/PhpRequirementTest.php

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The module now requires PHP 8.2 or later. Composer metadata, Drupal metadata, documentation, changelog entries, CI jobs, and PHPUnit tests reflect this requirement.

Changes

PHP 8.2 requirement alignment

Layer / File(s) Summary
PHP 8.2 declarations and documentation
composer.json, filefield_paths.info.yml, README.md, CHANGELOG.md
Composer and Drupal metadata declare PHP 8.2. The README and changelog document the requirement and PHP 8.1 pin guidance.
PHP 8.2 CI configuration
.gitlab-ci.yml
Previous-major jobs use PHP 8.2. PHPUnit uses an absolute configuration path.
PHP requirement validation
tests/src/Unit/PhpRequirementTest.php
Tests compare Composer and Drupal requirements, detect source syntax requirements, and validate version and file parsing helpers.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0f99b

PHP 8.1 installations are consistently prevented and Drupal 10 CI now uses PHP 8.2. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: requiring PHP 8.2 because the rc2 code uses readonly classes. It is concise and specific.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (2 skipped: 2 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/3622262-php-parse-error

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.66%. Comparing base (7537322) to head (0f99b39).

Additional details and impacted files
@@           Coverage Diff            @@
##           8.x-1.x      #38   +/-   ##
========================================
  Coverage    83.66%   83.66%           
========================================
  Files           20       20           
  Lines          796      796           
========================================
  Hits           666      666           
  Misses         130      130           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Decipher
Decipher force-pushed the feature/3622262-php-parse-error branch 2 times, most recently from 25e2a5c to b70aba2 Compare September 11, 2026 01:43
@Decipher
Decipher force-pushed the feature/3622262-php-parse-error branch from b70aba2 to 0f99b39 Compare September 11, 2026 21:03
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