fix(#3622262): require PHP 8.2 for the readonly classes in rc2 - #38
fix(#3622262): require PHP 8.2 for the readonly classes in rc2#38Decipher wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe module now requires PHP 8.2 or later. Composer metadata, Drupal metadata, documentation, changelog entries, CI jobs, and PHPUnit tests reflect this requirement. ChangesPHP 8.2 requirement alignment
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to 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)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
25e2a5c to
b70aba2
Compare
b70aba2 to
0f99b39
Compare
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,FileFieldPathsProcessFileLegacyandFileUrlHooksasfinal readonly class, and readonly classes need PHP 8.2. Rector runs with thephp82set, 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 nextdrush crstops with a parse error insrc/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.jsonrequiresphp >=8.2. This is cicciobat's commit.filefield_paths.info.ymldeclaresphp: 8.2, for sites that install without Composer. On PHP 8.1 the Extend page will not enable the module.CHANGELOG.mdgets an Unreleased entry that tells PHP 8.1 sites to pin rc1.PhpRequirementTestchecks that the declared minimum covers the code, and thatcomposer.jsonand the info file agree..gitlab-ci.ymlruns 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 setsPHP_VERSIONon thecomposer (previous major)job.CORE_PREVIOUS_PHP_MINcomes from the CI/CD settings, and a settings variable wins over any value in this file..gitlab-ci.ymlalso givesphpunit (previous major)an absolute path tophpunit.gitlab-ci.xml. Drupal 10's test bootstrap changes directory intoweb/, 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
readonlyfrom 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 nextmake lint-fix, andmake lintfails 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
PhpRequirementTestis a unit test with three cases. It works out the minimum PHP version the code insrc/needs, which is 8.2 while any class isreadonly, and checks the info file andcomposer.jsonagainst it. On rc2'scomposer.jsonand info file all three fail, the first withfilefield_paths.info.yml does not declare a minimum PHP version.On this branch they pass.PhpRequirementTestphp -lon PHP 8.1.34unexpected token "readonly"php -lon PHP 8.2.32requires php >=8.2CI on the floor guards Drupal 10. GitHub's
test-php-min-d10-stableruns 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 lintis green.Not in this MR
The Drupal.org
phpunitjob fails on cicciobat's merge request for a reason unrelated to this change. gitlab_templates 1.17.0 switched_PHPUNIT_CONCURRENTto 1 on 8 September (https://www.drupal.org/node/3620894). The template now hands our--configuration=phpunit.gitlab-ci.xmlto run-tests.sh, which resolves it fromweb/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
Documentation
8.x-1.0-rc1.Quality