Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Three of these Laminas dependencies need updating for PHP 8 compatibility. We may need to pin to a commit of theirs or to a release when it becomes available.
- https://github.com/laminas/laminas-diactoros/milestone/5
- https://github.com/laminas/laminas-feed/milestone/2
- https://github.com/laminas/laminas-escaper/milestone/2
Proposed resolution
Discuss what to do at least for the 9.1.0 beta.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
The following laminas components are updated for PHP 8 compatibility:
- laminas-diactoros to 2.5.0
- laminas-escaper to 2.7.0
- laminas-feed to 2.13.0
The only API affecting change is that \Laminas\Diactoros\Stream
will throw a \Laminas\Diactoros\Exception\RuntimeException
instead of an \Laminas\Diactoros\Exception\InvalidArgumentException
when used with a non-resource stream.
Comment | File | Size | Author |
---|---|---|---|
#13 | 3180207-13-9.1.x.patch | 8.4 KB | andypost |
#13 | 3180207-13.patch | 8.4 KB | andypost |
#13 | interdiff.txt | 3.71 KB | andypost |
Comments
Comment #2
alexpottThis has quite a few impacts:
I've asked in the laminas slack what is blocking release of the next version of laminas-feed (since that is the only dependency that actually needs a fix based on core usage). And the response that we need to check out the milestones - see https://github.com/laminas/laminas-feed/milestone/2 - so https://github.com/laminas/laminas-feed/pull/28 needs to be fixed in order for 2.13.0 to be released.
For what it is worth I think we should tell people to install with the composer flag
--ignore-platform-reqs
rather than pin to dev branches / commits if they want to use PHP 8.0. I think early adopters can implement this at less cost than the whole community using unsupported dev branches.Comment #3
alexpottAdding the milestone info to the issue summary so if people what to help upstream they know where...
Comment #4
Gábor Hojtsy@alexpott re #2, I think the idea was to update to the dev versions for the scope of the core beta and then reconsider for RC later on, in hopes the tagged releases would be out by that time.
Comment #5
MixologicOne thing worth noting about pinning to -dev releases:
It pins the code, *however* its not possible to pin the contents of the composer.json.
I.e. if the composer.json on the -dev branch changes, we will get those changes, even if they end up being incompatible with the version of the code we have.
This is because packagist does not store individual metadata for each commit on a dev branch, only the tip, so its generally risky if we think the composer.json might change from the time we pin until a release is created.
Comment #6
xjmHmm, if Laminas will only be releasing the PHP 8 compatibility in a minor release, that means that we would not be able to backport the fix to a patch release. Which means core would be incompatible with PHP 8 another six months until 9.2.
We've relied on a dev version commit hash of a Composer dependency before; is there anything that's different between this case and that previous case? If not I think having this dev version pinned until the minor is released is worth the drawbacks in #2, as that would potentially allow us to update to the stable minor in a future pre-release milestone or patch release, so long as there are not serious disruptions between the dev version and the minor milestone.
Comment #7
alexpottI think the only dev dependency we've had in composer.json for a very long time was a development dependency. That's not the same as a production dependency where we make promises about security. I'd be surprised if Laminas was willing to say that their development branch had any type of security support.
Amusingly, if you're installing from the tarball composer checks don't matter so the tarball already supports PHP 8. Currently the build will work just fine because the only "breaks" in Laminas are additionally warnings that would not appear on a properly configured production PHP (i.e. they are deprecations). If you are installing from composer then you can do the ignore-platform-reqs thing.
Also we will "support" PHP 8 the moment they release as our composer constraints will allow upgrading the moment they release. Requiring people to use drupal/core and not drupal/core-recommended for PHP 8 support seems a better trade-off than asking that project production packaging processes, security scanners etc cope with development branch dependencies.
Comment #8
andypostAdded FTR of replacement, it was a big change in minor release
Comment #9
andypostComment #10
andypostComment #11
andypostUpgrade to https://github.com/laminas/laminas-escaper/releases/tag/2.7.0
Comment #12
andyposthttps://github.com/laminas/laminas-diactoros/releases/tag/2.5.0
Comment #13
andypostAnd the last one https://github.com/laminas/laminas-feed/releases/tag/2.13.0
Comment #14
alexpottThis is not quite enough to see us over the PHP 8 line but it gets us very very close. #3156595: Make Drupal 9 installable on PHP8 has a Drupal based fix for the javascript test fails and also a fix for installing with platform reqs. The laminas updates work as expected. So that's great.
This is rtbc for 9.2.x
Unfortunately 9.1.0-rc1 was released today so in order to commit this to 9.1.x we're going to have to make a release management exception. In my opinion the API changes in the laminas components are not "real" API changes. The change of exception type in Diactoros is not one we or a Drupal module would be catching and handling differently. Both are going to produce a 500 and an entry in the error log. I would like to see us commit this to 9.1.x but this is a release manager call.
Comment #16
catchCommitted/pushed to 9.2.x.
Moving back to 9.1.x for discussion.
Linking the maintainer's reply and quoting relevant sections:
https://github.com/laminas/laminas-diactoros/pull/46#discussion_r526314997
They always tag a new minor release when they support a new PHP minor or major version, even though there aren't API changes, so this is really the same as updating to a new patch release for some other projects and is a product of their overly-conservative approach to PHP compatibility.
On the exception change, they don't consider it an API change, because it's not an exception that's designed to be caught (i.e. it's a proper failure exception) - this is what they said:
So based on this, I think this is the same de facto as updating to a patch release, even though it's been tagged as a minor release. Obviously it would have been considerably better if this had all happened a week ago.
I don't think it particularly hurts if we have to wait until 9.2.0 for full PHP8 compatibility though. We also have the option of letting the releases bed into 9.2.x for a little while, and doing the update for a Drupal 9.1 patch release (which has the same problems as doing it during RC in terms of updating to a 'minor', but more time for issues to get flushed out).
Comment #17
alexpottHere's a comparison of all the changes:
For what it is worth I really don't think there is any reason to not update even though 9.1.x is in the RC stage. 9.1.x is already actually PHP 8.0 compatible. The only issue is that we trigger PHP 8.0 deprecations for libxml_disable_entity_loader() - see https://www.drupal.org/pift-ci-job/1891300
I've triggered a run of this patch on PHP 8.0 against 9.1.x - should be green.
Comment #18
alexpottAdded a release node for 9.1.x
Comment #19
alexpottThis passes tests on 9.1.x on PHP 7.3 and PHP 8.0 - and we have a release note. Therefore this is rtbc pending a release management decision.
Comment #21
catchDiscussed this a fair bit with xjm, alexpott, Gabor and others.
Laminas has a completely different approach to semver than us, making the approaches incompatible. The only way we can get around this is to treat their minor release as a patch release (which effectively it is), and make an exception to our release policy so that we can update to it.
We'd not be in this position had we been able to update 2-3 weeks earlier when we could commit minor updates to the alpha/beta, but we are now.
So... Committed/pushed to 9.1.x backport.