Problem/Motivation

The recent 7.67 security update added a dependency on PHP's fileinfo extension. Drupal core is shipped with https://github.com/TYPO3/phar-stream-wrapper library. It should be updated.

  • Drop requirement for fileinfo extension
  • Support PHP 7.4

See upstream fixes https://github.com/TYPO3/phar-stream-wrapper/pull/33 and https://github.com/TYPO3/phar-stream-wrapper/issues/49

Proposed resolution

Update typo3/phar-stream-wrapper library version to v2.1.3

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

typo3/phar-stream-wrapper library version to v2.1.3

CommentFileSizeAuthor
#20 3085098-20.patch8.33 KBlongwave
#6 3085098-phar-stream-wrapper-2.1.3.patch10.49 KBlongwave
#2 drupal-typo3_phar_php74_workaround-3085098-2.patch1013 bytestaran2l

Comments

Taran2L created an issue. See original summary.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
StatusFileSize
new drupal-typo3_phar_php74_workaround-3085098-2.patch1013 bytes

Temporary patch attached (so we can proceed with testing)

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented

I have created a Merge Request in Github for this change in the upstream repo. Once this has been committed, we could update the library in Drupal 7.

https://github.com/TYPO3/phar-stream-wrapper/pull/51

dinesh18’s picture

dinesh18 commented
Status: Active » Reviewed & tested by the community

#2 looks good to me. +1 to RTBC

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Reviewed & tested by the community » Needs work

@Dinesh18, provided patch should not be committed as is, it's more proof of concept

@sjerdo, your PR on Github has been accepted, and the new version has been released - https://github.com/TYPO3/phar-stream-wrapper/releases/tag/v2.1.3

So, a new patch should be provided containing the new official release of the library

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3085098-phar-stream-wrapper-2.1.3.patch10.49 KB

Attached patch updates the stream wrapper library to v2.1.3.

dinesh18’s picture

dinesh18 commented

@Taran2L https://www.drupal.org/project/drupal/issues/3094955 I created a patch here. Later I come to know the ticket is duplicate so I thought of reviewing the patch it looks proper so I put RTBC.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented

Patch #6 contains the same diff as upstream (excluding tests and .travis.yml file): https://github.com/TYPO3/phar-stream-wrapper/compare/v2.1.1...v2.1.3
I guess we don't need to include the .appveyor.yml file, just like the .travis.yml file which isn't included. We should exclude the .appveyor.yml file in the patch.

Triggered some additional tests for the patch for other PHP versions (PHP 5.3 + PHP 7.4-dev)

@Dinesh18 since this is a third party library the preferred solution is to fix this upstream. Just like we did in this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@sjerdo: I did debate whether to include .appveyor.yml but it is included in the release tarball at https://github.com/TYPO3/phar-stream-wrapper/archive/v2.1.3.tar.gz which is what I used to create the patch. I guess we should raise a PR on their repo to ignore .appveyor.yml in the release via .gitattributes, then it will be excluded like the tests and Travis config.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented

+1 to @longwave - I think unmodified original release should be included.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented
Status: Needs review » Reviewed & tested by the community

RTBC +1
Unmodified original release seems valid to me.
Tests pre PHP 7.4 all good

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I raised https://github.com/TYPO3/phar-stream-wrapper/pull/56 to remove the Appveyor file and it looks like there will be a new release soon, so we might want to wait for that?

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Reviewed & tested by the community » Postponed

Indeed, I think the new release is coming soon. Let's wait for it ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: PHP Phar Stream Wrapper should be updated to fully support PHP7.4 » PHP Phar Stream Wrapper should be updated (PHP7.4 support, drop fileinfo dependency)
Priority: Normal » Critical
Issue summary: View changes
Status: Postponed » Needs review

I closed #3054615: Bump typo3/phar-stream-wrapper library version to v2.1.2 to remove fileinfo extension dependency in favour of this. So bumping this to critical as this also solves that issue.

mcdruid’s picture

mcdruid commented
Issue tags: +Drupal 7 bugfix target
taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented

Both PRs have been merged: https://github.com/TYPO3/phar-stream-wrapper/pull/56 and https://github.com/TYPO3/phar-stream-wrapper/pull/57

Waiting for the new release ...

jthomas_elavon’s picture

jthomas_elavon commented

This patch has been merged in upstream.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

It is committed upstream but we are still waiting for a release; the "2 commits to v2 since v2.1.3" are what we are waiting for: https://github.com/TYPO3/phar-stream-wrapper/releases/tag/v2.1.3

edit: reached out to @ohader on GitHub via email to see if a new release can be published

oliver.hader’s picture

oliver.hader commented

https://github.com/TYPO3/phar-stream-wrapper/releases

Thanks @longwave for your message - v2.1.4 and v3.1.4 of typo3/phar-stream-wrapper have been tagged now

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3085098-20.patch8.33 KB

@oliver.hader: Thanks for the super fast response!

Attached patch upgrades typo3/phar-stream-wrapper to v2.1.4 which now excludes .appveyor.yml and .gitignore.

bburg’s picture

bburg
he/him
Location Washington, DC
commented

I added the PHP 7.4 test. Though it seems to fail for other reasons.

LAguilar’s picture

LAguilar commented

#20 Works for my

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Needs review » Reviewed & tested by the community

I would say it's RTBC

stefanos.petrakis’s picture

stefanos.petrakis
Primary language English
Location Biel, Switzerland
commented
Status: Reviewed & tested by the community » Postponed

The status should be "Postponed" if anything, till the tests come out green.

Which means letting the tests run again after at least #3084953: Trying to access array offset on value of type null in _drupal_build_css_path() is committed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed » Reviewed & tested by the community

This can be committed now, we don't have to wait for full PHP 7.4 compatibility.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

PHP 7.4 & MySQL 5.7 is down to 3 fails, all covered by other tickets.

mcdruid’s picture

mcdruid commented
Issue tags: +Pending Drupal 7 commit

Assuming (re)tests look okay, I think this is ready.

mcdruid’s picture

mcdruid commented

Tests do look okay; just the expected 3 remaining failures for PHP 7.4.

fabianx’s picture

fabianx commented

Approved, RTBM - let's get this in.

Thanks all!

fabianx’s picture

fabianx commented
Assigned: Unassigned » mcdruid

  • mcdruid committed b79f40a on 7.x 
    Issue #3085098 by longwave, Taran2L, sjerdo, Dinesh18, oliver.hader: PHP...
mcdruid’s picture

mcdruid commented
Assigned: mcdruid » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Drupal 7 bugfix target, -Pending Drupal 7 commit

Thanks everyone!

Status: Fixed » Closed (fixed)

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

andylarks’s picture

andylarks commented

Slightly nervous about questioning the Drupal gods (you guys are all awesome), but on my D7.70, I just got this having moved my site to a new server:

Deprecated: Array and string offset access syntax with curly braces is deprecated in sensible_path/misc/typo3/phar-stream-wrapper/src/PharStreamWrapper.php on line 479

...which looks a lot like this bug?

Server: Ubuntu 20.4
PHP: 7.4 php-fpm
Apache2: Apache/2.4.41

Pretty sure I must be doing/have done something stupid to be here?

mcdruid’s picture

mcdruid commented

Here's the commit that fixed that:

https://git.drupalcode.org/project/drupal/-/commit/b79f40a8c6795aa51a81b...

...but it's not in 7.70 (which was a security release):

https://git.drupalcode.org/project/drupal/-/blob/7.70/misc/typo3/phar-st...

It's in 7.71 (which was a bugfix release so included everything that's been committed to 7.x):

https://git.drupalcode.org/project/drupal/-/blob/7.71/misc/typo3/phar-st...

So you'll need to update to 7.71 to get the fix.

Security releases only include security fixes and are otherwise based on the previous release/tag, so they won't include bug fix commits that have not yet been included in a release, which is what has caught you out here :)

andylarks’s picture

andylarks commented

Ah!

Awesome, thanks @mcdruid - I should have spotted that :(

A.