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 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! > Can you gist the PHPUnit changes and point us to the coercive patch you compiled? Your results seem to be: 1. Weak mode in Dual Mode patch is too weak. 2. Coercive patch gives good feedback without strict. 3. Strict is too strict to apply to *all* your code retroactively that hasn't been designed for this from the beginning. Ironically if both RFCs are accepted you would get the best of your world, because Coercive STH is only a tightening of the rules for Weak mode of STHDual. > > -- > 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 > >
