Support for PHP 7.2 #260

kaplanlior · 2017-07-17T15:20:23Z

No description provided.

weierophinney

I've noted changes; we need a solution that does not use error suppression.

Also, if possible, please provide either unit tests that prove the fix, or data we can use to try and reproduce, to ensure we don't break this in the future.

weierophinney · 2017-07-17T17:55:48Z

src/Sql/AbstractSql.php

@@ -241,7 +241,7 @@ protected function createSqlFromSpecificationAndParameters($specifications, $par
            if (isset($paramSpecs[$position]['combinedby'])) {
                $multiParamValues = [];
                foreach ($paramsForPosition as $multiParamsForPosition) {
-                    $ppCount = count($multiParamsForPosition);
+                    $ppCount = @count($multiParamsForPosition);


Our coding guidelines have a preference for NOT using error suppression. In this case, I'd do one of the following:

See if $multiParamsForPosition is an array, and, if so, return count($multiParamsForPosition)

If $multiParamsForPosition is an object, test if it is Countable, and, if so, return $multiParamsForPosition->count()

Otherwise, raise an exception (invalid data is present!)

trasher · 2017-07-18T08:19:33Z

I had to patch for one of my projects; you've also missed some ocurrences. Attached the patch I've applied (zend-db 2.8.2), if that can help :)

zend-db-php7.2.zip

See zendframework/zend-db#260

Calling the count function triggers warning message: "count(): Parameter must be an array or an object that implements Countable". More info at https://wiki.php.net/rfc/counting_non_countables

More info at https://wiki.php.net/rfc/object-typehint

sgchris · 2017-07-19T12:44:51Z

Raising an exception breaks backward compatibility - if beforehand we passed a string to the function everything worked fine, now, after the fix it will fail.
(PHP's "count" with string raises a warning, but doesn't break the application)

weierophinney · 2017-07-19T13:07:15Z

(PHP's "count" with string raises a warning, but doesn't break the application)

What does it return in such cases? Is it usable?

sgchris · 2017-07-19T13:19:42Z

It returns 1, and starting from v7.2 it raises a warning.

weierophinney · 2017-07-19T13:30:17Z

It returns 1, and starting from v7.2 it raises a warning.

Right, and the warning is what the original patch was trying to eliminate. The question I have is: is 1 an appropriate return for this? If so, instead of raising an exception for such uncountable values, we could return 1. But if it's not, the exception is essentially fixing incorrect behavior.

sgchris · 2017-07-26T06:47:50Z

I think it's best to be stick to PHP's behavior, and it tells us kindly to use "count" in a proper way (with notice or warning), but it doesn't break the functionality of existing applications.

It would be very wrong and unpleasent situation when someone will upgrade his ZF minor version, and discover that something is broken.

I assume that in further PHP versions, this issue will become an error, than we should fix it in ZF too. But now it gives some time to breathe, reconsider "count" usage and fix the running apps.

So - yes, we should return 1 for "string" var (for other types we should check "count" behavior)

alextech · 2017-07-26T13:33:52Z

Exception is a proper way to be kindly notified that something needs fixing. Framework throwing new exception is something I can fix. Framework raising a notice or warning is not something I can deal with. If I do not want to take a hit at engine level for having notice rasied, AND cannot realistically deal with production logging now filled with warnings and CI build system that inspects code for engine message break the build, framework raising messages is something i cannot fix.

sgchris · 2017-07-26T14:22:03Z

Initially, I raised this issue in order to keep backwards compatibility. However, in the updated file I noticed even more significant change - the declaration of the method changed. According to the old PHPDoc the parameter was of type "string|array", but now it's just "array". That's a big issue.
Much simpler solution would be to wrap the string parameter in an array. A simple "if" statement in the beginning of the method, and we aren't dealing here with log messages and exceptions.

alextech · 2017-07-26T14:28:29Z

Sorry, can you please point at the declaration of method you refer to? I can't find it in the changelist.

sgchris · 2017-07-26T19:12:32Z

The one I meant is related to commit eb6cb76, in which the "string" word was removed from the phpdoc. There is a "count" applied on that parameter too.

trasher · 2017-08-15T16:33:47Z

I've just get an issue using something like $select->group('myfield'): the parameter in that case is a string; and the exception is raised... This is not expected.

I do not know enough the framework to be sure we really want a string in that case; but since we got one; calling an exception is probably not the way to go.
Actual behavihor (count equals to 1) does not seems correct. What about just taking the verbatim value when this is not an array? Instead of the exception; we could simply do a $multiParamValues[] = $multiParamsForPosition; I guess.

See zendframework/zend-db#260

remicollet · 2017-10-31T09:21:36Z

src/Sql/AbstractSql.php

+                    } else {
+                        throw new Exception\UnexpectedValueException(sprintf(
+                            'Parameters must be an array or a countable object, %s given',
+                            gettype($multiParamsForPosition)


I think this should not raise an exception, as this may break some other code, for the string case

At least in zend-paginator test suite

1) ZendTest\Paginator\Adapter\DbTableGatewayTest::testGetItemsWithWhereAndOrderAndGroup count(): Parameter must be an array or an object that implements Countable /usr/share/php/Zend/Db/Sql/AbstractSql.php:244 /usr/share/php/Zend/Db/Sql/AbstractSql.php:72 /usr/share/php/Zend/Db/Sql/AbstractPreparableSql.php:34 /usr/share/php/Zend/Db/Sql/Platform/Platform.php:100 /usr/share/php/Zend/Db/Sql/Sql.php:130 /builddir/build/BUILDROOT/php-zendframework-zend-paginator-2.7.0-3.fc28.noarch/usr/share/php/Zend/Paginator/Adapter/DbSelect.php:100 /builddir/build/BUILD/zend-paginator-42211f3e1e8230953c641e91fec5aa9fe964eb95/test/Adapter/DbTableGatewayTest.php:121 2) ZendTest\Paginator\Adapter\DbTableGatewayTest::testGetItemsWithWhereAndOrderAndGroupAndHaving count(): Parameter must be an array or an object that implements Countable /usr/share/php/Zend/Db/Sql/AbstractSql.php:244 /usr/share/php/Zend/Db/Sql/AbstractSql.php:72 /usr/share/php/Zend/Db/Sql/AbstractPreparableSql.php:34 /usr/share/php/Zend/Db/Sql/Platform/Platform.php:100 /usr/share/php/Zend/Db/Sql/Sql.php:130 /builddir/build/BUILDROOT/php-zendframework-zend-paginator-2.7.0-3.fc28.noarch/usr/share/php/Zend/Paginator/Adapter/DbSelect.php:100 /builddir/build/BUILD/zend-paginator-42211f3e1e8230953c641e91fec5aa9fe964eb95/test/Adapter/DbTableGatewayTest.php:143

ezimuel · 2017-11-28T12:51:04Z

Fixed in #276

weierophinney suggested changes Jul 17, 2017

View reviewed changes

trasher added a commit to galette/galette that referenced this pull request Jul 18, 2017

Patch for Zend-db PHP 7.2 compatibility

7622090

See zendframework/zend-db#260

kaplanlior added 2 commits July 19, 2017 15:09

PHP 7.2: Counting of non-countable objects

878ecb7

Calling the count function triggers warning message: "count(): Parameter must be an array or an object that implements Countable". More info at https://wiki.php.net/rfc/counting_non_countables

PHP 7.2: 'Object' is a special class name

39c0d1b

More info at https://wiki.php.net/rfc/object-typehint

kaplanlior force-pushed the zend-php7.2 branch from 9cf99f2 to 39c0d1b Compare July 19, 2017 12:11

trasher added a commit to galette/galette that referenced this pull request Aug 16, 2017

Update patch for Zend-db PHP 7.2 compatibility

1977545

See zendframework/zend-db#260

froschdesign added the enhancement label Oct 24, 2017

remicollet reviewed Oct 31, 2017

View reviewed changes

remicollet mentioned this pull request Oct 31, 2017

Fix for PHP 7.2 #276

Merged

ezimuel closed this Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support for PHP 7.2 #260

Support for PHP 7.2 #260

Uh oh!

kaplanlior commented Jul 17, 2017

Uh oh!

weierophinney left a comment

Uh oh!

weierophinney Jul 17, 2017

Uh oh!

trasher commented Jul 18, 2017

Uh oh!

sgchris commented Jul 19, 2017

Uh oh!

weierophinney commented Jul 19, 2017

Uh oh!

sgchris commented Jul 19, 2017

Uh oh!

weierophinney commented Jul 19, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

alextech commented Jul 26, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

alextech commented Jul 26, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

trasher commented Aug 15, 2017

Uh oh!

remicollet Oct 31, 2017 •

edited

Loading

Uh oh!

ezimuel commented Nov 28, 2017

Uh oh!

Uh oh!

Support for PHP 7.2 #260

Support for PHP 7.2 #260

Uh oh!

Conversation

kaplanlior commented Jul 17, 2017

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

weierophinney Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

trasher commented Jul 18, 2017

Uh oh!

sgchris commented Jul 19, 2017

Uh oh!

weierophinney commented Jul 19, 2017

Uh oh!

sgchris commented Jul 19, 2017

Uh oh!

weierophinney commented Jul 19, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

alextech commented Jul 26, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

alextech commented Jul 26, 2017

Uh oh!

sgchris commented Jul 26, 2017

Uh oh!

trasher commented Aug 15, 2017

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezimuel commented Nov 28, 2017

Uh oh!

Uh oh!

remicollet Oct 31, 2017 •

edited

Loading