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

Small fixes for tests in favor of PHP 8.0 compat #77

Closed
wants to merge 5 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Mar 13, 2021

Extracted changes made by @Megatherium from #32

@glensc
Copy link
Contributor Author

glensc commented Mar 13, 2021

The failing test for php 5.4:

PHP Catchable fatal error:  Argument 1 passed to Zend_Log_Writer_Abstract::setFormatter() must implement interface Zend_Log_Formatter_Interface, instance of stdClass given, called in /home/runner/work/zf1/zf1/tests/Zend/Log/Writer/AbstractTest.php on line 68 and defined in /home/runner/work/zf1/zf1/packages/zend-log/library/Zend/Log/Writer/Abstract.php on line 97
...................
Error: Process completed with exit code 1.

is either flaky or the change to resolve this is in some other PR.

@glensc
Copy link
Contributor Author

glensc commented Mar 15, 2021

The logFormatter was also discussed here:

Alexander Wozniak and others added 5 commits March 15, 2021 17:00
ZF-11344 only affected PHP7.0 and was therefore reenabled for all other
versions.
In a lot of cases with PHP8 Type and ValueErrors are thrown instead of
warnings emitted. The error handling in tests needed to be modifed
accordingly.
Where in PHP <8 we would expect exceptions PHP8 now throws errors.
Luckily we can just add extra catch statements without ruining the
compatibility to older versions.
Tests for PHP 8.0 are now also run via Github Actions and failure for
8.0 has been disallowed on Travis.
if (PHP_VERSION_ID >= 50400) {
if (PHP_VERSION_ID >= 70000 && PHP_VERSION_ID < 70100) {
Copy link
Member

@falkenhawk falkenhawk Mar 15, 2021

Choose a reason for hiding this comment

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

is there any chance that changes in this file could affect the failing Logger test on PHP 5.4? 🙈

} catch (Throwable $e) {
$this->assertTrue($e instanceof ValueError,
'Expecting object of type Zend_Db_Statement_Exception, got '.get_class($e));
$this->assertRegExp('#must be a bitmask#i', $e->getMessage());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and other Throwable, ValueError fixes to tests are wrong in this PR. Code must be adjusted to keep previous behavior, even if it's wrong behavior.

For example, outer code expects Zend_Db_Statement_Exception to be thrown, but instead, ValueError is thrown, and the whole application breaks. Even old PHPUnit breaks because \Error is not a subclass of \Exception.

My version of the problem fix:

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

falkenhawk commented Oct 1, 2021

These adjustments are no longer necessary. (covered by other PRs) Thanks!

@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