Hi Matthew, my replies are inline On Thu, Nov 17, 2011 at 9:46 PM, Matthew Weier O'Phinney < weierophin...@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: 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
