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.

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

Zend_Log: catch TypeError/ValueError for PHP 8.0 #88

Closed
wants to merge 4 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Mar 15, 2021

Similar to #85, catch TypeError/ValueError being thrown from PHP 8.0 from invalid arguments and convert to appropriate Zend_Exception.

refs:

PHP Fatal error:  Uncaught TypeError: fopen(): Argument zf1s#1 ($filename) must be of type string, XMLParser given
FPHP Fatal error:  Uncaught ValueError: Path cannot be empty
Configuration read from /Users/glen/scm/php/zf1/zf1s/phpunit.xml.dist
@@ -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:

  • 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.

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


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');
}

🤔

@glensc glensc mentioned this pull request Mar 16, 2021
32 tasks
@falkenhawk
Copy link
Member

superseded by #107

@falkenhawk falkenhawk closed this Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants