Problem/Motivation

#3039611: Update core PHP dependencies for 8.8.x updated Diactoros from 1.4 to 1.8. I don't think this is the correct choice as security coverage for 1.8 ended a couple weeks ago on Sept. 27, but 1.7 is an LTS version that will be supported into 2022. See: https://framework.zend.com/long-term-support

Proposed resolution

Pin Diactoros to 1.7 so we stay on the LTS until D8's EOL.

Remaining tasks

Patch please.

Release notes snippet

Diactoros has been updated from 1.4.1 to 1.8.7 (an LTS version which receives security coverage until March 2022).

CommentFileSizeAuthor
#29 3087531-8.8.x-28.patch3.93 KBalexpott
#29 3087531-9.0.x-28.patch3.93 KBalexpott
#29 3087531-8.9.x-28.patch3.93 KBalexpott
#15 interdiff-26c3f3.txt2.99 KBjibran
#14 Screenshot from 2019-10-12 22-54-46.png135.73 KBgnikolovski
#13 3087531-13.patch835 bytesjibran
#4 3087531-4.patch5.73 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Assigned: Unassigned » jibran

Working on a patch now.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Active » Needs review
FileSize
3087531-4.patch5.73 KB 
+------------------------------+-------+-------+
| Production Changes           | From  | To    |
+------------------------------+-------+-------+
| zendframework/zend-diactoros | 1.8.7 | 1.7.2 |
+------------------------------+-------+-------+
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/composer.json
@@ -43,7 +43,7 @@
-        "zendframework/zend-diactoros": "^1.1",
+        "zendframework/zend-diactoros": ">=1.7 <1.8",

👍 Looks good, thanks.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/composer.json
@@ -5,8 +5,8 @@
-        "wikimedia/composer-merge-plugin": "^1.4",
-        "drupal/core": "self.version"
+        "drupal/core": "self.version",
+        "wikimedia/composer-merge-plugin": "^1.4"

Weird artifact?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

#6 package should be in sorted order. I can revert this if we don't want the change but it is bound to happen at some time.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Reviewed & tested by the community

Sure, that seems fair. I guess the patch that added it added it at the end of the list manually.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f123666e10 to 9.0.x and c7cae3d7ba to 8.9.x and 4a7a0fa0af to 8.8.x. Thanks!

  • alexpott committed f123666 on 9.0.x 
    Issue #3087531 by jibran, xjm: Use Diactoros LTS version 1.7, not 1.8...

  • alexpott committed c7cae3d on 8.9.x 
    Issue #3087531 by jibran, xjm: Use Diactoros LTS version 1.7, not 1.8...

  • alexpott committed 4a7a0fa on 8.8.x 
    Issue #3087531 by jibran, xjm: Use Diactoros LTS version 1.7, not 1.8...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Assigned: jibran » Unassigned
Status: Fixed » Needs review
FileSize
3087531-13.patch835 bytes

This is wired. For some reason lock file is outdated.

gnikolovski’s picture

gnikolovski
Primary language Serbian
Location Subotica, Serbia
CreditAttribution: gnikolovski as a volunteer and at Studio Present commented
FileSize
Screenshot from 2019-10-12 22-54-46.png135.73 KB

I just tried installing Drupal 8.8dev with the new drupal/recommended-project template and I'm getting this:

Your requirements could not be resolved to an installable set of packages.

Problem 1
- drupal/core 8.8.x-dev requires zendframework/zend-diactoros >=1.7 <1.8 -> satisfiable by zendframework/zend-diactoros[1.7.0, 1.7.1, 1.7.2].
- Can only install one of: zendframework/zend-diactoros[1.8.7, 1.7.0].
- Can only install one of: zendframework/zend-diactoros[1.8.7, 1.7.1].
- Can only install one of: zendframework/zend-diactoros[1.8.7, 1.7.2].
- drupal/core-recommended 8.8.x-dev requires drupal/core 8.8.x-dev -> satisfiable by drupal/core[8.8.x-dev].
- drupal/core-recommended 8.8.x-dev requires zendframework/zend-diactoros 1.8.7 -> satisfiable by zendframework/zend-diactoros[1.8.7].
- Installation request for drupal/core-recommended ^8.8 -> satisfiable by drupal/core-recommended[8.8.x-dev].

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
FileSize
interdiff-26c3f3.txt2.99 KB

Patch #13 should fix that but we wouldn't know until it is committed. Also #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage will add explict tests to make sure templates provided by core should be installable at all times.

@alexpott I was thinking about adding a test to make sure that drupal/core is always up to date in lock file. PFA the interdiff and let me know if it is worth adding to the PATCH.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Fixed

The out-of-date lock file got fixed in #3086796: Explicitly require pear/archive_tar ^1.4.5 - the 8.8.x lock file seems way harder to manager than before.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

In order to do #15 we need #3087626: Convert drupal/core-recommended & c. into a subtree split

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

I have updated the metapackages in the meantim, so #14 will work until we change the next lockfile

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Symfony Security Check is complaining now.

Symfony Security Check Report
=============================

1 packages have known vulnerabilities.

zendframework/zend-diactoros (1.7.2)
------------------------------------

 * [CVE-NONE-0001][]: URL Rewrite vulnerability

[CVE-NONE-0001]: https://framework.zend.com/security/advisory/ZF2018-01

Note that this checker can only detect vulnerabilities that are referenced in the SensioLabs security advisories database.
Execute this command regularly to check the newly discovered vulnerabilities.
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Uhhh now that is interesting; what's the point of LTS coverage if you don't backport security patches to it?

We already determined Drupal doesn't exercise that particular vulnerability when we released 2018-005, but I'm more concerned about the next one. Let's file an issue in their queue?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Like the entire reason we didn't try to replace Diactoros as a dependency was that they document support for LTS releases.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

So apparently their LTS policy was finalized just a few weeks before 2018-005 was published so it may not have been totally in place yet. Their GitHub issue queue prompts you to ask questions in their Slack instead: https://zendframework-slack.herokuapp.com/

Too late tonight for me; can someone find out in their Slack whether or not the LTS date documented on their policy is real and whether 1.7 will get any subsequent security backports in light of 2018-005 not having been backported?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I've opened an issue against zend-diactoros - see https://github.com/zendframework/zend-diactoros/issues/373 - can't create a PR because there's no 1.7 branch to create a PR against but here's a fork with the fix https://github.com/alexpott/zend-diactoros/tree/1.7.x-CVE-NONE-0001 - also in zend slack it looks like they might bump the LTS version to 1.8

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

For now I'll put this as a known issue in the release notes. If they do actually update their page documenting that 1.8 will be supported until 2022 instead of last month, we should probably bump the version back to 1.8 before the alpha. Otherwise before beta. Otherwise... I dunno, but downgrading minors is technically a BC break.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

Their documentation on https://framework.zend.com/long-term-support is fairly obtuse.

It seems like they dont actually have LTS for individual packages, only the '12 months after a major' policy, except they *DO* provide it for any packages that happen to be required by their application skeleton templates.

In this case it looks like they just updated their page incorrectly, applying the '12 month' policy to 1.8, but leaving the 'defacto LTS at 1.7 because Expressive skeleton needs it'

However, the expressive skeleton has a requirement of zendframework/zend-diactoros: ^1.7 which means that security fixes in 1.8 *do* support their framework.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

I tried to bump the issue https://github.com/zendframework/zend-diactoros/issues/373#issuecomment-....

Meanwhile should we create a new issue to update Diactoros version to ^2.1 for 9.0.x branch?

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Fixed » Needs work

Revert this ticket because 1.8 is the LTS version:
https://github.com/zendframework/zend-diactoros/issues/373#issuecomment-...

Version 1.8 is LTS version, not 1.7. The issue is in the table - we will try to update it shortly.
Please note that there is a paragraph how to adapt LTS version:

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs re-roll, +Novice

This isn't a clean revert so we could do with a new patch here to update from 1.7 to 1.8

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
Issue tags: -Needs re-roll, -Novice
FileSize
3087531-8.9.x-28.patch3.93 KB
3087531-9.0.x-28.patch3.93 KB
3087531-8.8.x-28.patch3.93 KB

Here's some patches...

The last submitted patch, 29: 3087531-8.9.x-28.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks good to me thanks @alexpott. Also updated the release notes.

  • catch committed e330cfd on 9.0.x 
    Issue #3087531 by alexpott, jibran, gnikolovski, xjm, Mixologic: Use...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

  • catch committed 5bf368e on 8.9.x 
    Issue #3087531 by alexpott, jibran, gnikolovski, xjm, Mixologic: Use...

  • catch committed 559b260 on 8.8.x 
    Issue #3087531 by alexpott, jibran, gnikolovski, xjm, Mixologic: Use...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Thanks for fixing this. Can someone update https://www.drupal.org/core/dependencies explaining the vagaries of their LTS policy?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I've done this for now, https://www.drupal.org/node/3051219/revisions/view/11615200/11618941 - we should update again once their table is correct though.

Status: Fixed » Closed (fixed)

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