Everything

php

How to detect classes with a count_elements handler in userland?

php.internals

Christoph Becker

9 years ago

Hi!

In the discussion on PR #2185[1] we've stumbled upon the issue of
internal classes implementing a count_elements handler (but not the
Countable interface).

Is it possible to detect this in userland?  If not, I would suggest that
we introduce something like is_countable(), so users can check whether
count() may be safely called on a given value.

[1] <https://github.com/php/php-src/pull/2185>

--
Christoph M. Becker

Niklas Keller

9 years ago

2016-11-13 18:34 GMT+01:00 Christoph M. Becker <cmbecker69@gmx.de>:

> Hi!
>
> In the discussion on PR #2185[1] we've stumbled upon the issue of
> internal classes implementing a count_elements handler (but not the
> Countable interface).
>
> Is it possible to detect this in userland?  If not, I would suggest that
> we introduce something like is_countable(), so users can check whether
> count() may be safely called on a given value.
>
> [1] <https://github.com/php/php-src/pull/2185>
>

How about just making those classes implement the interface instead?

Regards, Niklas

Craig Duncan (PHP)

9 years ago

> How about just making those classes implement the interface instead?

Christoph pointed out that there may be classes in extensions that use
count_elements.

Also it would make userland code simpler:
is_countable($thing)
vs
is_array($thing) || $thing implements \Countable

Christoph Becker

9 years ago

On 13.11.2016 at 22:10, Craig Duncan wrote:

>> How about just making those classes implement the interface instead?
>
> Christoph pointed out that there may be classes in extensions that use
> count_elements.

Furthermore, letting those classes implement Countable could break BC.

> Also it would make userland code simpler:
> is_countable($thing)
> vs
> is_array($thing) || $thing implements \Countable

Good catch!

Note that Craig has added is_countable() to PR #2185 (which implements
the RFC).  In my opinion, that is okay, but if anybody has objections,
we probably need another RFC.

--
Christoph M. Becker

Nikita Popov

9 years ago

On Mon, Nov 14, 2016 at 2:22 PM, Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> On 13.11.2016 at 22:10, Craig Duncan wrote:
>
> >> How about just making those classes implement the interface instead?
> >
> > Christoph pointed out that there may be classes in extensions that use
> > count_elements.
>
> Furthermore, letting those classes implement Countable could break BC.
>

How would this break BC?

SXE even already has a count() method, and count_elements respects
overrides of that method. Clearly SXE is supposed to implement Countable,
it was simply forgotten.

> Also it would make userland code simpler:
> > is_countable($thing)
> > vs
> > is_array($thing) || $thing implements \Countable
>
> Good catch!
>
> Note that Craig has added is_countable() to PR #2185 (which implements
> the RFC).  In my opinion, that is okay, but if anybody has objections,
> we probably need another RFC.
>

This should be done as a separate change IMHO.

Nikita

Joe Watkins

9 years ago

Morning,

Just to chime in ... can you split the PR into the RFC, and the new thing
please.

Just another question on how we could make objects that have count_elements
(which is in object handlers) implement an interface on the class entry
(which is detached from handlers) ?

I'm sure it's doable, but it doesn't look very straight forward, some kind
of hacking in instanceof maybe ... not sure ...

Cheers
Joe

On Wed, Nov 16, 2016 at 9:02 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Craig Duncan (PHP)

9 years ago

>
> Just to chime in ... can you split the PR into the RFC, and the new thing
> please.
>

Of course, that's done now: https://github.com/php/php-src/pull/2206

Nikita Popov

9 years ago

On Thu, Nov 17, 2016 at 8:30 AM, Joe Watkins <pthreads@pthreads.org> wrote:

> Morning,
>
> Just to chime in ... can you split the PR into the RFC, and the new thing
> please.
>
> Just another question on how we could make objects that have
> count_elements (which is in object handlers) implement an interface on the
> class entry (which is detached from handlers) ?
>

> I'm sure it's doable, but it doesn't look very straight forward, some kind
> of hacking in instanceof maybe ... not sure ...
>

I don't think this needs any magic. We simply implement the interface by
convention. Just like you are supposed to implement Traversable if you're
implementing get_iterator. Similarly you should implement Countable if
you're using count_elements.

Nikita

Joe Watkins

9 years ago

Afternoon,

A change in convention makes sense, and sounds much nicer than hacking
anything to make it work.

I do prefer this approach to yet-another-function.

Is changing convention all we would do though, or would we raise a warning
when handler is present and interface is not ?

Cheers
Joe

On Thu, Nov 17, 2016 at 11:49 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Christoph Becker

9 years ago

On 16.11.2016 at 22:02, Nikita Popov wrote:

> On Mon, Nov 14, 2016 at 2:22 PM, Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
>
>> On 13.11.2016 at 22:10, Craig Duncan wrote:
>>
>>>> How about just making those classes implement the interface instead?
>>>
>>> Christoph pointed out that there may be classes in extensions that use
>>> count_elements.
>>
>> Furthermore, letting those classes implement Countable could break BC.
>
> How would this break BC?
>
> SXE even already has a count() method, and count_elements respects
> overrides of that method. Clearly SXE is supposed to implement Countable,
> it was simply forgotten.

Thinking about it, we most likely don't have to be concerned with some
*theoretic* BC break (such as `!(instance of Countable)`).

Thanks!

--
Christoph M. Becker