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

RFC: Counting of non-countable objects #2185

Closed
wants to merge 5 commits into
base: master
from

Conversation

8 participants
@duncan3dc
Contributor

duncan3dc commented Nov 5, 2016

Implementation for the recently accepted rfc: https://wiki.php.net/rfc/counting_non_countables

I've added new tests (count_invalid.phpt) to ensure all non-countables issue a warning.

For existing tests I've either hidden warnings (because there were too many) or added expectations where warnings were already in use. I'm not sure of the usual approach here?

@cmb69

This comment has been minimized.

Contributor

cmb69 commented Nov 5, 2016

IMO, tests shouldn't suppress warnings; you never know which warnings you might suppress inadvertently.

And please have a look at the failing tests reported by Travis. At least some of these failures are caused by the new warnings.

@duncan3dc

This comment has been minimized.

Contributor

duncan3dc commented Nov 5, 2016

@cmb69 Agreed, what would you suggest we do? Adding the warnings into the expected output of some of those tests makes them unmanageable in my opinion.

Sorry I checked Travis earlier and didn't think any were related, I'll look again 👍

@smalyshev smalyshev added the RFC label Nov 6, 2016

@cmb69

This comment has been minimized.

Contributor

cmb69 commented Nov 7, 2016

Adding the warnings into the expected output of some of those tests makes them unmanageable in my opinion.

Not sure what to do, but we may consider to change these tests. Perhaps splitting these PHPTs is an option.

@@ -1637,7 +1637,7 @@ function run_test($php, $file, $env)
$IN_REDIRECT['dir'] = realpath(dirname($file));
$IN_REDIRECT['prefix'] = trim($section_text['TEST']);
if (count($IN_REDIRECT['TESTS']) == 1) {
if (is_array($IN_REDIRECT['TESTS']) && count($IN_REDIRECT['TESTS']) == 1) {

This comment has been minimized.

@cmb69

cmb69 Nov 7, 2016

Contributor

It seems that this change lets several (PDO) tests fail.

@krakjoe

This comment has been minimized.

Member

krakjoe commented Nov 13, 2016

@duncan3dc The tests should have the new warnings added to expected output, and should not suppress anything.

@nikic

This comment has been minimized.

Member

nikic commented Nov 13, 2016

@krakjoe I think it's fine to suppress the warning for the sizeof() tests, as the emission of the warning is already tested for the alias count().

For the exif, filter, xml tests I would suggest to rewrite the test so it no longer emits a warning. The warning is a result of bad test code, not something that the test explicitly tests.

@cmb69

This comment has been minimized.

Contributor

cmb69 commented Nov 13, 2016

I think it's fine to suppress the warning for the sizeof() tests, as the emission of the warning is already tested for the alias count().

We may consider to remove the sizeof() tests altogether in favor of solely testing count().

@cmb69

This comment has been minimized.

Contributor

cmb69 commented Nov 13, 2016

I've noticed a small discrepancy between the RFC and this implementation. The RFC states:

This RFC proposes adding a warning when calling count() with a parameter that is a scalar, null, or an object that doesn't implement Countable.

However, the implementation (IMO rightly) also allows internal classes which do not implement Countable, but implement a count_elements handler, such as SimpleXMLElement. That would have to be documented, and it should also be documented which classes implement a count_element handler.

@duncan3dc

This comment has been minimized.

Contributor

duncan3dc commented Nov 13, 2016

Doesn't that make it impossible in userland to know when count() can be safely called? Is there any userland way to detect an object that has a count_elements handler? Or can/should we have these objects declare that they implement Countable?

@cmb69

This comment has been minimized.

Contributor

cmb69 commented Nov 13, 2016

Is there any userland way to detect an object that has a count_elements handler?

AFAIK, no.

Or can/should we have these objects declare that they implement Countable?

That would probably the cleanest solution, but what about existing classes that we don't control (i.e. PECL or even private)? Maybe we should add something like is_countable().

For the exif, filter, xml tests I would suggest to rewrite the test so it no longer emits a warning.

Done with 1ccada3.

"true" => true,
"false" => false,
"object" => (object) [],
"simplexml" => new SimpleXMLElement("<xml><tag1></tag1><tag2></tag2></xml>"),

This comment has been minimized.

@krakjoe

krakjoe Nov 13, 2016

Member

I know why it's using this class, but it's a bit strange to have a standard test bound to simplexml, which isn't always loaded.

Could we maybe find some other object with the same behaviour ?

This comment has been minimized.

@cmb69

cmb69 Nov 14, 2016

Contributor

Some of the SPL classes (e.g. ArrayObject and SplFixedArray) also implement count_elements, and are always available as of PHP 5.3.0.

This comment has been minimized.

@duncan3dc

duncan3dc Nov 14, 2016

Contributor

Thanks @cmb69, I'll take a look

@php-pulls

This comment has been minimized.

php-pulls commented Nov 17, 2016

Comment on behalf of krakjoe at php.net:

Merged

@php-pulls php-pulls closed this Nov 17, 2016

@duncan3dc duncan3dc deleted the duncan3dc:counting-non-countables branch Nov 17, 2016

@geomorillo

This comment has been minimized.

geomorillo commented Jun 17, 2018

This breaks lots of thing when upgrading

@gouchaoer

This comment has been minimized.

gouchaoer commented Jun 28, 2018

The count function break BC so much and make it difficult to upgrade to PHP7.2.
I hope the community don't make any wrong decision like count in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment