Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Zend_Log: catch TypeError/ValueError for PHP 8.0 #88

Closed
wants to merge 4 commits into from
Closed

Zend_Log: catch TypeError/ValueError for PHP 8.0 #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00830e8
Short-circuit for php 8.0 testing only
glensc Mar 15, 2021
ebdc301
Catch ValueError from fopen()
glensc Mar 15, 2021
b84f97f
Catch also ValueError
glensc Mar 15, 2021
4d9eaa4
Catch TypeError in Zend_Log_Writer_Stream::_write
glensc Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,9 @@ jobs:
matrix:
experimental:
- false
php:
- "5.3"
- "5.4"
- "5.5"
- "5.6"
- "7.0"
- "7.1"
- "7.2"
- "7.3"
- "7.4"
# https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#example-including-additional-values-into-combinations
include:
- php: "8.0"
experimental: true
runs-on: ubuntu-20.04

env:
Expand Down
21 changes: 19 additions & 2 deletions packages/zend-log/library/Zend/Log/Writer/Stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,17 @@ public function __construct($streamOrUrl, $mode = null)
$streamOrUrl = $streamOrUrl['stream'];
}

if (! $this->_stream = @fopen($streamOrUrl, $mode, false)) {
try {
$this->_stream = @fopen($streamOrUrl, $mode, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, there's such test:

  • zf1/tests/Zend/Log/Writer/StreamTest.php

    Lines 49 to 60 in 8e1040b

    public function testConstructorThrowsWhenResourceIsNotStream()
    {
    $resource = xml_parser_create();
    try {
    new Zend_Log_Writer_Stream($resource);
    $this->fail();
    } catch (Exception $e) {
    $this->assertTrue($e instanceof Zend_Log_Exception);
    $this->assertRegExp('/not a stream/i', $e->getMessage());
    }
    xml_parser_free($resource);
    }

And this no longer returns a resource but a XmlParser class: $resource = xml_parser_create()

The test expected this code path to be executed:

but instead it ends up here, with open throwing with TypeError:

Failed asserting that 'fopen(): Argument #1 ($filename) must be of type string, XmlParser given' matches PCRE pattern "/not a stream/i".

@falkenhawk: how to proceed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep the most backward compatible way, would need to change code to reach also code path where is_resource is true, but I can't just add instanceof XmlParser there! :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current assertions are something like this:

/Users/glen/.local/bin/php80 /private/var/folders/_x/06xnfg5j58s_qvckwbk31x8w0000gn/T/ide-phpunit.php --configuration /Users/glen/scm/php/zf1/zf1s/phpunit.xml.dist --filter Zend_Log_Writer_StreamTest --test-suffix StreamTest.php /Users/glen/scm/php/zf1/zf1s/tests/Zend/Log/Writer
Testing started at 20:28 ...
PHPUnit 3.7.38 by Sebastian Bergmann.

Configuration read from /Users/glen/scm/php/zf1/zf1s/phpunit.xml.dist

F...F.FF...

Time: 76 ms, Memory: 4.00Mb

There were 4 failures:

1) Zend_Log_Writer_StreamTest::testConstructorThrowsWhenResourceIsNotStream
Failed asserting that 'fopen(): Argument #1 ($filename) must be of type string, XmlParser given' matches PCRE pattern "/not a stream/i".

/Users/glen/scm/php/zf1/zf1s/tests/Zend/Log/Writer/StreamTest.php:57
/Users/glen/scm/php/zf1/zf1s/vendor/zf1s/phpunit/composer/bin/phpunit:63

2) Zend_Log_Writer_StreamTest::testConstructorThrowsWhenStreamCannotBeOpened
Failed asserting that 'Path cannot be empty' matches PCRE pattern "/cannot be opened/i".

/Users/glen/scm/php/zf1/zf1s/tests/Zend/Log/Writer/StreamTest.php:92
/Users/glen/scm/php/zf1/zf1s/vendor/zf1s/phpunit/composer/bin/phpunit:63

3) Zend_Log_Writer_StreamTest::testWriteThrowsWhenStreamWriteFails
Failed asserting that 'fwrite(): supplied resource is not a valid stream resource' matches PCRE pattern "/unable to write/i".

/Users/glen/scm/php/zf1/zf1s/tests/Zend/Log/Writer/StreamTest.php:122
/Users/glen/scm/php/zf1/zf1s/vendor/zf1s/phpunit/composer/bin/phpunit:63

4) Zend_Log_Writer_StreamTest::testShutdownClosesStreamResource
Failed asserting that 'fwrite(): supplied resource is not a valid stream resource' matches PCRE pattern "/unable to write/i".

/Users/glen/scm/php/zf1/zf1s/tests/Zend/Log/Writer/StreamTest.php:138
/Users/glen/scm/php/zf1/zf1s/vendor/zf1s/phpunit/composer/bin/phpunit:63

FAILURES!
Tests: 11, Assertions: 14, Failures: 4.

Process finished with exit code 1

It's difficult to see these in CI, as unexpected errors from #85 shadow tests in #88 and vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @falkenhawk

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glensc perhaps transform this else

zf1/packages/zend-log/library/Zend/Log/Writer/Stream.php

Line 73 in 8e1040b

} else {

into e.g.
else if (is_string($streamOrUrl) || is_array($streamOrUrl)) {
and add additional else block at the end to catch the rest?

else {
  throw new Zend_Log_Exception('Resource is not a stream');
}

🤔

} catch (ValueError $e) {
// require_once 'Zend/Log/Exception.php';
throw new Zend_Log_Exception($e->getMessage(), $e->getCode());
} catch (TypeError $e) {
// require_once 'Zend/Log/Exception.php';
throw new Zend_Log_Exception($e->getMessage(), $e->getCode());
}

if (!$this->_stream) {
// require_once 'Zend/Log/Exception.php';
$msg = "\"$streamOrUrl\" cannot be opened with mode \"$mode\"";
throw new Zend_Log_Exception($msg);
Expand Down Expand Up @@ -130,7 +140,14 @@ protected function _write($event)
{
$line = $this->_formatter->format($event);

if (false === @fwrite($this->_stream, $line)) {
try {
$result = @fwrite($this->_stream, $line);
} catch (TypeError $e) {
// require_once 'Zend/Log/Exception.php';
throw new Zend_Log_Exception($e->getMessage(), $e->getCode());
}

if ($result === false) {
// require_once 'Zend/Log/Exception.php';
throw new Zend_Log_Exception("Unable to write to stream");
}
Expand Down