Throw an exception when attempting to count a generator

[duncan3dc]

Generators are often looped around, and where you have code that accepts a traversable, it's logical to try and count it. At the moment counting a generator returns int(1), so the unsupported behaviour can easily go unnoticed.

This pr proposes to change that by throwing an exception whenever code attempts to count a generator

[jpauli]

This can't target PHP 7.0 as it breaks API.
It can target PHP 7.1 though.
ping @nikic ?

[nikic]

@jpauli I think this is a good change.

However I wonder whether we shouldn't make this an error for all counts on non-Countable objects. The current behavior is very useless and probably easy to make a mistake if you're working with iterators.

[duncan3dc]

Yeh I would much rather change it to throw an Exception for all non-countable objects. Would that need to target 8.0 though? Shall I adjust the patch, or can we get this change through in 7.1?

[jpauli]

@nikic I'm also +1 to throw an Exception on any count operation involving non-countable object , but that should target 8.0

@duncan3dc I think having a patch just for the Generator case makes little sense if we choose to apply it to every object. I guess it is time to propose such a change into an RFC

[nikic]

An RFC should be able to target this at 7.1. Whether it will pass likely depends on whether someone finds a legit BC concern with the change.

[jpauli]

I think the BC is too big for a minor and should target a major

[jerrygrey]

Throw an Exception for all non-countable objects is a good idea, I'm surprised it doesn't do it already.

[morrisonlevi]

Something that is related: foreach on objects that do not implement Iterator will iterate over the visible properties. It is my opinion that these should be tackled in connection with each other.

[nikic]

@jpauli How about a warning? Exceptions in standard library are a tricky issue, but I don't think anybody would have a problem with a warning.

@morrisonlevi That seems unrelated, and the current behavior is an explicitly supported feature. I'd appreciate the introduction of a proper property iterator type, which would allow us to phase out this behavior in the future, however I don't see the relation to count().

[jpauli]

@nikic Why not, but this would BC as well :-p

[duncan3dc]

@jpauli What about E_DEPRECATED for 7.*?

[nikic]

@jpauli I don't think we consider throwing additional non-fatal diagnostics to be a BC break. We really should specify this kind of stuff.

[jpauli]

To be asked to the RM :-)

[morrisonlevi]

In any case throwing an exception or raising an error should not be specific to Generators in my opinion. It's the same exact logic issue with all other non-Countable objects. I think we should not special case the Generator.

Said otherwise we should raise an error/warning/notice for all non-Countable objects (not just Generators)

[jpauli]

@morrisonlevi I tend to agree here, though I'm still not sure it should be done in a minor

[duncan3dc]

Thanks for the input everyone, the RFC is up for discussion now