Postponed on #3220220: Update guzzlehttp/psr7 to 2.1.0.

Problem/Motivation

Guzzle 7 is compatible with PSR-18 out of the box, but PSR-17 compatibility is only available with the 2.x branch of the guzzlehttp/psr7 component. (Yes, they acknowledge that the naming is confusing.)

Drupal currently uses Diactoros for its PSR-17 implementation. However, Laminas projects have recently adopted a policy of placing an upper bound on PHP version compatibility, meaning we need to wait for them to do a release after every PHP minor in order to support that minor, which means we could go 6+ months without supporting the latest PHP version. Therefore, it is desirable to look for alternatives to the Laminas packages.

Once #3220220: Update guzzlehttp/psr7 to 2.1.0 lands, PSR-17 support is available in Guzzzle and we can attempt to use it.

Proposed resolution

See if we can swap out Diactoros' PSR-17 implementation for Guzzle's.

Remaining tasks

TBD

User interface changes

N/A

API changes

TBD (but the PSR interfaces are supposed to be the public API, so...)

Data model changes

TBD

Release notes snippet

10.0.0: Composer dependency laminas/laminas-diactoros is removed. The functionality is now provided by Guzzle. If you have any project that depends on Diactoros' code beyond the functionality previously provided by core (uncommon), ensure sure it is declared as a dependency in your composer.json or change the code to use Guzzle.

9.4.0: Drupal 10 will switch from laminas/laminas-diactoros to Guzzle. It should not be necessary to make any changes unless you are directly referencing Diactoros classes. If your project does depend directly on any Diactoros code (uncommon), you should make sure it is declared as a dependency in your composer.json or change the code to use Guzzle.

CommentFileSizeAuthor
#15 3255243-15.patch9.79 KBkim.pepper
#9 3255243-8.patch9.79 KBlongwave
#7 3255243-7.patch13.22 KBlongwave
#3 3255243-3.patch7.5 KBlongwave

Comments

Gábor Hojtsy created an issue. See original summary.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Parent issue: #3220220: Update guzzlehttp/psr7 to 2.1.0 » #3251854: [META] Requirements for tagging Drupal 10.0.0-alpha1
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3255243-3.patch7.5 KB

Includes #3220220: Update guzzlehttp/psr7 to 2.1.0, the additional changes are pretty straightforward.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Needs review

Being bold and marking this straight off as NR as the tests passed and the changes are minimal.

Do we want to remove laminas/laminas-diactoros here as well?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Fail on PHP 8.0 is due to #3255439: Update PHP 8.0.0 to PHP 8.0.13 on DrupalCI to allow Drupal 10 testing on PHP 8.0.x.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Re removing laminas/laminas-diactoros, if this is the only reason we used it, IMHO this would be the best place to remove it.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3255243-7.patch13.22 KB 
+---------------------------+-------+---------+
| Production Changes        | From  | To      |
+---------------------------+-------+---------+
| guzzlehttp/psr7           | 1.8.3 | 2.1.0   |
| laminas/laminas-diactoros | 2.8.0 | REMOVED |
+---------------------------+-------+---------+
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

#3220220: Update guzzlehttp/psr7 to 2.1.0 was committed just now so this needs to be updated.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3255243-8.patch9.79 KB

Rerolled :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs release manager review

I think the only question here is what to do in a Drupal 9.4 variant of this patch? How to deprecate a purely 3rd party library where we are not wrapping it? The Drupal 10 patch looks RTBC though :) Tagging to make sure a release manager considers that question.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re #10 I think we just add it to the release notes of 10.0.0 - if anyone is using laminas/laminas-diactoros ideally they should depend on it explicitly anyway, and they can easily composer require it if their code breaks following the upgrade.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +Needs release note, +10.0.0 release notes

Tagging for needing release notes and to be included in release notes then.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Issue tags: -Needs release note +Needs change record

it needs CR now to pro

+++ b/core/core.services.yml
@@ -812,16 +812,16 @@ services:
   psr17.server_request_factory:
-    class: Laminas\Diactoros\ServerRequestFactory
+    class: GuzzleHttp\Psr7\HttpFactory

As `guzzlehttp/psr7` used now

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll because composer.lock conflicts.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 3255243-15.patch9.79 KB

Ran the command: COMPOSER_ROOT_VERSION=10.0.x-dev composer update --lock

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

Thanks @kim.pepper.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d9e7844 and pushed to 10.0.x. Thanks!

  • alexpott committed d9e7844 on 10.0.x 
    Issue #3255243 by longwave, kim.pepper, Gábor Hojtsy, andypost: Replace...
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Issue tags: -Needs release manager review, -Needs change record

Retrospectively untagging. Encouragingly easy one.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Fixed » Needs work
Issue tags: +9.4.0 release notes

Wow, great to see this in! So glad this was as straightforward and I'd dared hope it might be.

We do need some way to notify people of this change prior to D10. Haven't we raised deprecation warnings in the past when we remove a core Composer dep?

At a minimum we will say it is deprecated in the 9.4.0 release notes, but in the past we've been able to raise deprecation warnings related to it.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

Haven't we raised deprecation warnings in the past when we remove a core Composer dep?

I don't think we have. In this case code should only be relying on this via the services, and the API of those doesn't change.

For direct usages despite this, if a module depends on a composer dependency directly, they'll still get it pulled in. If they don't, they could get a hard break but we can't account for modules not declaring dependencies like that.

Searching contrib for Diactoros\Response finds a lot of what looks like incorrect type declarations (RedirectResponse, AjaxResponse) which should be pointing to Symfony's classes instead. This looks already-broken to me.

Usage of the things we're actually replacing here doesn't show up.

http://grep.xnddx.ru/search?text=Diactoros%5CResponse&filename=

The only core references we're changing outside of services.yml is in a couple of tests. For those cases it would be theoretically possible to provide a bridge class in 9.4.x and 10.0.x (extending Diactoros and Guzzle respectively), but don't think we should unless we can find a contrib usage.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs work » Fixed

I fixed the release notes to tell people what to do (declare their dependencies correctly) based on #21. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Adding CR link to the release notes as well.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Better place for the link.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.