[RFC] Add is_countable function #3026

carusogabriel · 2018-01-19T22:35:35Z

In #2185 a Warning was added while trying to count non-countables:

Warning: count(): Parameter must be an array or an object that implements Countable in %s on line %d

and we start to see everyone changing their codes, usually adding the following condition:

if (is_array($foo) || $foo instanceof \Countable) {...

Inspired by that, this PR adds is_countable() function that returns a boolean:

true, if a value is an array or an instance of Countable interface
false for other values

RFC: https://wiki.php.net/rfc/is-countable

morrisonlevi · 2018-01-21T18:36:54Z

Zend/zend_API.c

@@ -4281,6 +4281,19 @@ ZEND_API zend_bool zend_is_iterable(zval *iterable) /* {{{ */
 }
 /* }}} */

+ZEND_API zend_bool zend_is_countable(zval *countable) /* {{{ */


If possible we should make this zval const *. At a glance it depends if instanceof_function and related calls are const or not.

By convention, zvals are never passed by const pointer.

So it should stay as it is?

The convention likely stems from the days where our zvals directly contained reference counting information. Passing such values by const pointer would prevent making copies on reference counted types, so it wasn't done.

I would prefer it to be const - maybe we should wait for someone else (maybe @dstogov?) to chime in.

Can we please move this discussion elsewhere (i.e. php.internals) and focus on this patch? We currently have two discussions here both unrelated to the patch itself. Thanks. 👍

@Majkl578 You misunderstand. The const discussion is about the patch.

By convention, zvals are never passed by const pointer.

This really sounds like a global issue for all zvals, not just this patch. Trying to mitigate this convention is out of scope here.

This is my opinion.: it appears instanceof_function takes its parameters by pointer to const and Z_TYPE_P should be const correct as well. This function should take its parameter by pointer to const because we have verified here, today, that it is const correct. It is easier to do this now than later. No other functions need to be adjusted for this function to work with const parameters. As such I believe this is in scope for this PR. If other functions would have to be adjust for it to work then it would be out of scope.

@morrisonlevi To avoid this discution on to be const or not to be, should I move this switch to the function declaration, in type.c like #2206?

Also, may I ask you something? What are the difference between have the function here in Zend_api.c or there in type.c?

Having it in the API means it can be called from other places in the C code. I think it belongs there - I would not move it inline like the other PR.

morrisonlevi · 2018-01-21T18:37:18Z

ext/standard/basic_functions.c

@@ -2586,6 +2586,10 @@ ZEND_END_ARG_INFO()
 ZEND_BEGIN_ARG_INFO_EX(arginfo_is_iterable, 0, 0, 1)
 	ZEND_ARG_INFO(0, var)
 ZEND_END_ARG_INFO()
+
+ZEND_BEGIN_ARG_INFO_EX(arginfo_is_countable, 0, 0, 1)


This should have return type information of bool as well.

It may also return null on zpp failure, at which point the typehint is more harmful than useful until such a time as we make all zpp failures throwing.

Can we please make a push to fix these type-safety related issues in PHP 8? Internal and external function behaviors need to be brought in-line with each other. If we can't even ensure that callable (mixed): bool will actually return bool if it happens to be an internal function then we have can't reasonably think about type safety at all.

So it should be ZEND_BEGIN_ARG_INFO(arginfo_is_countable, 0)?

I tried to research what are the differences between ZEND_BEGIN_ARG_INFO and ZEND_BEGIN_ARG_EX but I didn't found 😞

Agreed with @morrisonlevi here, we should eliminate these inconsistency in the near future.

duncan3dc · 2018-01-22T09:52:29Z

Internal classes can also be supported by implementing the count_elements handler, and ignoring the \Countable interface, see #2206 for an example

carusogabriel · 2018-01-22T13:57:13Z

@duncan3dc I’ve searched in both open and close PRs, and, somehow, I didn’t found that one. I’ll include the count_elements handler, and tests for it! Thank you very much.

PS: after that, our code I’ll look almost the same. Should I credit the RFC to you? 😄

duncan3dc · 2018-01-22T14:01:04Z

@carusogabriel No need for that, it's all your own work 👍

carusogabriel · 2018-01-22T21:26:50Z

@duncan3dc Applied count_elements in 5cc4c91, thank you for request it

@morrisonlevi 44118dc Is this the correct way to inform that this returns bool?

lubosdz · 2018-01-23T09:13:09Z

This function would not be necessary, if warning "trying to count uncountable" since 7.2 was adopted under strict type mode. It brings sometime lots of additional coding when mixing strict behaviours into default weak mode. Actually, in a weak mode there's nothing wrong with count(null) = 0 or count("") = 0 or 1 + null = 1 - this way PHP worked since years and such a simple behaviour made PHP so popular. A couple of behaviours have been introduced since PHP 7.1 which should have been implemented under strict mode rather than applied globally (e.g. we are not able to upgrade some of our large projects above 7.0).

carusogabriel · 2018-01-23T16:29:19Z

@lubosdz Thanks for your feedback. Some questions and points:

This function would not be necessary, if warning "trying to count uncountable" since 7.2 was adopted under strict type mode.

Do you have this information precisely? AFAIK, the Warning was add in general, as I've seen projects that are even fully optimized for PHP 7 doing this changes 😅

Actually, in a weak mode there's nothing wrong with count(null) = 0 or count("") = 0 or 1 + null = 1...

This is actually something that should be discussed with @duncan3dc in #2185, right?

rentalhost · 2018-02-26T17:24:54Z

Zend/zend_vm_def.h

@@ -8133,7 +8133,7 @@ ZEND_VM_HANDLER(190, ZEND_COUNT, CONST|TMP|VAR|CV, UNUSED)
 			}

 			/* if not and the object implements Countable we call its count() method */
-			if (instanceof_function(Z_OBJCE_P(op1), zend_ce_countable)) {
+			if (zend_is_countable(op1)) {


Why did you modified this line to use your function instead of keep as original? It seems that will do more checks than original one (switch, IS_ARRAY, IS_OBJECT and just on last step instanceof_function). I don't know if compiler could optimizes it.

@rentalhost I get the same question at #3146 when trying to use zend_is_iterable. Isn't a good practice to DRY our code with functions? Or this is just for PHP, in C we need to focus on performance? 😬

The trouble is the code below (line 8139) expects the object to have a count() method, but zend_is_countable() will return true if an extension class defines a count_elements handler.

As this code is specifically calling the count() method, I think it should check zend_ce_countable

@duncan3dc So, should I revert this changes, or revert the logic and give zend_ce_countable priority in the switch statement is also valid?

@duncan3dc That point is a bit moot, since the checks performed in the code above will already have eliminated that possibility.

I do agree that this change should be reverted, though. Trying to reuse the logic in zend_is_countable isn't overly useful, since we've already duplicated similar checks in the code above.

Reverting this changes as performance was affected :(

rentalhost · 2018-02-26T17:25:58Z

Zend/zend_API.c

+		case IS_ARRAY:
+			return 1;
+		case IS_OBJECT:
+			if (Z_OBJ_HT_P(countable)->count_elements) {


In which case it will be true?

@rentalhost it will be true for extension defined objects that implements their own countable technique

@rentalhost Like @KalleZ said. Also, see @duncan3dc's comment and #2206.

Ref: http://www.phpinternalsbook.com/classes_objects/object_handlers.html#c.count_elements

carusogabriel · 2018-03-10T22:59:12Z

This should be ready to go, as the RFC was approved 😃

nikic · 2018-03-11T15:42:17Z

Merged as d0ee2a8, thanks!

@nicolas-grekas

This PR was squashed before being merged into the 1.7-dev branch (closes #115). Discussion ---------- Add is_countable under PHP 7.3 As requested by @nicolas-grekas :) Waiting for php/php-src#3026 Commits ------- 803a622 Add is_countable under PHP 7.3

carusogabriel changed the title ~~Add is_countable function~~ [RFC] Add is_countable function Jan 21, 2018

morrisonlevi requested changes Jan 21, 2018

View reviewed changes

nikic added the RFC label Jan 24, 2018

carusogabriel mentioned this pull request Feb 14, 2018

[Basic functions] Added return type for arginfo #1492

Closed

rentalhost reviewed Feb 26, 2018

View reviewed changes

carusogabriel mentioned this pull request Mar 2, 2018

Add is_countable under PHP 7.3 symfony/polyfill#115

Merged

Add is_countable function

587fcc5

nikic approved these changes Mar 10, 2018

View reviewed changes

nikic closed this Mar 11, 2018

carusogabriel deleted the is-countable branch March 11, 2018 15:46

joshbruce mentioned this pull request Nov 2, 2022

[DISCUSSION] PHP RFC: Objects can be declared falsifiable #9868

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add is_countable function #3026

[RFC] Add is_countable function #3026

carusogabriel commented Jan 19, 2018 •

edited

Loading

morrisonlevi Jan 21, 2018

nikic Jan 21, 2018

carusogabriel Jan 21, 2018

morrisonlevi Jan 21, 2018

Majkl578 Jan 22, 2018 •

edited

Loading

morrisonlevi Jan 22, 2018

Majkl578 Jan 22, 2018

morrisonlevi Jan 22, 2018

carusogabriel Jan 24, 2018

morrisonlevi Jan 24, 2018

morrisonlevi Jan 21, 2018

nikic Jan 21, 2018

morrisonlevi Jan 21, 2018

carusogabriel Jan 21, 2018

jhdxr Jan 22, 2018

duncan3dc commented Jan 22, 2018

carusogabriel commented Jan 22, 2018

duncan3dc commented Jan 22, 2018

carusogabriel commented Jan 22, 2018 •

edited

Loading

lubosdz commented Jan 23, 2018 •

edited

Loading

carusogabriel commented Jan 23, 2018

rentalhost Feb 26, 2018

carusogabriel Feb 26, 2018 •

edited

Loading

duncan3dc Feb 26, 2018

carusogabriel Feb 26, 2018

tpunt Feb 26, 2018

carusogabriel Feb 26, 2018

rentalhost Feb 26, 2018

KalleZ Feb 26, 2018

carusogabriel Feb 26, 2018

carusogabriel commented Mar 10, 2018

nikic commented Mar 11, 2018

[RFC] Add is_countable function #3026

[RFC] Add is_countable function #3026

Conversation

carusogabriel commented Jan 19, 2018 • edited Loading

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

Majkl578 Jan 22, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan3dc commented Jan 22, 2018

carusogabriel commented Jan 22, 2018

duncan3dc commented Jan 22, 2018

carusogabriel commented Jan 22, 2018 • edited Loading

lubosdz commented Jan 23, 2018 • edited Loading

carusogabriel commented Jan 23, 2018

Choose a reason for hiding this comment

carusogabriel Feb 26, 2018 • edited Loading

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

carusogabriel commented Mar 10, 2018

nikic commented Mar 11, 2018

carusogabriel commented Jan 19, 2018 •

edited

Loading

Majkl578 Jan 22, 2018 •

edited

Loading

carusogabriel commented Jan 22, 2018 •

edited

Loading

lubosdz commented Jan 23, 2018 •

edited

Loading

carusogabriel Feb 26, 2018 •

edited

Loading