PHP8.0 compatibility patch #32

Megatherium · 2020-11-19T09:58:06Z

Some of the more common things that needed be fixed were:

A lot of times where PHP used to emit a warning (or even nothing) we
now get Type- and ValueErrors which either need to be prevented by
intializing or casting variables or catching the error.
file_exists will error out if you feed it NULL bytes. This needed to
be circumvented
optional parameters need to come after required params, thus some
default values had to be removed from method signatures and workarounds
put in place
some reflection methods like isArray have become deprecated and are
only called conditionally going forward
libxml_disable_entity_loader has been deprecated and is only called
conditionally

glensc · 2020-11-19T10:14:41Z

x-ref:

iconv_substr behaves differently under PHP8 #31

glensc

Dropped some notes, In overall good work!

composer.json

packages/zend-barcode/library/Zend/Barcode/Renderer/Image.php

packages/zend-file/library/Zend/File/ClassFileLocator.php

packages/zend-filter/library/Zend/Filter/Compress/Bz2.php

packages/zend-filter/library/Zend/Filter/Compress/Gz.php

packages/zend-reflection/library/Zend/Reflection/Parameter.php

tests/Zend/CodeGenerator/Php/ParameterTest.php

tests/Zend/Date/DateObjectTest.php

packages/zend-locale/library/Zend/Locale/Math.php

Megatherium · 2020-11-30T12:21:55Z

Finally got around to reworking that history. Let's see how Travis feels about it :)

Megatherium · 2020-11-30T13:13:45Z

Everything checks out except for that one thing in Zend_Log_Writer_AbstractTest with PHP5.4 which also only occurs if you run the full suite. All three tests pass if you run that thing manually.

Megatherium · 2020-12-03T07:29:29Z

@falkenhawk Do you want me to put a guard in for that specific case so we can have pretty green arrows?

packages/zend-loader/library/Zend/Loader/ClassMapAutoloader.php

packages/zend-pdf/library/Zend/Pdf/Filter/RunLength.php

tests/Zend/CodeGenerator/Php/ParameterTest.php

falkenhawk · 2020-12-03T09:18:37Z

tests/Zend/Controller/Action/HelperBroker/PriorityStackTest.php

-        $iterator = $this->stack->getIterator();
-        $this->assertEquals('Zend_Controller_Action_Helper_Redirector', get_class(current($iterator)));
-        next($iterator);
-        $this->assertEquals('Zend_Controller_Action_Helper_ViewRenderer', get_class(current($iterator)));
+        $iterator = $this->stack->getIterator()->getIterator();
+        $this->assertEquals('Zend_Controller_Action_Helper_Redirector', get_class($iterator->current()));
+        $iterator->next();
+        $this->assertEquals('Zend_Controller_Action_Helper_ViewRenderer', get_class($iterator->current()));


does php8 throw on calling current() / next() on an iterator?

It retuns false and thus get_class throws a TypeError

tests/Zend/Date/DateObjectTest.php

falkenhawk · 2020-12-03T09:20:28Z

tests/Zend/Db/TestSetup.php

+            if (stristr($e->getMessage(), 'undefined constant')) {
+                $this->markTestSkipped();
+            }


In which case this the db factory throws this undefined constant error? What kind of constants were missing?

DB2 constants

tests/Zend/Filter/HtmlEntitiesTest.php

falkenhawk · 2020-12-03T09:22:27Z

tests/Zend/Form/ElementTest.php

        $options['filters'] = array(
-            array('Digits', array('bar' => 'baz')),
+            array('Alnum', array('allowWhiteSpace' => true)),
            array('Alpha', array('foo')),
        );
        $this->element->setOptions($options);
-        $filter = $this->element->getFilter('Digits');
-        $this->assertTrue($filter instanceof Zend_Filter_Digits);
+        $filter = $this->element->getFilter('Alnum');
+        $this->assertTrue($filter instanceof Zend_Filter_Alnum);


the behavior of this tests has changed. Could you comment on why, please?

The behaviour hasn't really changed because the intent of the test is to use an array of arrays in setOptions. What has changed is that PHP8 supports calling functions with named parameters and will throw a Fatal if you try to access a parameter by name that does not exist. Digits::__construct has no param named bar ergo we get a Fatal.

I could add a version switch but that wouldn't make the test any more meaningful.

tests/Zend/Validate/DateTest.php

falkenhawk · 2020-12-03T09:27:32Z

tests/Zend/Log/Writer/StreamTest.php

        } catch (Exception $e) {
            $this->assertTrue($e instanceof Zend_Log_Exception);
            $this->assertRegExp('/not a stream/i', $e->getMessage());
+        } catch (TypeError $e) {
+            $this->assertRegExp('/must be of t/i', $e->getMessage());


I am not sure about the adjustments here. Were these TypeError, ValueError triggered as regular errors in PHP <8 or were they simply non-errors before and now PHP 8 is more strict? If that's the case, is it okay that PHP8 will throw such errors instead of the php <8 shrug-and-proceed behavior?

Usually where I added Error catches the behaviour in <8 was to emit a warning or fail silently.

Okay I guess it is fine in case of this component - when something is wrong with internals of this stream thing, it should throw

falkenhawk · 2020-12-03T09:41:47Z

@falkenhawk Do you want me to put a guard in for that specific case so we can have pretty green arrows?

@Megatherium I will try to run php5.4 test suite locally to see if I can figure out why it fails

glensc · 2020-12-06T22:19:07Z

#35 created a separate issue, so it can be worked independently of this PR.

As this branch does not contain the major fixes for PHP8 ( cf. zf1s#32) the tests will abort with a fatal. Thus Travis has beeb allowed to fail for PHP8.

Megatherium · 2021-02-05T09:17:01Z

So, next step:

Rebase this to master, add 8.0 to Github actions, disallow failure for 8.0 on Travis.

Correct?

Megatherium · 2021-02-05T14:35:43Z

I don't really get why that test bails all of the sudden in 5.4. As far as I can tell I haven't touched any file involved in that test. Also that test is supposed to cause an error which PHPUnit should handle... I'm confused.

@Megatherium

mainly backported from zf1s/zf1#32 PR by @Megatherium

Because the return values of iconv_substr changed in PHP8 calling Zend_Date::set could lead to TypeError

glensc · 2021-02-18T07:37:37Z

@falkenhawk @Megatherium seems this is stalled again. Is there any interest, I could cherry-pick commits related from here to separate PRs? So the changeset could be merged individually, not depending on the "big approval".

I need confirmation, so I won't end up doing unwanted work.

falkenhawk · 2021-02-18T08:10:50Z

@glensc thanks for stepping up. I believe there is lots of interest, but I guess we all might be constrained by the regular job responsibilities or other stuff. As much as I would like to have it done, I can't merge it yet when there are tests failing. I would also love to jump in and help but I am afraid I can't do at the moment.

Is that failing test due to the issue with iconv the only thing left to be sorted out here? If you have an idea how to fix it then perhaps could you either rebase or merge from master and push a fix? 🙏

Please coordinate with @Megatherium if possible so that work is not doubled, perhaps Alex has some work not pushed yet.

Megatherium · 2021-02-18T11:43:49Z

The thing is I believe the test setup and not the code to be wrong. I know this is basically like claiming a compiler bug but iconv is an external resource and I didn't have my little grubby hands anywhere near the failing code.
Same with Zend_Queue_Queue1Test: if you run it or the whole Zend_Queue test battery you don't get any failures. Only if you run tests/Zend/AllTests.php do you see that error and that means (at least to me, although I might be lacking some insight here) that we have some kind weird side effects that have nothing to do with the framework code.

tl;dr: I currently don't know how to give all green check marks.

glensc · 2021-02-18T12:44:01Z

@falkenhawk @Megatherium I proposed to extract all changes that are ok to a separate PR. I would not be interested in fixing actual bugs right now. I would like to receive the clear Go Ahead signal. Or would you @Megatherium want to do that yourself?

falkenhawk · 2021-02-18T12:49:27Z

@glensc please do 🙏

Megatherium · 2021-02-19T12:33:23Z

@glensc Please do. Maybe by chopping this stuff up it'll help me to narrow down the source of the remaining problems.

glensc · 2021-03-13T16:20:01Z

Created 14 PR's as can see from the web:

PHP8.0 compatibility patch #32 (comment)

The last one seems to have issues, probably depends on the change that went to some other pr:

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

or it could be just a flaky test.

falkenhawk · 2021-03-15T12:33:13Z

Closing in favor of multitude of PRs from @glensc . Thank you both!

glensc mentioned this pull request Nov 19, 2020

PHP 8 support? Shardj/zf1-future#120

Closed

glensc suggested changes Nov 19, 2020

View reviewed changes

glensc reviewed Nov 19, 2020

View reviewed changes

packages/zend-locale/library/Zend/Locale/Math.php Show resolved Hide resolved

falkenhawk mentioned this pull request Nov 20, 2020

Release 1.13.4 notes #30

Closed

Megatherium force-pushed the php80 branch 2 times, most recently from 7692726 to 69bed7b Compare November 30, 2020 12:19

Megatherium force-pushed the php80 branch from 69bed7b to 7f45c18 Compare November 30, 2020 12:40

falkenhawk requested review from falkenhawk, marcing and partikus December 3, 2020 08:24

falkenhawk reviewed Dec 3, 2020

View reviewed changes

falkenhawk mentioned this pull request Dec 9, 2020

php 8: The track_errors ini directive has been removed #35

Closed

Megatherium pushed a commit to Megatherium/zf1 that referenced this pull request Dec 31, 2020

Allowed Travis to fail for PHP8.0

35f1a01

As this branch does not contain the major fixes for PHP8 ( cf. zf1s#32) the tests will abort with a fatal. Thus Travis has beeb allowed to fail for PHP8.

glensc mentioned this pull request Jan 26, 2021

fix TypeError: fclose(): supplied resource is not a valid stream reso… laminas/laminas-mail#134

Merged

glensc mentioned this pull request Feb 3, 2021

PHP 8.0 porting status #51

Closed

32 tasks

Megatherium pushed a commit to Megatherium/zf1 that referenced this pull request Feb 4, 2021

Allowed Travis to fail for PHP8.0

83d3004

As this branch does not contain the major fixes for PHP8 ( cf. zf1s#32) the tests will abort with a fatal. Thus Travis has beeb allowed to fail for PHP8.

falkenhawk mentioned this pull request Feb 5, 2021

Remove usage of $php_errormsg #42

Merged

Megatherium force-pushed the php80 branch 2 times, most recently from 14a9170 to da24d0a Compare February 5, 2021 14:22

onlime added a commit to onlime/zf1-future that referenced this pull request Feb 7, 2021

Various fixes for PHP 8.0 compatibility

5ae9f92

mainly backported from zf1s/zf1#32 PR by @Megatherium

onlime mentioned this pull request Feb 7, 2021

Various fixes for PHP 8.0 compatibility Shardj/zf1-future#130

Merged

Fixed a bug in Zend_Date for PHP8.0

0b432b7

Because the return values of iconv_substr changed in PHP8 calling Zend_Date::set could lead to TypeError

falkenhawk mentioned this pull request Feb 9, 2021

Add php 8.0 to GitHub Actions ignoring its errors #59

Merged

This was referenced Mar 15, 2021

Remove default values from method signatures #78

Merged

Fix vsprintf TypeError for php 8.0 #79

Merged

Enforce types for PHP 8.0 #80

Merged

falkenhawk closed this Mar 15, 2021

This was referenced Mar 15, 2021

Fix Zend_Form_ElementTest #81

Merged

Fix iterators usage #82

Merged

DB2 constants are missing when setting up db factory #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP8.0 compatibility patch #32

PHP8.0 compatibility patch #32

Megatherium commented Nov 19, 2020 •

edited by falkenhawk

Loading

glensc commented Nov 19, 2020

glensc left a comment

Megatherium commented Nov 30, 2020

Megatherium commented Nov 30, 2020

Megatherium commented Dec 3, 2020

falkenhawk Dec 3, 2020

Megatherium Dec 3, 2020

falkenhawk Dec 3, 2020

Megatherium Dec 3, 2020

falkenhawk Dec 3, 2020

Megatherium Dec 3, 2020

falkenhawk Dec 3, 2020

Megatherium Dec 3, 2020

falkenhawk Dec 3, 2020

falkenhawk commented Dec 3, 2020

glensc commented Dec 6, 2020

Megatherium commented Feb 5, 2021

Megatherium commented Feb 5, 2021 •

edited

Loading

glensc commented Feb 18, 2021

falkenhawk commented Feb 18, 2021

Megatherium commented Feb 18, 2021

glensc commented Feb 18, 2021

falkenhawk commented Feb 18, 2021

Megatherium commented Feb 19, 2021

glensc commented Mar 13, 2021

falkenhawk commented Mar 15, 2021 •

edited

Loading

PHP8.0 compatibility patch #32

PHP8.0 compatibility patch #32

Conversation

Megatherium commented Nov 19, 2020 • edited by falkenhawk Loading

glensc commented Nov 19, 2020

glensc left a comment

Choose a reason for hiding this comment

Megatherium commented Nov 30, 2020

Megatherium commented Nov 30, 2020

Megatherium commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falkenhawk commented Dec 3, 2020

glensc commented Dec 6, 2020

Megatherium commented Feb 5, 2021

Megatherium commented Feb 5, 2021 • edited Loading

glensc commented Feb 18, 2021

falkenhawk commented Feb 18, 2021

Megatherium commented Feb 18, 2021

glensc commented Feb 18, 2021

falkenhawk commented Feb 18, 2021

Megatherium commented Feb 19, 2021

glensc commented Mar 13, 2021

falkenhawk commented Mar 15, 2021 • edited Loading

Megatherium commented Nov 19, 2020 •

edited by falkenhawk

Loading

Megatherium commented Feb 5, 2021 •

edited

Loading

falkenhawk commented Mar 15, 2021 •

edited

Loading