Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Ferenc Kovacs
On Mon, Jul 28, 2014 at 3:03 PM, Matteo Beccati p...@beccati.com wrote:

 Hi everyone,

 On 28/07/2014 09:46, Michael Wallner wrote:
 
 https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
 
  +1 on Matteo's patch. Rather a single fix than a couple.
  IIRC, we also have to think about the count_elements handler.

 I committed my patch locally and I was looking into the count_elements
 inconsistency, hoping to push both fixes later today. While doing that I
 had an epiphany which led me to think that we have a much bigger problem
 to solve.

 As it is now, the interface definition looks like:

 interface Countable {
   public function count();
 }

 Yes, it might (or will, if we don't apply my patch) be called as
 count($mode = COUNT_NORMAL), but the definition on the manual
 (http://php.net/manual/en/countable.count.php) is wrong and can only be
 considered a hint.

 To me this is quite a major gotcha. On the other hand we can't alter the
 method signature, otherwise the userland code implementing Countable
 will start triggering fatal errors:

 Declaration of SomeClass::count() must be compatible with
 Countable::count($mode = COUNT_NORMAL)

 Which is something we could do for .NEXT maybe, but certainly not in
 5.6RC3.

 My suggestion would be to revert the changes and start an RFC process to
 properly fix the issue, if that's what we want.

 We could even add a new CountableRecursive interface that implements
 Countable and provides count($mode = COUNT_NORMAL) if we don't want to
 take the BC hit, but then again it's not 5.6RC3 material to me.


 Cheers
 --
 Matteo Beccati

 Development  Consulting - http://www.beccati.com/

 --
 PHP Internals - PHP Runtime Development Mailing List
 To unsubscribe, visit: http://www.php.net/unsub.php


Hi,

I think considering the pitfalls we have discovered, I think it would be
better to revert the fix for https://bugs.php.net/bug.php?id=67064 and wait
for the next major to properly introduce the optional $mode parameter to
the Countable::count() signature.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu


Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Bob Weinand
Am 28.7.2014 um 15:03 schrieb Matteo Beccati p...@beccati.com:
 Hi everyone,
 
 On 28/07/2014 09:46, Michael Wallner wrote:
 https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
 
 +1 on Matteo's patch. Rather a single fix than a couple.
 IIRC, we also have to think about the count_elements handler.
 
 I committed my patch locally and I was looking into the count_elements
 inconsistency, hoping to push both fixes later today. While doing that I
 had an epiphany which led me to think that we have a much bigger problem
 to solve.
 
 As it is now, the interface definition looks like:
 
 interface Countable {
  public function count();
 }
 
 Yes, it might (or will, if we don't apply my patch) be called as
 count($mode = COUNT_NORMAL), but the definition on the manual
 (http://php.net/manual/en/countable.count.php) is wrong and can only be
 considered a hint.
 
 To me this is quite a major gotcha. On the other hand we can't alter the
 method signature, otherwise the userland code implementing Countable
 will start triggering fatal errors:
 
 Declaration of SomeClass::count() must be compatible with
 Countable::count($mode = COUNT_NORMAL)
 
 Which is something we could do for .NEXT maybe, but certainly not in 5.6RC3.
 
 My suggestion would be to revert the changes and start an RFC process to
 properly fix the issue, if that's what we want.
 
 We could even add a new CountableRecursive interface that implements
 Countable and provides count($mode = COUNT_NORMAL) if we don't want to
 take the BC hit, but then again it's not 5.6RC3 material to me.
 
 
 Cheers
 -- 
 Matteo Beccati
 
 Development  Consulting - http://www.beccati.com/


Is this a problem if the interface internally doesn't expect a parameter?

You're free to expect the parameter or not, where's  the issue?
We allow implementations to accept more optional parameters than the interface 
specifies, but not less.

So, it seems optimal to me, to make the interface specify no parameters, then 
the class implementing this interface is free to accept either no parameter (= 
don't care about recursive) or one optional one.

I'll though happily apply your patch now.

Bob

Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Matteo Beccati
Hi Bob,

On 30/07/2014 14:33, Bob Weinand wrote:
 Is this a problem if the interface internally doesn't expect a parameter?
 
 You're free to expect the parameter or not, where's  the issue?
 We allow implementations to accept more optional parameters than the
 interface specifies, but not less.

Yes, precisely. If something is using an Interface it should *not* add
unspecified parameters to the method calls, as the implementation might
have added some of their own with a different meaning.

 So, it seems optimal to me, to make the interface specify no parameters,
 then the class implementing this interface is free to accept either no
 parameter (= don't care about recursive) or one optional one.

So, PHP specifies a Countable interface, and its count() method has no
parameters. However, if you check the documentation then you'll find out
that in fact it might be sent one. On some occasions.

 I'll though happily apply your patch now.

I wrote it, but to be honest now I think it's just hiding the problem
under the carpet.


Cheers
-- 
Matteo Beccati

Development  Consulting - http://www.beccati.com/

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Ferenc Kovacs
On Wed, Jul 30, 2014 at 3:54 PM, Matteo Beccati p...@beccati.com wrote:

 Hi Bob,

 On 30/07/2014 14:33, Bob Weinand wrote:
  Is this a problem if the interface internally doesn't expect a parameter?
 
  You're free to expect the parameter or not, where's  the issue?
  We allow implementations to accept more optional parameters than the
  interface specifies, but not less.

 Yes, precisely. If something is using an Interface it should *not* add
 unspecified parameters to the method calls, as the implementation might
 have added some of their own with a different meaning.

  So, it seems optimal to me, to make the interface specify no parameters,
  then the class implementing this interface is free to accept either no
  parameter (= don't care about recursive) or one optional one.

 So, PHP specifies a Countable interface, and its count() method has no
 parameters. However, if you check the documentation then you'll find out
 that in fact it might be sent one. On some occasions.

  I'll though happily apply your patch now.

 I wrote it, but to be honest now I think it's just hiding the problem
 under the carpet.


 Cheers
 --
 Matteo Beccati

 Development  Consulting - http://www.beccati.com/


for the record here is a code snippet showing the BC break:
http://3v4l.org/4O9nA
and as far as I Can understand, even with Matteo's patch, the output would
be still different(1,1,0,0)

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu


Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Ferenc Kovacs
On Wed, Jul 30, 2014 at 4:48 PM, Ferenc Kovacs tyr...@gmail.com wrote:




 On Wed, Jul 30, 2014 at 3:54 PM, Matteo Beccati p...@beccati.com wrote:

 Hi Bob,

 On 30/07/2014 14:33, Bob Weinand wrote:
  Is this a problem if the interface internally doesn't expect a
 parameter?
 
  You're free to expect the parameter or not, where's  the issue?
  We allow implementations to accept more optional parameters than the
  interface specifies, but not less.

 Yes, precisely. If something is using an Interface it should *not* add
 unspecified parameters to the method calls, as the implementation might
 have added some of their own with a different meaning.

  So, it seems optimal to me, to make the interface specify no parameters,
  then the class implementing this interface is free to accept either no
  parameter (= don't care about recursive) or one optional one.

 So, PHP specifies a Countable interface, and its count() method has no
 parameters. However, if you check the documentation then you'll find out
 that in fact it might be sent one. On some occasions.

  I'll though happily apply your patch now.

 I wrote it, but to be honest now I think it's just hiding the problem
 under the carpet.


 Cheers
 --
 Matteo Beccati

 Development  Consulting - http://www.beccati.com/


 for the record here is a code snippet showing the BC break:
 http://3v4l.org/4O9nA
 and as far as I Can understand, even with Matteo's patch, the output would
 be still different(1,1,0,0)


if we want to introduce it properly, I think it would be better to do it in
a major version, where we can add the $mode to the Countable::count()
signature as a mandatory argument, so if by chance anybody already used an
optional argument to their count() implementation, they will be notified
via the Declaration of MyCountable::count() must be compatible with that
of Countable::count() fatal error.
Given that this is a BC break(and a hard-to-catch one) which is not allowed
in a minor release and that https://bugs.php.net/bug.php?id=67064 is more
of a feature request than an actual bugfix, I would like to ask you to
revert this from the PHP-5.6 branch.
Sorry for not spotting this sooner. :(

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu


Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-30 Thread Bob Weinand
Am 30.7.2014 um 17:08 schrieb Ferenc Kovacs tyr...@gmail.com:
 On Wed, Jul 30, 2014 at 4:48 PM, Ferenc Kovacs tyr...@gmail.com wrote:
 On Wed, Jul 30, 2014 at 3:54 PM, Matteo Beccati p...@beccati.com wrote:
 Hi Bob,
 
 On 30/07/2014 14:33, Bob Weinand wrote:
 Is this a problem if the interface internally doesn't expect a
 parameter?
 
 You're free to expect the parameter or not, where's  the issue?
 We allow implementations to accept more optional parameters than the
 interface specifies, but not less.
 
 Yes, precisely. If something is using an Interface it should *not* add
 unspecified parameters to the method calls, as the implementation might
 have added some of their own with a different meaning.
 
 So, it seems optimal to me, to make the interface specify no parameters,
 then the class implementing this interface is free to accept either no
 parameter (= don't care about recursive) or one optional one.
 
 So, PHP specifies a Countable interface, and its count() method has no
 parameters. However, if you check the documentation then you'll find out
 that in fact it might be sent one. On some occasions.
 
 I'll though happily apply your patch now.
 
 I wrote it, but to be honest now I think it's just hiding the problem
 under the carpet.
 
 
 Cheers
 --
 Matteo Beccati
 
 Development  Consulting - http://www.beccati.com/
 
 
 for the record here is a code snippet showing the BC break:
 http://3v4l.org/4O9nA
 and as far as I Can understand, even with Matteo's patch, the output would
 be still different(1,1,0,0)
 
 
 if we want to introduce it properly, I think it would be better to do it in
 a major version, where we can add the $mode to the Countable::count()
 signature as a mandatory argument, so if by chance anybody already used an
 optional argument to their count() implementation, they will be notified
 via the Declaration of MyCountable::count() must be compatible with that
 of Countable::count() fatal error.
 Given that this is a BC break(and a hard-to-catch one) which is not allowed
 in a minor release and that https://bugs.php.net/bug.php?id=67064 is more
 of a feature request than an actual bugfix, I would like to ask you to
 revert this from the PHP-5.6 branch.
 Sorry for not spotting this sooner. :(
 
 -- 
 Ferenc Kovács
 @Tyr43l - http://tyrael.hu

Okay, after lengthy discussions it's now reverted.

Bob

p.s.: And I didn't make attention and a part of the revert went into the next 
commit (which is basically totally unrelated...), sorry.



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-28 Thread Ferenc Kovacs
On Fri, Jul 25, 2014 at 11:54 AM, Matteo Beccati p...@beccati.com wrote:

 On 23/07/2014 13:01, Dan Ackroyd wrote:
  Regarding the change in behaviour, I think the patch
 
 https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
  should definitely be applied.
 
  Currently the engine is doing magic stuff by modifying the parameters
  that are passed to the function. i.e. you call count() with no
  parameters and it arrives at the function with parameters. \o/
 
  Even if that wasn't a BC break, having magic behaviour in a language
  is very, very bad.

 I agree, that's why I wrote the tiny patch in the first place ;)

 IMHO, the functionality as it is now breaks the principle of least
 surprise.


Bob, what's your opinion on this?
You also had a conversation with Remi on irc, but I don't remember what was
the decision if any.
The PR from Remi for imagick to support 5.6 doesn't seem so bad (as in:
doesn't require too big of a change from the extension's part), but we also
have a couple of classes implementing the countable interface in core:
http://lxr.php.net/search?q=spl_ce_Countabledefs=refs=path=hist=project=PECL
Whats the current plan? Removing the internal BC break, or keeping it and
accomodating the affected parts in php-src?

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu


Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-28 Thread Michael Wallner
On 28/07/14 09:30, Ferenc Kovacs wrote:
 On Fri, Jul 25, 2014 at 11:54 AM, Matteo Beccati p...@beccati.com wrote:

  On 23/07/2014 13:01, Dan Ackroyd wrote:
  Regarding the change in behaviour, I think the patch
 
  https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
  should definitely be applied.
 
  Currently the engine is doing magic stuff by modifying the parameters
  that are passed to the function. i.e. you call count() with no
  parameters and it arrives at the function with parameters. \o/
 
  Even if that wasn't a BC break, having magic behaviour in a language
  is very, very bad.
 
  I agree, that's why I wrote the tiny patch in the first place ;)
 
  IMHO, the functionality as it is now breaks the principle of least
  surprise.
 
 
 Bob, what's your opinion on this?
 You also had a conversation with Remi on irc, but I don't remember what was
 the decision if any.
 The PR from Remi for imagick to support 5.6 doesn't seem so bad (as in:
 doesn't require too big of a change from the extension's part), but we also
 have a couple of classes implementing the countable interface in core:
 http://lxr.php.net/search?q=spl_ce_Countabledefs=refs=path=hist=project=PECL
 Whats the current plan? Removing the internal BC break, or keeping it and
 accomodating the affected parts in php-src?


+1 on Matteo's patch. Rather a single fix than a couple.
IIRC, we also have to think about the count_elements handler.

-- 
Regards,
Mike

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-28 Thread Matteo Beccati
Hi everyone,

On 28/07/2014 09:46, Michael Wallner wrote:
 https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
 
 +1 on Matteo's patch. Rather a single fix than a couple.
 IIRC, we also have to think about the count_elements handler.

I committed my patch locally and I was looking into the count_elements
inconsistency, hoping to push both fixes later today. While doing that I
had an epiphany which led me to think that we have a much bigger problem
to solve.

As it is now, the interface definition looks like:

interface Countable {
  public function count();
}

Yes, it might (or will, if we don't apply my patch) be called as
count($mode = COUNT_NORMAL), but the definition on the manual
(http://php.net/manual/en/countable.count.php) is wrong and can only be
considered a hint.

To me this is quite a major gotcha. On the other hand we can't alter the
method signature, otherwise the userland code implementing Countable
will start triggering fatal errors:

Declaration of SomeClass::count() must be compatible with
Countable::count($mode = COUNT_NORMAL)

Which is something we could do for .NEXT maybe, but certainly not in 5.6RC3.

My suggestion would be to revert the changes and start an RFC process to
properly fix the issue, if that's what we want.

We could even add a new CountableRecursive interface that implements
Countable and provides count($mode = COUNT_NORMAL) if we don't want to
take the BC hit, but then again it's not 5.6RC3 material to me.


Cheers
-- 
Matteo Beccati

Development  Consulting - http://www.beccati.com/

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-25 Thread Matteo Beccati
On 23/07/2014 13:01, Dan Ackroyd wrote:
 Regarding the change in behaviour, I think the patch
 https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
 should definitely be applied.
 
 Currently the engine is doing magic stuff by modifying the parameters
 that are passed to the function. i.e. you call count() with no
 parameters and it arrives at the function with parameters. \o/
 
 Even if that wasn't a BC break, having magic behaviour in a language
 is very, very bad.

I agree, that's why I wrote the tiny patch in the first place ;)

IMHO, the functionality as it is now breaks the principle of least surprise.


Cheers
-- 
Matteo Beccati

Development  Consulting - http://www.beccati.com/

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



[PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Remi Collet
Notice,

All Internal classes, implementing Countable still doesn't accept this
optional parameter.

Ex:

Method [ internal:SPL, prototype Countable public method count ] {

  - Parameters [0] {
  }

Indeed, all Spl classes still use zend_parse_parameters_none().
This work, because  count($a) doesn't use the method, but the
count_elements handler.

If a internal class doesn't implement the handler, it will be hit by
this change.

Both solution should fix this:
- adding the count_elements handler
- accepting the optional mode

But, Reflection still not correct, and count($a) still raise the

Warning: Imagick::count() expects exactly 0 parameters, 1 given in
/work/GIT/imagick/tests/021-countable.phpt on line 19

I think this is not consistent (internal vs userland)


Remi.


P.S. discovered on imagick
See: https://github.com/mkoppanen/imagick/pull/35


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



[PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Remi Collet
Notice,

All Internal classes, implementing Countable still doesn't accept this
optional parameter.

Ex:

Method [ internal:SPL, prototype Countable public method count ] {

  - Parameters [0] {
  }

Indeed, all Spl classes still use zend_parse_parameters_none().
This work, because  count($a) doesn't use the method, but the
count_elements handler.

If a internal class doesn't implement the handler, it will be hit by
this change.

Both solution should fix this:
- adding the count_elements handler
- accepting the optional mode

But, Reflection still not correct, and count($a) still raise the

Warning: Imagick::count() expects exactly 0 parameters, 1 given in
/work/GIT/imagick/tests/021-countable.phpt on line 19

I think this is not consistent (internal vs userland)


Remi.


P.S. discovered on imagick
See: https://github.com/mkoppanen/imagick/pull/35


-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Remi Collet
Le 23/07/2014 10:26, Remi Collet a écrit :

 Both solution should fix this:
 - adding the count_elements handler
 - accepting the optional mode

Both works.

 But, Reflection still not correct, and count($a) still raise the

IT is ok (sorry for the noise, tested with a bad path...)

 I think this is not consistent (internal vs userland)

Inconsistency is :

  $a-count() call the method (with the optional arg),
  count($a) call the handler


Rmei.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Matteo Beccati
On 23/07/2014 10:57, Remi Collet wrote:
 I think this is not consistent (internal vs userland)
 
 Inconsistency is :
 
   $a-count() call the method (with the optional arg),
   count($a) call the handler

quite a while ago I've attached a patch that fixes such incostistency:

https://bugs.php.net/bug.php?id=67064

I didn't apply it as I wasn't sure it was the desired behaviour.


Cheers
-- 
Matteo Beccati

Development  Consulting - http://www.beccati.com/

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Dan Ackroyd
Hi Remi,

Thanks for the PR. I'll merge it as soon as I've been able to test it,
as the class should respect the definition of the method in the
interface.

Regarding the change in behaviour, I think the patch
https://bugs.php.net/patch-display.php?bug=67064patch=bug67064-BCrevision=1402667581
should definitely be applied.

Currently the engine is doing magic stuff by modifying the parameters
that are passed to the function. i.e. you call count() with no
parameters and it arrives at the function with parameters. \o/

Even if that wasn't a BC break, having magic behaviour in a language
is very, very bad.

cheers
Dan

On 23 July 2014 09:57, Remi Collet r...@fedoraproject.org wrote:
 Le 23/07/2014 10:26, Remi Collet a écrit :

 Both solution should fix this:
 - adding the count_elements handler
 - accepting the optional mode

 Both works.

 But, Reflection still not correct, and count($a) still raise the

 IT is ok (sorry for the noise, tested with a bad path...)

 I think this is not consistent (internal vs userland)

 Inconsistency is :

   $a-count() call the method (with the optional arg),
   count($a) call the handler


 Rmei.

 --
 PHP Internals - PHP Runtime Development Mailing List
 To unsubscribe, visit: http://www.php.net/unsub.php


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



Re: [PHP-DEV] Re: [PHP-CVS] com php-src: Fix bug #67064 = Countable interface inconsistency

2014-07-23 Thread Tjerk Meesters
Hi!


On Wed, Jul 23, 2014 at 4:26 PM, Remi Collet r...@famillecollet.com wrote:

 Notice,

 All Internal classes, implementing Countable still doesn't accept this
 optional parameter.

 Ex:

 Method [ internal:SPL, prototype Countable public method count ]
 {

   - Parameters [0] {
   }

 Indeed, all Spl classes still use zend_parse_parameters_none().
 This work, because  count($a) doesn't use the method, but the
 count_elements handler.


Actually, the count_elements handler doesn't actually use the mode, so
that's another inconsistency; fixing that will cause a binary
incompatibility, though. Are we allowed to fix that so close to a release?

Unless the current changes are reverted, at the very least all SPL classes
that implement Countable must support the optional argument for `::count()`.


 If a internal class doesn't implement the handler, it will be hit by
 this change.

 Both solution should fix this:
 - adding the count_elements handler
 - accepting the optional mode


Not sure whether you meant to say that any of those solutions work, because
either only the latter is implemented or both; i.e. you can't implement
count_elements handler and then not accept the optional mode :)



 But, Reflection still not correct, and count($a) still raise the

 Warning: Imagick::count() expects exactly 0 parameters, 1 given in
 /work/GIT/imagick/tests/021-countable.phpt on line 19

 I think this is not consistent (internal vs userland)


Agreed!




 Remi.


 P.S. discovered on imagick
 See: https://github.com/mkoppanen/imagick/pull/35


 --
 PHP Internals - PHP Runtime Development Mailing List
 To unsubscribe, visit: http://www.php.net/unsub.php




-- 
--
Tjerk