Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Matthew, Thanks a bunch for going through this and writing such a detailed report. On 27/02/2015 00:29, Matthew Weier O'Phinney wrote: ### STHv5 [snip] Analysis I did not expect the float value to be coerced, particularly as it had a fractional part. Yes, I understand that this is how casting _currently_ works in PHP, and that the patch uses the current casting rules. However, I'd expect that any float with a fractional value would not be cast to an integer; doing so is lossy, and can lead to unexpected results. The strict_types mode operated how I would expect it, but meant I was forced to add the declaration to the top of the file. Which leads to this: My tests operated differently in strict vs normal mode. That means my code does, too. Testing both modes would be difficult to do in an automated fashion; essentially, you're left needing to choose to support one mode or the other. This goes back to the point I was making earlier this week: I worry that having two modes will lead to a schism in userland libraries: those that support strict, and those that do not; there will be little possibility of testing both. I think this last bit is misguided. Your *tests* are running differently depending if they are in strict and weak mode, that is correct, but the code's behavior itself is not changing based on the test suite's strictness. That does not mean your library is tested to run in mode X or Y, it just means your test suite's correctness relies on testing the PHP type hints in this case, and it breaks when you change the way the hints behave. For users the code will work the same no matter if they are in strict or weak mode, but if they are in weak mode they should indeed watch out if they send floats they will be silently truncated as those are the rules of weak mode. In the end: kudos to everyone who is working on these patches and RFCs. I'm excited about scalar type hints in PHP 7! I agree with Benjamin's conclusion that having both RFCs combined might be for the best. At the very least lossy float-int should be disabled in the STHv5's weak mode because that's dangerous and unlikely to occur IMO. The other casting changes from STHcoerce I am not sure if they're for the best or not. I am a bit worried we change things arbitrarily based on who has a good argument not to change them, and we might end up with a completely inconsistent set of casting rules. Cheers -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Am 27.02.2015 um 01:29 schrieb Matthew Weier O'Phinney: - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) Pull requests for PHPUnit would be appreciated ;-) -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
On Fri, Feb 27, 2015 at 4:25 PM, Sebastian Bergmann sebast...@php.net wrote: Am 27.02.2015 um 01:29 schrieb Matthew Weier O'Phinney: - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) Pull requests for PHPUnit would be appreciated ;-) https://gist.github.com/beberlei/8a33ae940829f1186da2 - these are the necessary changes for dev-msater to run on php7+coercive patch -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Hey:
On Fri, Feb 27, 2015 at 3:59 PM, Xinchen Hui larue...@php.net wrote:
Hey:
On Fri, Feb 27, 2015 at 10:06 AM, François Laupretre franc...@php.net wrote:
De : Matthew Weier O'Phinney [mailto:matt...@zend.com]
- PHPUnit passes a boolean false to `debug_backtrace()`... which is
documented
as expecting an integer! (There are actually several constant values it
accepts, all of which are integer values.) In this case, PHPUnit is
relying
on the fact that the engine casts booleans to the integers 0 and 1. (Zeev
has
written to the list already indicating that this coercion path will be
supported in the patch.)
AFAIK, we won't support boolean to integer. IMO, considering Boolean as
integer is a bug and must not be supported.
in a internal developer's view:
bool false === 0, bool true == 1;
maybe this case should be supported?
considering such usage :
if (substr($str, strrpos($str, ':delimiter:')) {
}
this is not a really world usage, I just made it up, this codes is try
to find a last part of a string which is contacted with a dimileter
:delimiter:
sorry, the example is wrong, but I think you may get my idea.
a works example is:
find the first part:
substr($str, 0, strpos($str, :delimiter:))
which is common used,, right?
thanks
like a:delimiter:b, c:dilimiter:d
it works well before, because, if there is no match, it returns false
before.. (which we will think it's '0')
then the whole $str is returned..
but if we don't support bool false - 0?
thanks
- PHPUnit is passing the results of $reflector-getDocComment() blindly to
substr() and preg_match*(). getDocComment() is documented as returning
EITHER
a string OR boolean false. Again, PHPUnit is relying on PHP to cast
boolean
false to an empty string. (Zeev has also indicated this coercion path may
be
re-introduced.)
The same as above for bool - string.
I hope you're wrong because I wouldn't like supporting boolean to scalar
back again.
Your test demonstrates this because you found undetected bugs. I am more and
more sure that, what I first said as a joke, will prove true : during the
next years, STH will be used mostly as a debugging tool, proving opposite
arguments were FUD or, at least, phantasm.
Regards
François
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
--
Xinchen Hui
@Laruence
http://www.laruence.com/
--
Xinchen Hui
@Laruence
http://www.laruence.com/
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Matthew, On Fri, Feb 27, 2015 at 1:29 AM, Matthew Weier O'Phinney matt...@zend.com wrote: I've taken some time the last couple days to compile both the Scalare Type Hints v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore STHcoerce) patches and test some code against them. In each case, I modified the `Phly\Http\Response` class from my phly/http package to add scalar type hints, remove previous argument validation, and then ran the unit tests. Here's my details of the experience, and analysis. ### STHv5 With STHv5, my tests failed out of the box. First, I needed to add an error handler that would convert the E_RECOVERABLE_ERROR to an InvalidArgumentException so that I would get useful error messages from PHPUnit. Next, once I had done that, I had tests that were throwing invalid input at methods, and validating that the invalid arguments were caught. This worked in all but one specific case: passing a float value to an argument expecting an integer. In this particular case, the value was coerced to an integer, albeit lossily, and no error was raised. When I changed my test case to operate under strict_types mode, the test executed as I had originally expected, and succeeded. Analysis I did not expect the float value to be coerced, particularly as it had a fractional part. Yes, I understand that this is how casting _currently_ works in PHP, and that the patch uses the current casting rules. However, I'd expect that any float with a fractional value would not be cast to an integer; doing so is lossy, and can lead to unexpected results. The strict_types mode operated how I would expect it, but meant I was forced to add the declaration to the top of the file. Which leads to this: My tests operated differently in strict vs normal mode. That means my code does, too. Testing both modes would be difficult to do in an automated fashion; essentially, you're left needing to choose to support one mode or the other. This goes back to the point I was making earlier this week: I worry that having two modes will lead to a schism in userland libraries: those that support strict, and those that do not; there will be little possibility of testing both. ### STHcoerce With STHcoerce, my tests also failed out of the box, but not for the same reason. Instead, I had a bunch of errors reported based on code that PHPUnit was executing! In this case, I discovered that: - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) I was able to fix these in a matter of minutes, and then my tests ran, and passed without any changes. Analysis STHcoerce worked about how I expect STHv5 will work _if_ _every_ _file_ were declared in strict_types mode. In other words, it uncovered errors in calling both userland and internal functions. Userland typehints worked as I expected, with floats using fractional values not casting to integers. ### Wrap Up STHcoerce was actually far more strict than I found strict_types mode to be in STHv5. The reason is that it's a single, non-optional mode. This poses a potential challenge to migration, as noted in my problems using PHPUnit (Yes, I WILL be sending patches their way soon!): method calls and arguments that previously worked because of how PHP juggles types often do not work, particularly when going from boolean to other scalar values. However, the patch does what I'd expect in terms of preventing operations that would result in either data loss or data additions, all of which can have unexpected side effects if you don't understand the casting rules currently. The flip side to this is that you do not need to make any changes to your code in order to locate and fix errors. What this means from a library/framework author perspective is that, if the STHcoerce patch is merged, I can test against PHP 7, make fixes, and my code is not only forward-compatible, but also backwards to the various PHP 5 versions I might already be supporting — and that code now benefits from being more correct. Because STHv5 is opt-in only, the only way to see similar type errors is to somehow automate the addition of the strict_types declaration to all files in your project. While it can be done with tools like
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Hey:
On Fri, Feb 27, 2015 at 10:06 AM, François Laupretre franc...@php.net wrote:
De : Matthew Weier O'Phinney [mailto:matt...@zend.com]
- PHPUnit passes a boolean false to `debug_backtrace()`... which is
documented
as expecting an integer! (There are actually several constant values it
accepts, all of which are integer values.) In this case, PHPUnit is relying
on the fact that the engine casts booleans to the integers 0 and 1. (Zeev
has
written to the list already indicating that this coercion path will be
supported in the patch.)
AFAIK, we won't support boolean to integer. IMO, considering Boolean as
integer is a bug and must not be supported.
in a internal developer's view:
bool false === 0, bool true == 1;
maybe this case should be supported?
considering such usage :
if (substr($str, strrpos($str, ':delimiter:')) {
}
this is not a really world usage, I just made it up, this codes is try
to find a last part of a string which is contacted with a dimileter
:delimiter:
like a:delimiter:b, c:dilimiter:d
it works well before, because, if there is no match, it returns false
before.. (which we will think it's '0')
then the whole $str is returned..
but if we don't support bool false - 0?
thanks
- PHPUnit is passing the results of $reflector-getDocComment() blindly to
substr() and preg_match*(). getDocComment() is documented as returning
EITHER
a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean
false to an empty string. (Zeev has also indicated this coercion path may
be
re-introduced.)
The same as above for bool - string.
I hope you're wrong because I wouldn't like supporting boolean to scalar back
again.
Your test demonstrates this because you found undetected bugs. I am more and
more sure that, what I first said as a joke, will prove true : during the
next years, STH will be used mostly as a debugging tool, proving opposite
arguments were FUD or, at least, phantasm.
Regards
François
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
--
Xinchen Hui
@Laruence
http://www.laruence.com/
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
- PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) Matthew, I just wanted to point out that actually, supporting boolean-int coercion is not planned. Converting boolean to integer is actually a very likely scenario where you'd have an outcome that you didn't expect. It's debatable whether sending false to a bitmask is OK or not, but given it's a bitmask, an explicit 0 is a lot more correct. - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) We're looking to re-introduce string-boolean (which will likely be common in database scenarios), but not the other way around - where it's very likely hiding a bug. Virtually all of the boolean-string coercions that were flagged as deprecated by the patch were at the very least suspects of being real world bugs. Thanks! Zeev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
Thank you so much for taking the time to do this. Your explanations of a more real-world example are extremely valuable and provide a very strong hint at the effects that these RFC implementations may have, both in the short and long term. Seriously, very appreciated. On Thu, Feb 26, 2015 at 7:30 PM Matthew Weier O'Phinney matt...@zend.com wrote: I've taken some time the last couple days to compile both the Scalare Type Hints v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore STHcoerce) patches and test some code against them. In each case, I modified the `Phly\Http\Response` class from my phly/http package to add scalar type hints, remove previous argument validation, and then ran the unit tests. Here's my details of the experience, and analysis. ### STHv5 With STHv5, my tests failed out of the box. First, I needed to add an error handler that would convert the E_RECOVERABLE_ERROR to an InvalidArgumentException so that I would get useful error messages from PHPUnit. Next, once I had done that, I had tests that were throwing invalid input at methods, and validating that the invalid arguments were caught. This worked in all but one specific case: passing a float value to an argument expecting an integer. In this particular case, the value was coerced to an integer, albeit lossily, and no error was raised. When I changed my test case to operate under strict_types mode, the test executed as I had originally expected, and succeeded. Analysis I did not expect the float value to be coerced, particularly as it had a fractional part. Yes, I understand that this is how casting _currently_ works in PHP, and that the patch uses the current casting rules. However, I'd expect that any float with a fractional value would not be cast to an integer; doing so is lossy, and can lead to unexpected results. The strict_types mode operated how I would expect it, but meant I was forced to add the declaration to the top of the file. Which leads to this: My tests operated differently in strict vs normal mode. That means my code does, too. Testing both modes would be difficult to do in an automated fashion; essentially, you're left needing to choose to support one mode or the other. This goes back to the point I was making earlier this week: I worry that having two modes will lead to a schism in userland libraries: those that support strict, and those that do not; there will be little possibility of testing both. ### STHcoerce With STHcoerce, my tests also failed out of the box, but not for the same reason. Instead, I had a bunch of errors reported based on code that PHPUnit was executing! In this case, I discovered that: - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) I was able to fix these in a matter of minutes, and then my tests ran, and passed without any changes. Analysis STHcoerce worked about how I expect STHv5 will work _if_ _every_ _file_ were declared in strict_types mode. In other words, it uncovered errors in calling both userland and internal functions. Userland typehints worked as I expected, with floats using fractional values not casting to integers. ### Wrap Up STHcoerce was actually far more strict than I found strict_types mode to be in STHv5. The reason is that it's a single, non-optional mode. This poses a potential challenge to migration, as noted in my problems using PHPUnit (Yes, I WILL be sending patches their way soon!): method calls and arguments that previously worked because of how PHP juggles types often do not work, particularly when going from boolean to other scalar values. However, the patch does what I'd expect in terms of preventing operations that would result in either data loss or data additions, all of which can have unexpected side effects if you don't understand the casting rules currently. The flip side to this is that you do not need to make any changes to your code in order to locate and fix errors. What this means from a library/framework author perspective is that, if the STHcoerce patch is merged, I can test against PHP 7, make fixes, and my code is not only forward-compatible, but also backwards to the various PHP 5 versions I might already be supporting — and
Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
On Thu, Feb 26, 2015 at 4:29 PM, Matthew Weier O'Phinney matt...@zend.com wrote: I've taken some time the last couple days to compile both the Scalare Type Hints v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore STHcoerce) patches and test some code against them. Thanks for this detailed report, much appreciated! In each case, I modified the `Phly\Http\Response` class from my phly/http package to add scalar type hints, remove previous argument validation, and then ran the unit tests. Here's my details of the experience, and analysis. ### STHv5 With STHv5, my tests failed out of the box. First, I needed to add an error handler that would convert the E_RECOVERABLE_ERROR to an InvalidArgumentException so that I would get useful error messages from PHPUnit. Next, once I had done that, I had tests that were throwing invalid input at methods, and validating that the invalid arguments were caught. This worked in all but one specific case: passing a float value to an argument expecting an integer. In this particular case, the value was coerced to an integer, albeit lossily, and no error was raised. When I changed my test case to operate under strict_types mode, the test executed as I had originally expected, and succeeded. Analysis I did not expect the float value to be coerced, particularly as it had a fractional part. Yes, I understand that this is how casting _currently_ works in PHP, and that the patch uses the current casting rules. However, I'd expect that any float with a fractional value would not be cast to an integer; doing so is lossy, and can lead to unexpected results. The strict_types mode operated how I would expect it, but meant I was forced to add the declaration to the top of the file. Which leads to this: My tests operated differently in strict vs normal mode. That means my code does, too. Testing both modes would be difficult to do in an automated fashion; essentially, you're left needing to choose to support one mode or the other. This goes back to the point I was making earlier this week: I worry that having two modes will lead to a schism in userland libraries: those that support strict, and those that do not; there will be little possibility of testing both. It seems you still are still confused about that one :) This opt-in mode is the key value of this RFC. Nothing changes if you do not care and do not want to care. And if you care and want to port part of your code, you can, being fully informed about what it means for this file. ### STHcoerce With STHcoerce, my tests also failed out of the box, but not for the same reason. Instead, I had a bunch of errors reported based on code that PHPUnit was executing! In this case, I discovered that: - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) And now let wait all other cases not covered by errors but casting to different values, maybe only a few, maybe none, we simply do not know. Cheers, -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
De : Matthew Weier O'Phinney [mailto:matt...@zend.com] - PHPUnit passes a boolean false to `debug_backtrace()`... which is documented as expecting an integer! (There are actually several constant values it accepts, all of which are integer values.) In this case, PHPUnit is relying on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has written to the list already indicating that this coercion path will be supported in the patch.) AFAIK, we won't support boolean to integer. IMO, considering Boolean as integer is a bug and must not be supported. - PHPUnit is passing the results of $reflector-getDocComment() blindly to substr() and preg_match*(). getDocComment() is documented as returning EITHER a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean false to an empty string. (Zeev has also indicated this coercion path may be re-introduced.) The same as above for bool - string. I hope you're wrong because I wouldn't like supporting boolean to scalar back again. Your test demonstrates this because you found undetected bugs. I am more and more sure that, what I first said as a joke, will prove true : during the next years, STH will be used mostly as a debugging tool, proving opposite arguments were FUD or, at least, phantasm. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
De : Pierre Joye [mailto:pierre@gmail.com] And now let wait all other cases not covered by errors but casting to different values, maybe only a few, maybe none, we simply do not know. Pierre, excuse me to repeat here, because I jst replied the same in another thread but there is no 'casting to different values'. That's why I didn't understand when you adked about failures on different casting. This is a pure subset. We diable cases, but absolutely don't change conversions. I agree that changing conversion rules would be terrible. It is absolutely not the case. Our only fault is to have made this not clear enough in the RFC. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
