Warning (5.x) -> TypeError (7) for internal Constructors

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?

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:

try {
new PDO([]);
} catch (\TypeError $e) {

} catch (\PDOException $e) {

}

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:

try {
new PDO([]);
} catch (\Throwable $e) {
// must be throwable as it's the only common thing between \TypeError
and \PDOException
}

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.

This then gives us the following flow:

try {
new PDO([]); // Warning, array given, string expected
} catch (\PDOException $e) {
// maybe connection failed, or constructor failed because we passed an
array!
}

OR we enable strict types and you basically need the examples shown
originally, but at least that's a conscious choice and expected behavior.

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.

Why do you have an expectation that a function should throw one exception
type and one type only? It is customary that different causes for an
exception will also result in different exception types. Here TypeError is
thrown if a wildly inappropriate type is passed, say an array instead of a
string. A PDOException on the other hand is thrown if some failure specific
to PDO's domain occurs, say a non-existing socket or invalid user name. A
programmer wishing to catch a PDOException is unlikely to be interested in
catching the kind of error that TypeErrors signify.

The motivation behind using TypeError here is twofold. Firstly, it is an
error type that exists specifically to be thrown for zpp failures.
Secondly, and more importantly, this avoid throwing different exception
types between weak and strict mode. If we didn't throw TypeError here, then
switching from weak to strict would change what was previously a
PDOException to a TypeError -- which I would consider to be rather
unexpected and suboptimal.

Nikita

-----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

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?

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:

try {
new PDO([]);
} catch (\TypeError $e) {

} catch (\PDOException $e) {

}

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:

try {
new PDO([]);
} catch (\Throwable $e) {
// must be throwable as it's the only common thing between \TypeError and
\PDOException }

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.

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.

This then gives us the following flow:

try {
new PDO([]); // Warning, array given, string expected } catch (\PDOException
$e) {
// maybe connection failed, or constructor failed because we passed an
array!
}

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.

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.

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

On Fri, Aug 14, 2015 at 4:42 AM, Anatol Belski anatol.php@belski.net
wrote:

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