Fork laminas/laminas-diactoros

longwave → credited Spokje → .

guzzlehttp/psr7 has a similar advisory, crediting Spokje for pointing this out - we might as well solve both these updates in one issue, but now we need patches for all supported branches.

We might want to do 📌 Remove laminas-feed, laminas-escaper, and laminas-stdlib from drupal/core-recommended to allow Drupal 9.5 to be installed on PHP 8.2 Fixed first for sites running PHP8+ on 9.5 then worry about PHP 7.3/4 afterwards.

https://github.com/laminas/laminas-diactoros/issues/142 is won't fix and the Laminas maintainers are being characteristically unhelpful so if we want to fix this for PHP 7.3/7.4 users we will either have to swap out the class via class_alias() and autoloader trickery or fork and cherry-pick the commit ourselves.

I forked Diactoros at https://github.com/longwave/laminas-diactoros, cherry-picked the fix and tagged 2.14.1. I also added a replace section to composer.json so Composer should understand that if you also request laminas/laminas-diactoros, then this package can be used instead. However, this needs testing.

The attached patches update guzzlehttp/psr7 for all branches and switch laminas/laminas-diactoros for the fork in Drupal 9.

cspell should know better.

@longwave, thanks for taking on the fork!

I tried to do a quick fix by requiring your package and the results were this:

It looked promising because it started with:

$ composer require longwave/laminas-diactoros
./composer.json has been updated
Running composer update longwave/laminas-diactoros
Gathering patches from patch file.
> DrupalProject\composer\ScriptHandler::checkComposerVersion
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 1 removal
  - Removing laminas/laminas-diactoros (2.14.0)
  - Locking longwave/laminas-diactoros (2.14.1)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 1 removal
  - Removing laminas/laminas-diactoros (2.14.0)
Gathering patches from patch file.
Gathering patches for dependencies. This might take a minute.
  - Installing longwave/laminas-diactoros (2.14.1): Extracting archive
...

But then when I tried to update the security advisories it said:

$ composer update roave/security-advisories
❯ composer update roave/security-advisories
Gathering patches from patch file.
> DrupalProject\composer\ScriptHandler::checkComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - longwave/laminas-diactoros is locked to version 2.14.1 and an update of this package was not requested.
    - roave/security-advisories dev-latest conflicts with laminas/laminas-diactoros <2.18.1|>=2.24,<2.24.2|>=2.25,<2.25.2|= 2.23.0|= 2.22.0|= 2.21.0|= 2.20.0|= 2.19.0 (longwave/laminas-diactoros 2.14.1 replaces laminas/laminas-diactoros ^2.14.0).
    - Root composer.json requires roave/security-advisories dev-latest -> satisfiable by roave/security-advisories[dev-latest].

Keep in mind I may not know enough of composer to understand, but I think it's telling us that the version of your fork need to be >= 2.18.1?

FYI this doesn't pass for our local builds that require-dev with roave/security-advisories:

  Problem 1
    - Root composer.json requires ***/houston *@dev -> satisfiable by ***/houston[dev-884bab45f58af3bd535ff881ca749fbb1f8d1156].
    - roave/security-advisories dev-latest conflicts with laminas/laminas-diactoros <2.18.1|>=2.24,<2.24.2|>=2.25,<2.25.2|= 2.23.0|= 2.22.0|= 2.21.0|= 2.20.0|= 2.19.0 (longwave/laminas-diactoros 2.14.1 replaces laminas/laminas-diactoros ^2.14.0).

I haven't fully traced it out but as far as I can tell composer checks the "provides" against the "conflicts" as such since "longwave/laminas-diactoros" providing "laminas/laminas-diactoros ^2.14.0 conflicts with roave/security-advisories.

https://github.com/composer/composer/blob/e0c1ad14480e217e10462f1575af93...

I've seen some argument that replaces should be a single version not a constraint however that is probably moot here since longwave/laminas-diactoros should not claim to provide anything more than 2.14 which would still trigger a conflict.

This might need to be something that sites accept that they can't use roave/security-advisories unless they fork since composer does not support "conditional" conflicts.

The only functional differences between Diactoros 2.14 and 2.18 that I can see are adoption of PHP 8 features and breaking compatibility with PHP 7.3/7.4: https://github.com/laminas/laminas-diactoros/compare/2.14.0...2.18.0

If someone can confirm this, I will tag and release longwave/laminas-diactoros 2.18.1 including the security fix but without any of the language changes - ie. exactly the same as 2.14.1, but I should probably also fix the "replaces" section of composer.json as detailed by @cmlara. This will provide a version that satisfies roave/security-advisories but that is still compatible with PHP 7.3 and 7.4.

Yeah, the conversion from get_class() to ::class in https://github.com/laminas/laminas-diactoros/compare/2.14.0...2.18.0 will break PHP 7.3/7.4.

@longwave
I think these are php 8 compatibility changes (+ php version requirements) as you mentioned (link to changed files)
https://github.com/laminas/laminas-diactoros/compare/2.14.0...2.18.0#fil...
So we don't need these changes (to keep php 7.3/7.4 compatibility)

Would tagging a release of longwave/laminas-diactoros at 2.18.1 or 2.18.2 (with the code that's currently in the 2.14.0 tag) work to avoid the roave/security-advisories dependency issue? Even though the code is not exactly the same as the upstream, I think it might pass muster?

Another thought, if we are forking anyway, I think perhaps it could be a complete replacement of the package (completely remove laminas/laminas-diactoros and add longwave/laminas-diactoros) instead of trying to bring in our fork in as a replacement of laminas/laminas-diactoros? Or would that cause a lot of refactoring? Perhaps there's a psr4 namespacing trick to alias the namespace to avoid the wholesale refactor?

I'm just kinda throwing things out there, maybe my random ideas can trigger some other path to a solution. I appreciate all the hard work trying to find a solution.

Another thought, if we are forking anyway, I think perhaps it could be a complete replacement of the package (completely remove laminas/laminas-diactoros and add longwave/laminas-diactoros) instead of trying to bring in our fork in as a replacement of laminas/laminas-diactoros?

I believe this is what the current patch is essentially doing without having to update any of the namespace/class usages in core.

A temporary workaround that seems to have worked in a quick test is to run composer require laminas/laminas-diactoros:"2.14.0 as 2.18.1" for your project which satisfies roave/security-advisories while keeping the code at the version Drupal currently requires. But obviously doesn't fix the security issues with the laminas/laminas-diactoros package.

I should probably also fix the "replaces" section of composer.json as detailed by

Would tagging a release of longwave/laminas-diactoros at 2.18.1 or 2.18.2 (with the code that's currently in the 2.14.0 tag) work to avoid the roave/security-advisories dependency issue?

I believe (though I haven't fully traced) its only the replace line that is triggering roave, if the replace line were "2.18.1" I believe it wouldn't matter what the version of longwave/laminas-diactoros is, it could I believe be even 1.0.0 and not pose an issue with the constraints.

I haven't looked over the full code commits from #13 however if the statements from #11 are accurate it sounds like it should have minimal negative impact claiming to replace 2.18.1. If there had been new features or anything along those lines it would of been a potential problem.

The biggest risk I can think of would be if some contrib package or one of its dependencies has explicitly said they don't support 2.18 unless its on PHP8, for example they maintain a release that is "PHP7.4 && laminas-diactoros < 2.15" and one that is "PHP8 && laminas-diactoros >= 2.15" Hopefully this doesn't exist anywhere however its worth at least acknowledgment that it is a risk of increasing the replace to 2.18.1 as the code 'diverges'.

The new 'composer audit' thankfully does not suffer from this issue and allows auditing as part of a CI system, though it obviously doesn't prevent composer from installing insecure versions and depends on the audit preventing deployment. Faults for roave like packages where sites need to locally work around being prevented from installing are not unheard of however since its a limitation of how they implement the protection.

I think we're realizing that it might be smarter as D9 continues in the next couple of months to rely on drupal/core-recommended less often. I feel like we're more likely to keep running into this issue with old versions that do not get security updates anymore, than backwards-incompatibility blockers in minor version upgrades.

@cmlara regarding "replaces 2.18.1" I believe you are correct. So in the interests of not pretending to be anything that we're not, starting from 2.14.1 I have:

• extended the PHP version constraint to include PHP 8.2 and confirmed that all the existing tests pass
• switched the "replace" constraint to 2.18.1
• tagged this as 2.14.2

I chose 2.14.2 as this is nothing more than a fixed version of Diactoros 2.14, that includes the security fix and that works on PHP 7.3, 7.4 and 8.2.

I have further confirmed that installing roave/security-advisories with longwave/laminas-diactoros 2.14.2, but it would be great to get confirmation from the community that this is working as expected.

The attached patches upgrade Drupal 9.4/9.5 to use longwave/laminas-diactoros 2.14.2.

As it is, this issue is no longer about updating, so changing the title accordingly.

I think we're realizing that it might be smarter as D9 continues in the next couple of months to rely on drupal/core-recommended less often. I feel like we're more likely to keep running into this issue with old versions that do not get security updates anymore, than backwards-incompatibility blockers in minor version upgrades.

True, especially for sites that are getting ready for for D10. The problems now with core-recommended:
1) Drupal 9.5 still supporting an ancient version of PHP, which is a pain from the maintainer point of view only.
2) Not having a constraint that also allows the minor version that is used by or would have been used by the next major version of core is a pity. If the core-recommended constraint was "~2.14 || ~2.18" then a lot of sites on core-recommended would have been able to update already. There are of course better examples to reason about then laminas/diactoros as this particular one is not in the next major version of core anymore.
I'm sure this is discussed elsewhere, but don't no where right now.

@Eric_A we have an issue for loosening the constraint for Laminas at 📌 Remove laminas-feed, laminas-escaper, and laminas-stdlib from drupal/core-recommended to allow Drupal 9.5 to be installed on PHP 8.2 Fixed which pre-dates the security release here, obviously would have been great if we'd done that already. I guess on that issue we'll need to make sure it's possible to switch between the fork and the newer Laminas releases, although with the fork supporting PHP 8.2 it also becomes a bit less of an issue anyway.

📌 Drupal 9 uses PHP syntax that's deprecated in PHP 8.2, so exclude that from error_reporting() and DeprecationListenerTrait Fixed now has a patch that makes Drupal 9.5 tests pass on PHP 8.2 (by silencing E_DEPRECATED errors). If we do that and fork all of the Laminas dependencies (not just diactoros) to versions that support PHP 7.3 through PHP 8.2, then drupal/core-recommended:9.5 will also become compatible with PHP 8.2. Is it worth doing that as part of this issue, or should we constrain this issue to only the libraries with security vulnerabilities? One reason to do them together is if we think this is disruptive at all, then would it be better to do the full disruption in a single patch release rather than doing some libraries in one patch release and more in a later one?

@effulgentsia RE #22, while I think that solution is proactive, it's solving a problem we don't have yet. I'd err on the side of YAGNI, this problem arises from a security release, we have no crystal ball on the likelihood of other Laminas packages getting into the same situation.

So for me, I'd recommend keeping the scope narrow.

It would be good to get this issue done in time for next week's patch release window, and I think we still need to move the diactoros fork under the drupal namespace if possible. Given that I think we should stick to Diactoros here, but could maybe repurpose the other issue to fork instead of widen the constraint for feed?

Makes sense. I updated the title and summary of 📌 Remove laminas-feed, laminas-escaper, and laminas-stdlib from drupal/core-recommended to allow Drupal 9.5 to be installed on PHP 8.2 Fixed accordingly.

I think it would be great if we can get 📌 Remove laminas-feed, laminas-escaper, and laminas-stdlib from drupal/core-recommended to allow Drupal 9.5 to be installed on PHP 8.2 Fixed (whichever option we pick) into next week's patch release as well, if possible. We're already going to need release notes, etc. to explain the diactoros fork, and I think it would be better for site owners to have to only process that information once, instead of one patch release for diactoros and another one for the rest of Laminas.

Can we decouple the update of guzzlehttp/psr7, which seems to be the low hanging fruit here?

For laminas/* we seems to have two different opinions about scoping:

1. Fork All The Things That Start With laminas/* to be prepared for "The Future".
2. YAGNI: Fork only laminas/laminas-diactoros, since that is fixing a current CVE.

I'm a fan of #2, but that's not really important, as @effulgentsia stated, we need to get moving for the next patch release:

I think it would be great if we can get [snipped] (whichever option we pick) into next week's patch release as well, if possible.

Agree with splitting the Guzzle update into its own issue, when I suggested combining them I had no idea that it was going to require forking the Laminas codebase.

After a quick Slack chat with @longwave, I've split updating guzzlehttp/psr7 into a separate issue to prevent this being held up on the laminas disussion in here.

For laminas/* we seems to have two different opinions about scoping...I'm a fan of #2

I also agree with keeping this issue scoped to only laminas/diactoros. I hope that in addition to getting this issue done, that we can also get 📌 Remove laminas-feed, laminas-escaper, and laminas-stdlib from drupal/core-recommended to allow Drupal 9.5 to be installed on PHP 8.2 Fixed done before the next patch release, but I don't think that issue should block this issue, since this issue is Critical and that one isn't.

Back to NW since the guzzlehttp/psr7 update was handled in 📌 Update guzzlehttp/psr7 Fixed

Attached patch for 9.5.x, based on 356283-18-9.5.x.patch (without guzzlehttp/psr7 update)

Attached patch for 9.4.x, based on 356283-18-9.4.x.patch (without guzzlehttp/psr7 update)

The last submitted patch, 31: 3356283-31-9.5.x.patch, failed testing. View results →

The fail in #31 seems perhaps a random fail (although it's suspicious):

There was 1 failure:

1) Drupal\Tests\Core\Theme\ClassyPreprocessUnchangedTest::testNoNewPreprocess
The file hash for classy.theme has changed. Any additions or changes to preprocess functions should be added to the themes that inherit Classy. 
If the changes to classy.theme are not changes to preprocess functions, update the hash in this test to: '1a5f162bc900c45957aaa89959bcb607' so it will pass.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'1a5f162bc900c45957aaa89959bcb607'
+'c42ff3a1291a258b42f0c44010cd28c7'

This patch doesn't touch classy.theme at all. Not sure why/how that hash would be changing due to this.

Running Drupal\Tests\Core\Theme\ClassyPreprocessUnchangedTest::testNoNewPreprocess() locally with a clean 9.5.x test site is also currently failing (as of commit b05f3fa77589f). It was broken by commit 34f609005 from 📌 Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard Fixed . I'll comment there...

Meanwhile, I #31 doesn't apply to 10.1.x and I'm 95% sure we need to commit this there, first, then backport to earlier branches (if I've been understanding this cluster of related issues correctly).

@dww
in 10.x we don't have laminas/* packages. In this issue for 10.0.x, 10.1.x branches only the guzzlehttp/psr7 was updated, what was already committed in issue #3357247: Update guzzlehttp/psr7

So I think we are good with patches #31, #32 only for 9.4.x/9.5.x branches.

Ahh, missed that "detail". 😂 Thanks!

@longwave welcome to the dictionary!

The changes in #31 and #32 are the minimum changeset to move us from laminas to longwave. Thanks!

Committed/pushed to 9.5.x and 9.4.x, thanks!

Are there any steps to get require longwave/laminas-diactoros to actually be installed. Last week I upgraded from 9.4 to 9.5.8. Today I upgraded to 9.5.9 and I get this

PHP Warning: require(/var/application/vendor/composer/../laminas/laminas-diactoros/src/functions/create_uploaded_file.php): failed to open stream: No such file or directory in /var/application/vendor/composer/autoload_real.php on line 82

My composer.json has
"longwave/laminas-diactoros": "^2.14" in the require section
same in the composer.lock
"longwave/laminas-diactoros": "^2.14",

@StevenPatz which method did you use to install and update Drupal - did you download the .tar.gz or use Composer?

You might need to rm -rf your vendor directory and then composer install again.

I use composer. Everything looks like it's updating but it's not. I'll try the rm -rf in the morning.

I think I need a bit more assistance here.

I've tried removing vendor and composer install. That seems to be okay until I run drush and get errors for that. Then I try just removing vendor/longwave. but then it's back to PHP Warning: require(/var/application/vendor/composer/../laminas/laminas-diactoros/src/functions/create_uploaded_file.php): failed to open stream: No such file or directory in /var/application/vendor/composer/autoload_real.php on line 82

Is there a subset of vendor I can remove so I can get this working?

That seems to be okay until I run drush and get errors for that.

Can you post (a stack-trace of) that drush error?

PHP Fatal error:  Uncaught Exception: Could not locate autoload.php. cwd is /var/application/docroot; __DIR__ is /var/application/vendor/drush/drush in /var/application/vendor/drush/drush/drush.php:58
Stack trace:
#0 /var/application/vendor/drush/drush/drush(4): require()
#1 {main}
  thrown in /var/application/vendor/drush/drush/drush.php on line 58

Good morning,

After updating my project 9.5.8 to 9.5.9 (composer update "drupal/core-*" --with-all-dependencies), I can no longer run a drush command. I have the following error:

PHP Fatal error:  Cannot redeclare Laminas\Diactoros\createUploadedFile() (previously declared in /usr/local/src/drush/vendor/laminas/laminas-diactoros/src/functions/create_uploaded_file.php:14) in /var/www/html/unc/vendor/longwave/laminas-diactoros/src/functions/create_uploaded_file.php on line 16

PHP Version : 7.4.33
Drush version : 11

For people who have problem with drush, try requiring drush into your Drupal project, especially when you are working with a composer project. You won't have dependency conflicts between drush and drupal then. It's unfortunate to introduce those kind of conflicts in a patch release, but we didn't really have much of a choice. Just run `composer require drush/drush`, drush should then automatically use the built-in drush version.

I have to say I'm surprised that not doing so even still worked at all, I thought drush pretty much requires having it integrated for years now.

FYI I had been using drush/drush in composer

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

#49 I am stuck with same issue, how did you solve?

//first time on chat

Neither of patches above applies for D9.5.8, any idea of solution?

Solution is to upgrade to D9.5.10

D9.5.8 have partly updated some of the laminas repos so therefore patches aboce does not work, the do partly, but not all the way.
D9.5.10 Works perfectly.

For me, the composer update ran successfully after I completely deleted the contents of the vendor folder once after reading this link: https://www.jeffgeerling.com/blog/2020/watch-out-if-composer-update-keep...