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 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 sed and awk, > realistically it > means that existing libraries cannot and/or will not benefit easily from > the > changes until they support PHP 7 as a minimum version. Additionally, it > poses > potential problems in terms of different testing results based on which > mode you > run in, leading to ambiguity for users. The only way to address the latter > as a > library author is to specify which mode your code is known to run in. > > Ironically, I'm seeing STHcoerce as more strict than STHv5 in terms of type > handling, due to the fact that every call, userland or internal, is > affected. > The fact that it's always on makes it simpler for me to test against it > now, and > makes it easier to make my existing code both forward-compatible and more > correct — even if I do not add type hints until I update my minimum > supported > version to PHP 7. > > I am slightly worried about the amount of work needed to make code run > from v5 > to v7 if STHcoerce is adopted. That said, the issues I found in PHPUnit > were > corrected in minutes (though a more thorough patch that allows early > return from > methods if doc comments are not strings would be better), and my own > library's > tests ran without problems once the PHPUnit issues were corrected (both > code > with and without typehints). Strict_types is definitely interesting, but I > found > that strict_types mode was more or less how I wanted code to operate, and > to > opt-in to that means changes to every file in my library — which is a much > larger migration problem. > > In the end: kudos to everyone who is working on these patches and RFCs. I'm > excited about scalar type hints in PHP 7! > > > -- > Matthew Weier O'Phinney > Principal Engineer > Project Lead, Zend Framework and Apigility > matt...@zend.com > http://framework.zend.com > http://apigility.org > PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
