Problem/Motivation

PHP 8.0 is stricter about argument types and throws an error when fopen() is called with a $path === FALSE in \Drupal\Core\StreamWrapper\LocalStream::stream_open()

Steps to reproduce

Run tests on PHP 8.0 - see https://www.drupal.org/pift-ci-job/1855374

$ php7 -r 'var_dump(fopen(FALSE, "r"));'
...
Warning: fopen(): Filename cannot be empty in Command line code on line 1

$ php8 -r 'var_dump(fopen(FALSE, "r"));'
...
Fatal error: Uncaught ValueError: Path cannot be empty in Command line code on line 1

Proposed resolution

Get $path value and return early.

Remaining tasks

review/commit

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#27 3177541-7.x-reroll.patch770 bytesswim
#14 3177541-12.patch825 bytesalexpott
#14 8-12-interdiff.txt777 bytesalexpott
#8 3177541-8.patch756 bytesandypost
#8 interdiff.txt640 bytesandypost
#2 3177541-2.patch687 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
Issue tags: +PHP 8.0
FileSize
3177541-2.patch687 bytes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Would be great to add inline comment otherwise rtbc+1

alexpott’s picture

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

@andypost this really does not need an inline comment - how can fopen open a path that is FALSE?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes

updated IS with steps to reproduce - $filename is required now

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes

Better example

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes

fix php8 cli

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt640 bytes
3177541-8.patch756 bytes

@alexpott kind of this coment

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

The only case documented is \Drupal\Core\StreamWrapper\LocalStream::getLocalPath()'s return value

If $uri is set but not valid, returns FALSE.

Btw BC affected with current patch - before it STREAM_REPORT_ERRORS allowed to catch error to calling code and react, now just FALSE returned

alexpott’s picture

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

@andypost that's a comment that will have to be removed once PHP 8 is the minimum supported version. I'd argue that this comment helps no one.

alexpott’s picture

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

Good point on BC. We can be inspired by \Drupal\Core\StreamWrapper\LocalReadOnlyStream::stream_open() and do the same.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
@@ -164,6 +164,7 @@ public function stream_open($uri, $mode, $options, &$opened_path) {
     if ($path === FALSE) {

is strict comparison, the question is why that instead of if (!$path) {

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

php -r 'var_dump(fopen("", "r"));' also fatal error now

PHP Fatal error:  Uncaught ValueError: Path cannot be empty in Command line code:1
Stack trace:
#0 Command line code(1): fopen()
#1 {main}
  thrown in Command line code on line 1

Fatal error: Uncaught ValueError: Path cannot be empty in Command line code on line 1

ValueError: Path cannot be empty in Command line code on line 1

Call Stack:
    0.0001     344528   1. {main}() Command line code:0
    0.0001     344528   2. fopen($filename = '', $mode = 'r') Command line code:1
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue tags: +Needs tests
FileSize
8-12-interdiff.txt777 bytes
3177541-12.patch825 bytes

Here we go. Removing comment added in #8 because it really will only have to be removed in Drupal 10 and we'll forgot to do that. And it serves no purpose.

I guess we now need to find a place to test this.

alexpott’s picture

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

Re #12 @andypost how does \Drupal\Core\StreamWrapper\LocalStream::getLocalPath() return an empty string?

alexpott’s picture

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

I looked for test coverage of the local stream wrapper but there is only implicit test coverage - because it is used everywhere :) - we know the implicit coverage exists because the PHP 8 test run failed because of this.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I think no tests needed too, but curious about hiding error - should we add follow-up or not?

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
@@ -163,6 +163,12 @@ protected function getLocalPath($uri = NULL) {
+      if ($options & STREAM_REPORT_ERRORS) {
+        trigger_error('stream_open() filename cannot be empty', E_USER_WARNING);
...
     $this->handle = ($options & STREAM_REPORT_ERRORS) ? fopen($path, $mode) : @fopen($path, $mode);

I think "report errors" should not prevent fatal errors on PHP 8 when it become minimally required, could use todo and follow-up

alexpott’s picture

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

@andypost well this is maintaining the current behaviour of returning FALSE in this case. Which we're relying on in FileSystemRequirementsTest as per the test run.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: -Needs tests

Filed follow-up #3178994: Allow fatal error on PHP 8.0 in LocalStream::stream_open() for fopen()

is code duplication ($options & STREAM_REPORT_ERRORS) fine here?

alexpott’s picture

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

It's not really code duplication - since we have to check this in both places to maintain current behaviour regardless of PHP version.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Then it ready for commiters! Thank you

  • catch committed fc7d4a5 on 9.2.x 
    Issue #3177541 by alexpott, andypost: stream_open() needs to cope with a...

  • catch committed f762f07 on 9.1.x 
    Issue #3177541 by alexpott, andypost: stream_open() needs to cope with a...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

I think this is good for PHP 8 compatibility and thanks for opening the Drupal 10 follow-up.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x.

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@alexpott follow-up is ready for review #3178994-14: Allow fatal error on PHP 8.0 in LocalStream::stream_open() for fopen()

swim’s picture

swim CreditAttribution: swim commented
FileSize
3177541-7.x-reroll.patch770 bytes

For those lucky enough to still be dealing with 7; here's a reroll against 7.x

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue tags: +panopoly
cafuego’s picture

cafuego CreditAttribution: cafuego at United Nations commented
Version: 9.1.x-dev » 7.x-dev

That reroll patch, when applied, seems to turn a logged PHP error into a segmentation fault without further logs :-)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 7.x-dev » 9.1.x-dev

@cafuego tbis issue for 9.x the 7.x one is #3273723: fopen() error in stream_wrappers.inc on PHP 8