When upgrading from 4.0.17 to 4.0.21 on a Drupal 9 with PHP 7.4, the following error occurs :
ParseError: syntax error, unexpected ')', expecting variable (T_VARIABLE) in Composer\Autoload\{closure}() (line 96 of modules/contrib/environment_indicator/src/ToolbarHandler.php).
If PHP < 8.0 is not supported anymore :
- it should be reflected in the composer.json
- it shoud at least be a minor version and not a bug fix version
Issue fork environment_indicator-3519033
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
liam morlandComment #5
trackleft2My merge requests adds back support for PHP 7.4
It looks like the issue was caused by the trailing comma on this line:
https://git.drupalcode.org/project/environment_indicator/-/merge_request...
This syntax is valid in newer PHP versions, but breaks on PHP 7.4, which the module currently supports.
To help catch similar issues, I’ve added a PHPUnit test to lint PHP files across supported versions:
https://git.drupalcode.org/project/environment_indicator/-/merge_request...
While this won’t catch the trailing comma specifically (as PHP 7.4 is no longer available in the GitLab runner environment), it sets us up for stricter compatibility checks going forward. I’d recommend holding off on using syntax that requires PHP 8.0+ until the module drops support for older versions in a future minor release.
I am also adding this issue to the #3483897: [META] Release Plan for Environment Indicator Patch Release 4.0.22
Comment #6
liam morlandIf changes like this are going to be made, it makes it very urgent to get a branch which is aimed at currently-supported versions of Drupal. This change introduces a coding standards error. Contributing to this module will become more difficult because people will need to remember to write in an old version of PHP, remembering what changes have been made and what coding standard issues to ignore.
Comment #7
trackleft2I agree that maintaining compatibility with PHP 7.4 makes contributions trickier, and there’s value in targeting modern environments. That said, this patch is specifically scoped to fix a regression introduced in a patch release (4.0.18–4.0.21) that broke sites still running PHP 7.4 — without any documented change in requirements.
To avoid repeating that mistake, I’d suggest the following sequence:
We’d appreciate review on any of the “Needs review” issues blocking the final 4.0.x release:
#3483897: [META] Release Plan for Environment Indicator Patch Release 4.0.22 or #3468997: [META] Release Plan for Environment Indicator Minor (feature) Release 4.1.0
Comment #8
liam morlandI think it would be reasonable to fix just the regression and then move on to 4.1.x.
Comment #9
trackleft2I've updated the PHPUnit tests so that they pass in all of the gitlab-ci jobs.
While updating the PHPUnit Test I was testing the module using the combination of PHP 8.3 and Drupal Core 11.0.13 as well.
Comment #10
trackleft2Also, it appears that all of the gitlab-ci tests are currently passing due to this issue: #3526179: Validate stage may use outdated artifact, leading to flaky test results
Comment #11
trackleft2Comment #13
trackleft2Comment #14
liam morlandYou could create a hotfix release based on 4.0.21 which just has the fix for this regression and then continue development of 4.x with a minimum of Drupal 10.3 in preparation for a 4.1.0 release.
Comment #15
trackleft2Sorry for taking so long in creating a release with this in it. We have a lot tied up in our release number at the moment and don't want to back track since our next patch release should drop any day now and it would be a lot of administrative work to sneak in a hotfix.
On the bright side, during the process of creating the new release, we did realize that we should be supporting 7.3 and Drupal 9.3 and changed that to our target.
Comment #16
trackleft2