Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87753
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Davey Shafik'" <davey@php.net>
Cc: <internals@lists.php.net>,
	"'Nikita Popov'" <nikic@php.net>,
	"'Dmitry Stogov'" <dmitry@php.net>
References: <CANaX5ndkK1O2joJKj5HKh-XxPw3ZHLpdZpKyg_qSZDYiW_szfg@mail.gmail.com>	<003401d0d66d$385af9d0$a910ed70$@belski.net> <CANaX5ndN-6QbMYQk0=UVxxXR=Xnpiybb0yvdj2VDmN=Lq5ijLw@mail.gmail.com>
In-Reply-To: <CANaX5ndN-6QbMYQk0=UVxxXR=Xnpiybb0yvdj2VDmN=Lq5ijLw@mail.gmail.com>
Date: Fri, 14 Aug 2015 16:56:53 +0200
Message-ID: <009f01d0d6a1$7728b6a0$657a23e0$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQMRFRSX5VL4TX2hWWiGtsXSI+iT/wE755U6AnXnmsmbbWo2oA==
Content-Language: en-us
Subject: RE: [PHP-DEV] Warning (5.x) -> TypeError (7) for internal Constructors
From: anatol.php@belski.net ("Anatol Belski")


> -----Original Message-----
> From: me@daveyshafik.com [mailto:me@daveyshafik.com] On Behalf Of =
Davey
> Shafik
> Sent: Friday, August 14, 2015 12:25 PM
> To: Anatol Belski <anatol.php@belski.net>
> Cc: internals@lists.php.net
> Subject: Re: [PHP-DEV] Warning (5.x) -> TypeError (7) for internal =
Constructors
>=20
> > So from this POV it is good, IMO. Namely, to know that it is the
> > concrete
> case delivering the concrete exception type instead of abstract =
Exception,
> IntlException, PharException, etc. The chosen exception class name =
might make
> you think that it's like enforcing strict mode, but that's not the =
case. Hm, so
> question is - are exception class names really bound to whether strict =
mode is
> used or not?
>=20
> I don't disagree with this on principle, consistency is good, but now =
we actually
> have LESS consistency because the constructor can throw one of many
> exceptions. For example, PDO can throw a \TypeError or a \PDOException =
on
> connection failure, or if the constructor fails in some other way. =
This leads to
> code like this:
>=20
> try {
>    new PDO([]);
> } catch (\TypeError $e) {
>=20
> } catch (\PDOException $e) {
>=20
> }
>=20
As far as I follow the history now, conversions from warning to =
Exception in constructors came in through the RFC you mention. So this =
is so far about Warning -> Exception conversion. The subsequent =
metamorphoses happened afterwards.

> With the potential for duplication in the error handling (in both =
catch blocks), OR
> code like this which is overly generic and will potentially catch =
other random
> stuff:
>=20
> try {
>     new PDO([]);
> } catch (\Throwable $e) {
>     // must be throwable as it's the only common thing between =
\TypeError and
> \PDOException }
>=20
But yeah, why I said it's a BC deviation, older codes won't catch Error. =
This might be a real issue in the old scripts, because they are going to =
break execution except they had an error handler.=20

> This is why I say the previous behavior should be preserved: Warning =
for type
> mis-match, with a (in this case) \PDOException if the constructor =
fails (possibly
> as a result of the mis-match, or for any other reason) UNLESS strict =
types are on,
> then a \TypeError should be thrown, and it wouldn't attempt to execute =
the
> constructor and no \PDOException would be thrown.
>=20
> This then gives us the following flow:
>=20
> try {
>     new PDO([]); // Warning, array given, string expected } catch =
(\PDOException
> $e) {
>      // maybe connection failed, or constructor failed because we =
passed an
> array!
> }
>=20
Technically, it is handy for logging or to rethrow onw exception. But =
practically, if you want to handle it - different exception class names =
are more useful. Having only PDOException how do you know in the script =
flow, what is happened? Having different class name allows better =
granularity in the code flow. Thinking primarily about C++ with =
std::bad_alloc, std::string, etc. Say in pseudo code

try {
	new MyClass;
} catch (Connection $e) {
	sleep();
	// retry
} catch (Argument $e) {
	// log and die
} ...

Some say it is ugly, some say it is practical. Having one exception for =
all makes the code short within one scope and is actually simpler, which =
is a big plus. Having different exceptions allows better granularity in =
handling, which is good as well :) Catching Throwable in PHP7 is the way =
for simplicity, in that sense the option is kept.

Furthermore - standardized class names are good, too. Say it would throw =
ConnectionFailed exception in PDO, or pgsql, where ever in a DB class.

> OR we enable strict types and you basically need the examples shown =
originally,
> but at least that's a conscious choice and expected behavior.
>=20
> I believe that the goal should be: don't surprise your users, and this =
change fails
> that. The new behavior is not what I think most people would expect =
coming
> from 5.x, and that is a bad thing.
>=20
As an unexpected behavior change - yes, it could be classified. As a =
complete fails - IMHO not at all. What to do today about it, well ... =
Given how far in is it now, not sure it should be reverted. Firstly - =
it's going to neglect some work done for the PHP7 adoption in frameworks =
or elsewhere so far, and that is not small. Secondly - the current state =
went through at least 3 metamorphoses, reverting to some pre- of them is =
quite tricky. While doable, a revert will throw the codebase back into =
unstable state for some time.

However were this seen by many core devs as a critical issue that needs =
to be fixed before RC1 - yes, going for beta4 as next were still an =
option. This also why I resist so with introducing any exception usage =
in functions ATM, avoid fundamental changes to be short cut. And this is =
a good example as the topic we're talking about was introduced in April, =
the first notice about a side effect comes in August :)

Regards

Anatol
=20
> On Fri, Aug 14, 2015 at 4:42 AM, Anatol Belski <anatol.php@belski.net>
> wrote:
>=20
> > Hi Davey,
> >
> > > -----Original Message-----
> > > From: me@daveyshafik.com [mailto:me@daveyshafik.com] On Behalf Of
> > > Davey Shafik
> > > Sent: Thursday, August 13, 2015 7:01 PM
> > > To: internals@lists.php.net
> > > Subject: [PHP-DEV] Warning (5.x) -> TypeError (7) for internal
> > Constructors
> > >
> > > Hi,
> > >
> > > I was trying to come up with an example for the
> > > https://wiki.php.net/rfc/internal_constructor_behaviour RFC and
> > > noticed
> > some
> > > unexpected behavior.
> > >
> > > Based on one of the examples:
> > >
> > > <?php
> > > var_dump(new ReflectionFunction([])); ?>
> > >
> > > In PHP 5.6 it will emit a Warning, and returns a unusable instance
> > > of ReflectionFunction, the latter of which should be resolved by =
the
> > > RFC
> > above.
> > >
> > > Warning: ReflectionFunction::__construct() expects parameter 1 to =
be
> > string,
> > > array given
> > >
> > > In PHP 7b3 (and going back to b1 at least), it now throws a
> > > TypeError on
> > the mis-
> > > match. I would expect this behavior in strict type mode, but not =
by
> > default. (see:
> > > http://3v4l.org/jQQtX). The same if you do new PDO([]); but it =
would
> > have
> > > returned a null previously.
> > >
> > > Yes, the result is the same, in both cases, an exception should be
> > thrown, but it
> > > should be Constructor failed (the exact exception seems to change
> > depending
> > > on the class you're instantiating).
> > >
> > > In some cases, this behavior is as expected:
> > >
> > > new MessageFormatter('en_US', null);
> > >
> > > Will result in an IntlException with message "Constructor failed"
> > >
> > > (see: http://3v4l.org/uAjUr)
> > >
> > > I discussed this with Anthony and he pointed out this commit by
> > > nikic as
> > the
> > > change that introduced this behavior:
> > > https://github.com/php/php-src/pull/1215
> > >
> > From what I see in the patch, it is not related to strict mode. It =
is
> > more about unifying places that already throw exceptions with
> > different names to look
> >
> > TypeError: NumberFormatter::__construct() expects parameter 1 to be
> > string, array given
> >
> > So from this POV it is good, IMO. Namely, to know that it is the
> > concrete case delivering the concrete exception type instead of
> > abstract Exception, IntlException, PharException, etc. The chosen
> > exception class name might make you think that it's like enforcing
> > strict mode, but that's not the case. Hm, so question is - are
> > exception class names really bound to whether strict mode is used or =
not?
> >
> > The one with ReflectionFunction you've discovered - yes, that's a BC
> > deviation. However fits into the scheme and didn't happen at much =
places.
> > What is more of backward incompatibility is that the old codes won't
> > now catch those TypeError as most likely they catch Exception, not =
Error.
> >
> > From what I've spotted yet, some look like
> >
> > TypeError: NumberFormatter::__construct() expects at least 2
> > parameters, 0 given
> >
> > that is probably not very logic as it has nothing to do with the
> > argument types. So not sure about that. Where TypeError is =
relatively
> > appropriate (but maybe a bit confusing because it is used in the
> > strict mode as well) for "expect type X, type Y given", the case =
"expect X
> params, Y given"
> > could have another error class name.
> >
> > > I believe that internal functions and methods should only emit a
> > > warning
> > (as per
> > > 5.x) for incorrect argument types, unless (1) it's a constructor
> > failure, and (2)
> > > you're not using strict mode.
> > >
> > About exceptions in the functions - there's another thread. =
Exceptions
> > in objects is quite a ordirary  thing anyway. The tendency is that
> > since ZE throws them instead of fatals, the behavior to extend over
> > the whole code base. Though it needs quite some serious =
deliberations
> > which didn't any big progress yet. Maybe you could contribute there,
> > too. This however has no relation to the strict mode as well.
> >
> > Regards
> >
> > Anatol
> >