How did this end?
-Hannes
On Sun, Apr 13, 2014 at 1:50 PM, Bob Weinand wrote:
Am 13.4.2014 um 22:32 schrieb Stas Malyshev <smalyshev@sugarcrm.com>:
though adding new functions does not force anybody to change anything.
Same with new parameter to existing functions - see for example "Changes
to existing functions" section.
I thought UPGRADING would be only for new versions (not micros).
Looking at it again, I don't think we can put this patch in either 5.4
or 5.5. It changes extension API and breaks any module that implements
Countable because most of them expect 0 params and will produce warning
if given one param. Right now it broke Phar and SplObserver, but
third-party modules will have the same issue. Unless you find a way to
fix this in a generic way, we can only do it in 5.6, and even that only
with permission from Ferenc, since we're in the freeze now. In any case,
this means UPGRADING and probably UPGRADING.INTERNALS note is absolutely
necessary, as every extension having Countable classes will need to be
fixed.Btw. I really didn't think about the fact that zpp warns if there are too many
parameters. I think your concerns are valid; will ask Ferenc for PHP 5.6 (put him in CC)
(Although it's not worth a RFC, I should have PR that first…)
Bob
Am 13.4.2014 um 22:32 schrieb Stas Malyshev <smalyshev@sugarcrm.com>:
Hi!
function, starting with next version, you do. If you write code that
implements this function, you need to know it changed.
Also, this change seems to break a lot of tests, see:
https://travis-ci.org/php/php-src/builds/22895161
Looks like it is not as BC safe as it was intended. Please fix it or
revert the change - BC break in 5.4 is not acceptable.
The tests fail, because Zend checks for the number of parameters passed and warns then.an API change. The interface hasn't changed.
It has. Formerly, you did not have a parameter supplied for thisfunction, starting with next version, you do. If you write code that
implements this function, you need to know it changed.
Also, this change seems to break a lot of tests, see:
https://travis-ci.org/php/php-src/builds/22895161
Looks like it is not as BC safe as it was intended. Please fix it or
revert the change - BC break in 5.4 is not acceptable.
It'd only be an API change if it forced the user to change its declaration.
Not true. For example, we document new functions in UPGRADING, eventhough adding new functions does not force anybody to change anything.
Same with new parameter to existing functions - see for example "Changes
to existing functions" section.
I thought UPGRADING would be only for new versions (not micros).
Looking at it again, I don't think we can put this patch in either 5.4
or 5.5. It changes extension API and breaks any module that implements
Countable because most of them expect 0 params and will produce warning
if given one param. Right now it broke Phar and SplObserver, but
third-party modules will have the same issue. Unless you find a way to
fix this in a generic way, we can only do it in 5.6, and even that only
with permission from Ferenc, since we're in the freeze now. In any case,
this means UPGRADING and probably UPGRADING.INTERNALS note is absolutely
necessary, as every extension having Countable classes will need to be
fixed.
parameters. I think your concerns are valid; will ask Ferenc for PHP 5.6 (put him in CC)
This is, BTW, why we should have RFC and pull request before committing
such changes.
I really thought nothing bad could happen: a few lines which look absolutely clean...such changes.
(Although it's not worth a RFC, I should have PR that first…)
Bob
