Greetings!
My team and I (which means Ralph Schindler and Enrico Zimuel) took some
time this week to:
- Build PHP 5.4.0RC1 and run make tests (and send feedback)
- Run Zend Framework 1.11 unit tests against PHP 5.4.0RC1
- Run Zend Framework 2.0 (dev) unit tests against PHP 5.4.0RC1
We found the following issues when running the tests, and I've ordered
them starting with those that affect us most. Overall, however, 5.4.0RC1
largely worked out of the box.
-
Array to string conversion notices break tests
In 5.2 and 5.3, array to string conversions raise no notices, and as
such allow the tests to continue to run. Now, however, a notice is
raised, and PHPUnit then raises an error.While I understand we should likely test values before using them in
string contexts, this feels like an arbitrary change at this time.I recall from earlier discussions that Stas was in favor of reverting
this change -- what's the status at this time? This single change
will likely require a fair bit of time for us to fix across our ZF
versions if not reverted, and I suspect it will hit a lot of other
projects. -
ob_get_status(true) does not always return an array
One of our cache adapters uses ob_get_status(true) to determine
things like nesting levels. However, despite the fact that we're
passing the boolean true value to force return of an array, we get a
non-array when the output buffer is empty now -- and this is true
only of 5.4.0RC1. With the $full_status argument, I'd argue this
should always return an array, even if that array is empty -- which
is precisely how it's documented. I see no reason for the change in
5.4. -
ob_get_clean()now raises a notice if no buffer to delete
Prior to 5.4.0RC1, if there were no buffers left to delete,
ob_get_clean()was silent. It now raises a notice -- which, when
using PHPUnit, means an error is now raised, making the test fail.
I do not see any benefit to raising the notice; if there's nothing
left to clean, return an empty string or false (which was the former
behavior, and is documented). -
Redefining a constructor with different arguments now results in E_FATAL
Prior to 5.4.0RC1, if a concrete class added arguments to a
constructor defined in an abstract class, no warning was raised. This
behavior now results in an E_FATAL. I'm ambivalent about this change,
however -- the code that raised this for us should be changed
anyways.However, I'll argue, as others have on the list, that constructors
should have the flexibility of redefinition without raising notices
or compilation errors. -
Introduction of the new keyword "callable" means that this can no
longer be used for classnames, function names, or method names.We had one case where a "callable()" method was defined, and this now
breaks; fortunately, it's only in tests, so we can easily fix it with
no BC issues on our part. I'll note that I've also used the
class|interface name "Callable" in the past on other projects.I'm okay with this change, but it needs to be clearly spelled out in
the migration docs.
--
Matthew Weier O'Phinney
Project Lead | matthew@zend.com
Zend Framework | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Hi!
I recall from earlier discussions that Stas was in favor of reverting this change -- what's the status at this time? This single change
I was not and still am not - I think if something warrants notice this
is exactly the case. Conversion of array to string "Array" IMHO makes no
sense and not useful in any case, so if you code does that it's most
probably a bug. I understand that some tests were written without
accounting for this, but frankly in this case I think the right way
would be to fix the tests. This is not the feature your code should rely
on (and I think it should have never existed in PHP in the first place)
and if we shall guarantee compatibility with every bug and bad decision
we took in the past we won't be able to advance anywhere. I think in
this case the inconvenience from fixing the tests is outweighed by the
advantage of promoting better code and exposing hard-to-find bugs.
- ob_get_status(true) does not always return an array
According to docs, ob_get_status(true) should return array, so it looks
like a bug. Please submit a bug report.
ob_get_clean()now raises a notice if no buffer to delete
This is where I say the notice is totally unneeded. Please submit a bug
for it, it should be silent and return false, just as the docs say.
Redefining a constructor with different arguments now results in E_FATAL
Prior to 5.4.0RC1, if a concrete class added arguments to a
constructor defined in an abstract class, no warning was raised. This
behavior now results in an E_FATAL. I'm ambivalent about this change,
however -- the code that raised this for us should be changed
anyways.However, I'll argue, as others have on the list, that constructors
should have the flexibility of redefinition without raising notices
or compilation errors.
Here I agree with you, but looks like the majority disagrees. Though I'm
not sure, frankly, what the point of defining ctor in the abstract class
is, especially if argument list doesn't stay constant. Maybe clarifying
this would change the opinion here.
Introduction of the new keyword "callable" means that this can no
longer be used for classnames, function names, or method names.We had one case where a "callable()" method was defined, and this now
breaks; fortunately, it's only in tests, so we can easily fix it with
no BC issues on our part. I'll note that I've also used the
class|interface name "Callable" in the past on other projects.I'm okay with this change, but it needs to be clearly spelled out in
the migration docs.
I will update UPGRADING, thanks. The existence of the callable hint is
documented, but not the fact that it's reserved now.
Thanks for the testing!
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
I recall from earlier discussions that Stas was in favor of reverting this change -- what's the status at this time? This single changeI was not and still am not - I think if something warrants notice this
is exactly the case. Conversion of array to string "Array" IMHO makes no
sense and not useful in any case, so if you code does that it's most
probably a bug. I understand that some tests were written without
accounting for this, but frankly in this case I think the right way
would be to fix the tests. This is not the feature your code should rely
on (and I think it should have never existed in PHP in the first place)
and if we shall guarantee compatibility with every bug and bad decision
we took in the past we won't be able to advance anywhere. I think in
this case the inconvenience from fixing the tests is outweighed by the
advantage of promoting better code and exposing hard-to-find bugs.
I can live with this.
- ob_get_status(true) does not always return an array
According to docs, ob_get_status(true) should return array, so it looks
like a bug. Please submit a bug report.
Done: https://bugs.php.net/bug.php?id=60321
ob_get_clean()now raises a notice if no buffer to deleteThis is where I say the notice is totally unneeded. Please submit a bug
for it, it should be silent and return false, just as the docs say.
Done: https://bugs.php.net/bug.php?id=60322
Redefining a constructor with different arguments now results in
E_FATAL Prior to 5.4.0RC1, if a concrete class added arguments
to a constructor defined in an abstract class, no warning was
raised. This behavior now results in an E_FATAL. I'm ambivalent
about this change, however -- the code that raised this for us
should be changed anyways.However, I'll argue, as others have on the list, that
constructors should have the flexibility of redefinition without
raising notices or compilation errors.Here I agree with you, but looks like the majority disagrees. Though I'm
not sure, frankly, what the point of defining ctor in the abstract class
is, especially if argument list doesn't stay constant. Maybe clarifying
this would change the opinion here.
Ralph has done this in a separate thread now.
Introduction of the new keyword "callable" means that this can no
longer be used for classnames, function names, or method names.We had one case where a "callable()" method was defined, and this now
breaks; fortunately, it's only in tests, so we can easily fix it with
no BC issues on our part. I'll note that I've also used the
class|interface name "Callable" in the past on other projects.I'm okay with this change, but it needs to be clearly spelled out in
the migration docs.I will update UPGRADING, thanks. The existence of the callable hint is
documented, but not the fact that it's reserved now.
Excellent, thanks.
Thanks for the testing!
Thanks for the release! :)
--
Matthew Weier O'Phinney
Project Lead | matthew@zend.com
Zend Framework | http://framework.zend.com/
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc
Hi!
I recall from earlier discussions that Stas was in favor of reverting this change -- what's the status at this time? This single changeI was not and still am not - I think if something warrants notice this
is exactly the case. Conversion of array to string "Array" IMHO makes no
sense and not useful in any case, so if you code does that it's most
probably a bug. I understand that some tests were written without
accounting for this, but frankly in this case I think the right way
would be to fix the tests. This is not the feature your code should rely
on (and I think it should have never existed in PHP in the first place)
and if we shall guarantee compatibility with every bug and bad decision
we took in the past we won't be able to advance anywhere. I think in
this case the inconvenience from fixing the tests is outweighed by the
advantage of promoting better code and exposing hard-to-find bugs.
I completely agree. If you are seeing a lot of these in unit tests, then
your tests are likely not testing what you think they are.
I am seeing complaints about this notice in tests that do this:
array_diff($a, $b)
Where either $a or $b contains nested arrays. The thing with this is
that this really is not doing what you think it is when you feed it
nested arrays. For example:
$a = [1,2,[3]];
$b = [1,2,[4]];
This ends up comparing [1,2,'Array'] against [1,2,'Array'] and these two
arrays are thus identical as far as array_diff() is concerned. Or even
worse:
$a = [1,2,'Array'];
$b = [1,2,[4,5,6,7,8,9]];
Again, array_diff() is going to tell you that there is no difference
between $a and $b. This seems like an obvious case where we issue a
NOTICE telling the user that the code probably didn't do what they
wanted it to.
-Rasmus
2011/11/17 Rasmus Lerdorf rasmus@php.net:
Hi!
I recall from earlier discussions that Stas was in favor of reverting
this change -- what's the status at this time? This single changeI was not and still am not - I think if something warrants notice this
is exactly the case. Conversion of array to string "Array" IMHO makes no
sense and not useful in any case, so if you code does that it's most
probably a bug. I understand that some tests were written without
accounting for this, but frankly in this case I think the right way
would be to fix the tests. This is not the feature your code should rely
on (and I think it should have never existed in PHP in the first place)
and if we shall guarantee compatibility with every bug and bad decision
we took in the past we won't be able to advance anywhere. I think in
this case the inconvenience from fixing the tests is outweighed by the
advantage of promoting better code and exposing hard-to-find bugs.I completely agree. If you are seeing a lot of these in unit tests, then
your tests are likely not testing what you think they are.I am seeing complaints about this notice in tests that do this:
array_diff($a, $b)
Where either $a or $b contains nested arrays. The thing with this is
that this really is not doing what you think it is when you feed it
nested arrays. For example:$a = [1,2,[3]];
$b = [1,2,[4]];This ends up comparing [1,2,'Array'] against [1,2,'Array'] and these two
arrays are thus identical as far asarray_diff()is concerned. Or even
worse:$a = [1,2,'Array'];
$b = [1,2,[4,5,6,7,8,9]];Again,
array_diff()is going to tell you that there is no difference
between $a and $b. This seems like an obvious case where we issue a
NOTICE telling the user that the code probably didn't do what they
wanted it to.-Rasmus
Stas and Rasmus summarized my point of view very well when I proposed
that patch.
Note that this change should have be a one liner, but it turns out to
be a much bigger work [1] because of internal tests also relying on
this conversion not to complain.
[1] http://svn.php.net/viewvc?view=revision&revision=318288
--
Patrick
Hi Matthew, my replies are inline
On Thu, Nov 17, 2011 at 9:46 PM, Matthew Weier O'Phinney <
weierophinney@php.net> wrote:
Greetings!
My team and I (which means Ralph Schindler and Enrico Zimuel) took some
time this week to:
- Build PHP 5.4.0RC1 and run make tests (and send feedback)
- Run Zend Framework 1.11 unit tests against PHP 5.4.0RC1
- Run Zend Framework 2.0 (dev) unit tests against PHP 5.4.0RC1
We found the following issues when running the tests, and I've ordered
them starting with those that affect us most. Overall, however, 5.4.0RC1
largely worked out of the box.
nice
- Array to string conversion notices break tests
In 5.2 and 5.3, array to string conversions raise no notices, and as
such allow the tests to continue to run. Now, however, a notice is
raised, and PHPUnit then raises an error.While I understand we should likely test values before using them in
string contexts, this feels like an arbitrary change at this time.I recall from earlier discussions that Stas was in favor of reverting
this change -- what's the status at this time? This single change
will likely require a fair bit of time for us to fix across our ZF
versions if not reverted, and I suspect it will hit a lot of other
projects.
We didn't see one sane use-case where it is intended to compare an array as
'Array' to another string, so I think that albeit you aren't the only ones
bumping into this, but usually this means broken code/tests.
Chx also reported some drupal tests failing because of this change:
https://bugs.php.net/bug.php?id=60198 however as Rasmus explained that was
a bug on their part (they were expecting that array_diff/array_intersect
would correctly work with nested arrays), and it is sheer luck that they
are getting the "expected" results.
- ob_get_status(true) does not always return an array
One of our cache adapters uses ob_get_status(true) to determine
things like nesting levels. However, despite the fact that we're
passing the boolean true value to force return of an array, we get a
non-array when the output buffer is empty now -- and this is true
only of 5.4.0RC1. With the $full_status argument, I'd argue this
should always return an array, even if that array is empty -- which
is precisely how it's documented. I see no reason for the change in
5.4.
Agree.
ob_get_clean()now raises a notice if no buffer to delete
Prior to 5.4.0RC1, if there were no buffers left to delete,
ob_get_clean()was silent. It now raises a notice -- which, when
using PHPUnit, means an error is now raised, making the test fail.
I do not see any benefit to raising the notice; if there's nothing
left to clean, return an empty string or false (which was the former
behavior, and is documented).
Agree, the new behavior maybe a little bit more explicit, but I think it is
not intentional, just a side effect, but I can be wrong ofc.
- Redefining a constructor with different arguments now results in E_FATAL
Prior to 5.4.0RC1, if a concrete class added arguments to a
constructor defined in an abstract class, no warning was raised. This
behavior now results in an E_FATAL. I'm ambivalent about this change,
however -- the code that raised this for us should be changed
anyways.However, I'll argue, as others have on the list, that constructors
should have the flexibility of redefinition without raising notices
or compilation errors.
Yeah, this change is mentioned here:
https://bugs.php.net/bug.php?id=55085it was only a fix for
https://bugs.php.net/bug.php?id=51421
We have a lengthy discussion about this change as it is a little bit too
drastic: it prevents some sane use-cases, and triggering E_FATAL is also a
little bit too sever.
For the record here is a link for the thread on the mailing list:
internals@lists.php.net/msg53561.html" rel="nofollow" target="_blank">http://www.mail-archive.com/internals@lists.php.net/msg53561.html
and Etienne Kneuss wrote an RFC on the current (as of 5.4) behavior which
could be used as a base to what should we change/refine:
https://wiki.php.net/rfc/prototype_checks
As I mentioned in the original thread I would like to see both the
restricted cases and the error level relaxed.
- Introduction of the new keyword "callable" means that this can no
longer be used for classnames, function names, or method names.We had one case where a "callable()" method was defined, and this now
breaks; fortunately, it's only in tests, so we can easily fix it with
no BC issues on our part. I'll note that I've also used the
class|interface name "Callable" in the past on other projects.I'm okay with this change, but it needs to be clearly spelled out in
the migration docs.
Agree.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
ob_get_clean()now raises a notice if no buffer to delete
Prior to 5.4.0RC1, if there were no buffers left to delete,
ob_get_clean()was silent. It now raises a notice -- which, when
using PHPUnit, means an error is now raised, making the test fail.
I do not see any benefit to raising the notice; if there's nothing
left to clean, return an empty string or false (which was the former
behavior, and is documented).
I've looked into this for the purpose of coming up with a patch, and judging from the tests and also behavior in PHP 5.3, it doesn't seem that simple.
First of all, ob_clean() and ob_end_clean() will raise the same notice even in PHP 5.3. It seems inconsistent to me to treat these three differently, so in that regard, PHP 5.4 is actually fixing behavior (although arguably the other way to approach the problem is to remove the notice from all three of them).
Also, the documentation states for ob_get_clean():
"ob_get_clean() essentially executes both ob_get_contents() and ob_end_clean()."
Since ob_end_clean() currently raises the notice, it'd not be unreasonable to expect it for ob_get_clean() as well.
David
Hi!
First of all,
ob_clean()andob_end_clean()will raise the same
notice even in PHP 5.3. It seems inconsistent to me to treat these
three differently, so in that regard, PHP 5.4 is actually fixing
behavior (although arguably the other way to approach the problem is
to remove the notice from all three of them).
I don't think ob_end_clean() should produce notices, otherwise it leads
to needless boilerplate code like:
if(ob_get_evel()>0) ob_end_clean();
while you could just write ob_end_clean() and be done with it.
ob_clean() is trickier since it's supposed to leave buffer in place, but
for functions that are supposed to remove the buffer warning doesn't add
any value, only makes people write uglier code.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
I agree with Stas' point there is really no need to force people do
the while (ob_get_level()) ob_end_clean(); loop or "force" people to
use the @ob_end_clean(); to avoid notices. If there are no buffers to
clear it should be a noop, without any notices being raised.
Hi!
First of all,
ob_clean()andob_end_clean()will raise the same
notice even in PHP 5.3. It seems inconsistent to me to treat these
three differently, so in that regard, PHP 5.4 is actually fixing
behavior (although arguably the other way to approach the problem is
to remove the notice from all three of them).I don't think
ob_end_clean()should produce notices, otherwise it leads
to needless boilerplate code like:if(ob_get_evel()>0)
ob_end_clean();while you could just write
ob_end_clean()and be done with it.
ob_clean()is trickier since it's supposed to leave buffer in place, but
for functions that are supposed to remove the buffer warning doesn't add
any value, only makes people write uglier code.Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
So the consensus so far is to drop notices from ob_clean(), ob_end_clean() and ob_get_clean()? What about ob_flush() or others?
David
I agree with Stas' point there is really no need to force people do
the while (ob_get_level())ob_end_clean(); loop or "force" people to
use the @ob_end_clean(); to avoid notices. If there are no buffers to
clear it should be a noop, without any notices being raised.Hi!
First of all,
ob_clean()andob_end_clean()will raise the same
notice even in PHP 5.3. It seems inconsistent to me to treat these
three differently, so in that regard, PHP 5.4 is actually fixing
behavior (although arguably the other way to approach the problem is
to remove the notice from all three of them).I don't think
ob_end_clean()should produce notices, otherwise it leads
to needless boilerplate code like:if(ob_get_evel()>0)
ob_end_clean();while you could just write
ob_end_clean()and be done with it.
ob_clean()is trickier since it's supposed to leave buffer in place, but
for functions that are supposed to remove the buffer warning doesn't add
any value, only makes people write uglier code.Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
same here, and for any other places in the ob_* APIs. Functions
returns false on error, cleaning something already cleaned or empty is
not an erro.
I agree with Stas' point there is really no need to force people do
the while (ob_get_level()) ob_end_clean(); loop or "force" people to
use the @ob_end_clean(); to avoid notices. If there are no buffers to
clear it should be a noop, without any notices being raised.Hi!
First of all,
ob_clean()andob_end_clean()will raise the same
notice even in PHP 5.3. It seems inconsistent to me to treat these
three differently, so in that regard, PHP 5.4 is actually fixing
behavior (although arguably the other way to approach the problem is
to remove the notice from all three of them).I don't think
ob_end_clean()should produce notices, otherwise it leads
to needless boilerplate code like:if(ob_get_evel()>0)
ob_end_clean();while you could just write
ob_end_clean()and be done with it.
ob_clean()is trickier since it's supposed to leave buffer in place, but
for functions that are supposed to remove the buffer warning doesn't add
any value, only makes people write uglier code.Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227--
--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
2011/11/18 Pierre Joye pierre.php@gmail.com:
same here, and for any other places in the ob_* APIs. Functions
returns false on error, cleaning something already cleaned or empty is
not an erro.
Same opinion here.
--
Regards,
Felipe Pena
2011/11/18 Pierre Joye pierre.php@gmail.com:
same here, and for any other places in the ob_* APIs. Functions
returns false on error, cleaning something already cleaned or empty is
not an erro.Same opinion here.
WOW. All the developers agreeing with a common goal.
Go on. I dare any one of you to spoil my good mood.
I handed my notice in today. Off to work for Fantasy Shopper in the
New Year. Very excited. I know most (if not all) of you don't know who
I am, but hey, it's Friday and tonight is insane drinking until you
fall over night!
Have a good weekend everyone. I know I will.
--
Richard Quadling
Twitter : EE : Zend : PHPDoc : Fantasy Shopper
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea :
fan.sh/6/370
On Fri, Nov 18, 2011 at 5:40 PM, Richard Quadling rquadling@gmail.comwrote:
2011/11/18 Pierre Joye pierre.php@gmail.com:
same here, and for any other places in the ob_* APIs. Functions
returns false on error, cleaning something already cleaned or empty is
not an erro.Same opinion here.
WOW. All the developers agreeing with a common goal.
it happens. :)
Go on. I dare any one of you to spoil my good mood.
I handed my notice in today. Off to work for Fantasy Shopper in the
New Year. Very excited. I know most (if not all) of you don't know who
I am, but hey, it's Friday and tonight is insane drinking until you
fall over night!Have a good weekend everyone. I know I will.
good luck with your next gig!
(if I would guessed, I would said that your next job will be some official
position at Experts Exchange.)
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
On Fri, Nov 18, 2011 at 5:40 PM, Richard Quadling rquadling@gmail.com
wrote:2011/11/18 Pierre Joye pierre.php@gmail.com:
same here, and for any other places in the ob_* APIs. Functions
returns false on error, cleaning something already cleaned or empty is
not an erro.Same opinion here.
WOW. All the developers agreeing with a common goal.
it happens. :)
Go on. I dare any one of you to spoil my good mood.
I handed my notice in today. Off to work for Fantasy Shopper in the
New Year. Very excited. I know most (if not all) of you don't know who
I am, but hey, it's Friday and tonight is insane drinking until you
fall over night!Have a good weekend everyone. I know I will.
good luck with your next gig!
(if I would guessed, I would said that your next job will be some official
position at Experts Exchange.)Ferenc Kovács
@Tyr43l - http://tyrael.hu
No. I'm going to be running the back end project of the new social
gaming site Fantasy Shopper (http://fan.sh/6/370).
High Street and Fashion Retailing meets social gaming for real world value.
New, exciting and very very interesting. Lots of scope for growth. New
ideas, integration into different media/technologies. Gamification of
shopping but with the real shops and real products.
We just won the Amazon Web Services StartUp Challenge 2011. The first
non US company to ever win it. http://aws.amazon.com/startupchallenge/
We are all extremely pleased with ourselves.
And it is a great opportunity for me too. So feeling really good.
Richard.
P.S. Experts Exchange would have been great too, but I don't know if
it is really in the same league.
--
Richard Quadling
Twitter : EE : Zend : PHPDoc : Fantasy Shopper
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea :
fan.sh/6/370