Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:83942
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.47 as permitted sender)
MIME-Version: 1.0
References: <CACouGvmRKX+qrexHfoW-9k0E9vW-ZPkRc6oz0vZ1g9CH7krQMQ@mail.gmail.com>
Date: Fri, 27 Feb 2015 00:45:39 +0000
Message-ID: <CAHXmDMCSc7VPvWModAvXi8bjK_w16sc=suh2RxGEAWY0t8T4ow@mail.gmail.com>
To: "Matthew Weier O'Phinney" <matthew@zend.com>, internals@lists.php.net
Content-Type: multipart/alternative; boundary=001a11395a3006552705100730ed
Subject: Re: [PHP-DEV] Follow-up to STH user experience, this time with actual testing
From: rican7@gmail.com (Trevor Suarez)

--001a11395a3006552705100730ed
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 <matthew@zend.com>
wrote:

> I've taken some time the last couple days to compile both the Scalare Typ=
e
> 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, an=
d
> 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 err=
or
> 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 cod=
e
> 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 testin=
g
> 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 t=
o
>   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, an=
d
> 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/framewo=
rk
> 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 =E2=
=80=94
> 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 file=
s
> 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 latte=
r
> 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 ty=
pe
> 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 =E2=80=94 even if I do not add type hints until I update my minim=
um
> 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 =E2=80=94 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
> matthew@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
>
>

--001a11395a3006552705100730ed--