Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:83940
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.215.43 as permitted sender)
MIME-Version: 1.0
Date: Thu, 26 Feb 2015 18:29:58 -0600
Message-ID: <CACouGvmRKX+qrexHfoW-9k0E9vW-ZPkRc6oz0vZ1g9CH7krQMQ@mail.gmail.com>
To: internals@lists.php.net
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Follow-up to STH user experience, this time with actual testing
From: matthew@zend.com ("Matthew Weier O'Phinney")

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 PHP=
Unit.
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 a=
n
integer. In this particular case, the value was coerced to an integer, albe=
it
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_ wor=
ks 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 force=
d 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 ha=
ving
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 PHPUni=
t was
executing! In this case, I discovered that:

- PHPUnit passes a boolean false to `debug_backtrace()`... which is documen=
ted
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is relyi=
ng
  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 EI=
THER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast boole=
an
  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_ wer=
e
declared in strict_types mode. In other words, it uncovered errors in calli=
ng
both userland and internal functions. Userland typehints worked as I expect=
ed,
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 (Ye=
s, 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 p=
atch
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 c=
ode
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 ag=
ainst
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 files =
in
your project. While it can be done with tools like sed and awk, realistical=
ly it
means that existing libraries cannot and/or will not benefit easily from th=
e
changes until they support PHP 7 as a minimum version. Additionally, it pos=
es
potential problems in terms of different testing results based on which mod=
e 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 affecte=
d.
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 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 wer=
e
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 librar=
y's
tests ran without problems once the PHPUnit issues were corrected (both cod=
e
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 t=
o
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!


--=20
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