Fix for PHP 7.4 by remicollet · Pull Request #195 · zendframework/zend-view

remicollet · 2019-10-10T09:01:06Z

The second commit revels an issue with current code.

Zend\View\Variable implements ArrayObject but array_key_exists doesn't work in this case, as eplained in https://wiki.php.net/rfc/deprecations_php_7_4

This PR is not complete, as with 7.4.0RC3 I still encounter an issue
There were 2 errors:


1) ZendTest\View\Helper\PartialLoopTest::testNestedCallsShouldNotOverrideObjectKey
Trying to get property 'helper' of non-object

/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/_files/modules/application/views/scripts/partialLoopChildObject.phtml:10
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Renderer/PhpRenderer.php:506
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/Partial.php:61
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:83
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:61
/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/_files/modules/application/views/scripts/partialLoopParentObject.phtml:15
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Renderer/PhpRenderer.php:506
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/Partial.php:61
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:83
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:61
/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/PartialLoopTest.php:329

2) ZendTest\View\Helper\PartialLoopTest::testNestedCallsShouldNotOverrideObjectKeyInLoopMethod
Trying to get property 'helper' of non-object

/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/_files/modules/application/views/scripts/partialLoopChildObject.phtml:10
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Renderer/PhpRenderer.php:506
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/Partial.php:61
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:83
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:61
/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/_files/modules/application/views/scripts/partialLoopParentObject.phtml:15
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Renderer/PhpRenderer.php:506
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/Partial.php:61
/dev/shm/BUILDROOT/php-zendframework-zend-view-2.11.2-1.fc29.remi.x86_64/usr/share/php/Zend/View/Helper/PartialLoop.php:83
/dev/shm/BUILD/zend-view-4f5cb653ed4c64bb8d9bf05b294300feb00c67f2/test/Helper/PartialLoopTest.php:640

michalbundyra · 2019-10-10T09:02:22Z

See #192

remicollet · 2019-10-10T09:24:39Z

@webimpress sorry I miss this PR,

BTW second commit still make sense ?

michalbundyra · 2019-10-10T09:35:25Z

@remicollet

sorry I miss this PR,

no problem 👍

second commit still make sense ?

yeah, but...

isset !== array_key_exists as you know, so in this case I can find failing test case, for example:

$model->setVariables(['test' => null], true);
$model->getVariable('test', 'default');

before we got null, now we are gonna get default.

Because of that, I think we need change the condition:

if (is_array($this->variables)) {
    if (array_key_exists($name, $this->variables)) {
        return $this->variables[$name];
    }
} elseif ($this->variables->offsetExists($this->variables)) {
    return $this->variables[$name];
}

return $default;

or something like that, haven't tested yet. Do you know what I mean?

remicollet · 2019-10-10T09:39:08Z

Yes, As I explain in initial description, there is some issue.

In current code array_key_exists is used which have never work for ArrayAccess, so don't know how it works (test suite) with PHP <= 7.3

(so this trivial PR preserve current behavior, which is perhaps broken)

michalbundyra · 2019-10-10T09:40:41Z

@remicollet Ok, thanks, I'll have a look on it later on and will try to release bugfix version with PHP 7.4 compatibility later today. Thanks for your effort!

From PHP 7.4 docs: Calling get_object_vars() on an ArrayObject instance will now always return the properties of the ArrayObject itself (or a subclass). Previously it returned the values of the wrapped array/object unless the STD_PROP_LIST flag was specified. Other affected operations are: * ReflectionObject::getProperties() * reset(), current(), etc. Use Iterator methods instead. * Potentially others working on object properties as a list.

Added tests to verify the change.

michalbundyra · 2019-10-10T22:12:34Z

@remicollet I've fixed other failing tests - see commit c711fbd.
So current function on object is not working either - https://3v4l.org/kZidu

I've changed your fix to keep the previous behaviour and added tests to cover the fix.
Let me know what do you think please.

(BTW. yeah, I do not like it too much, but I don't want introduce BC break with the fix...)

remicollet · 2019-10-11T05:54:42Z

+            if (array_key_exists($name, $this->variables)) {
+                return $this->variables[$name];
+            }
+        } elseif ($this->variables->offsetExists($name)) {


Don't you want to check if this is an ArrayAccess object first ?

As I can see from the code (construct, setter...) it must be an array or Zend\View\Variables (extends ArrayObject), so I think no additional check is needed here.

remicollet · 2019-10-11T06:02:31Z

Tested 2.11.2 + pr #192 + pr #195
Everything seems ok

remicollet · 2019-10-11T06:08:58Z

 	echo $vars['message'];
 } else {
-	$objKey = current($this->vars())->helper->getObjectKey();
+	$objKey = current($vars)->helper->getObjectKey();


Nice,

This BC break on ArrayAccess object in 7.4 is terrible
Already bitten on another project, not easy to catch

remicollet · 2019-10-11T06:33:46Z

FYI, according to Fedora CI, excepted zend-server, ZF seems in good shape for 7.4

michalbundyra · 2019-10-11T18:13:51Z

Thanks, @remicollet!

Fix for PHP 7.4

Forward port #195

michalbundyra added this to the 2.11.3 milestone Oct 10, 2019

fix Using array_key_exists() on objects

cd691f9

michalbundyra force-pushed the issue-php74 branch from fc0e69c to cd691f9 Compare October 10, 2019 21:17

michalbundyra added 2 commits October 10, 2019 23:01

Correct the fix so the behaviour of the original code is not changed

81ed6ca

Added tests to verify the change.

michalbundyra added the enhancement label Oct 10, 2019

remicollet commented Oct 11, 2019

View reviewed changes

michalbundyra merged commit 81ed6ca into zendframework:master Oct 11, 2019

michalbundyra added a commit that referenced this pull request Oct 11, 2019

Merge pull request #195 from remicollet/issue-php74

e20147f

Fix for PHP 7.4

michalbundyra added a commit that referenced this pull request Oct 11, 2019

Adds CHANGELOG entry for #195

e5b3af9

michalbundyra added a commit that referenced this pull request Oct 11, 2019

Merge branch 'hotfix/195' into develop

2182d85

Forward port #195

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for PHP 7.4#195

Fix for PHP 7.4#195
michalbundyra merged 3 commits into
zendframework:masterfrom
remicollet:issue-php74

remicollet commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

remicollet commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

remicollet commented Oct 10, 2019 •

edited

Loading

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019 •

edited

Loading

Uh oh!

remicollet Oct 11, 2019

Uh oh!

michalbundyra Oct 11, 2019

Uh oh!

remicollet commented Oct 11, 2019

Uh oh!

remicollet Oct 11, 2019

Uh oh!

remicollet commented Oct 11, 2019

Uh oh!

michalbundyra commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

remicollet commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

remicollet commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

remicollet commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalbundyra commented Oct 10, 2019

Uh oh!

michalbundyra commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remicollet Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

michalbundyra Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

remicollet commented Oct 11, 2019

Uh oh!

remicollet Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

remicollet commented Oct 11, 2019

Uh oh!

michalbundyra commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

remicollet commented Oct 10, 2019 •

edited

Loading

michalbundyra commented Oct 10, 2019 •

edited

Loading