Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:80695
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain basereality.com from 209.85.212.170 cause and error)
MIME-Version: 1.0
Date: Sat, 17 Jan 2015 14:02:19 +0000
Message-ID: <CA+kxMuQcfMY00k+WtnNUppD7N9PMQsx356h1XTy1X8px1m0MBw@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Class constructor behaviour
From: danack@basereality.com (Dan Ackroyd)

Hi,

I've been going through some bug reports, and have found there are
several bad behaviours related to class constructors that ought to be
corrected, but could only be done at a major version. The bad
behaviours are:

Constructors returning null
---------------------------

Several classes in PHP return null when there is a problem in the
parameters passed to their constructor e.g.

<?php

$mf = new MessageFormatter('en_US', '{this was made intentionally incorrect}');
if ($mf === null) {
    echo "Surprise!";
}

?>

This is pretty horrible and should be fixed by making sure that
constructors either return an object or throw an exception.
Additionally the exception policy for core (that was previously
discussed here: http://marc.info/?t=119263748000001&r=1&w=2 ) should
be updated so that any constructor returning NULL is considered a bug,
no matter what the ini settings are. To be clear, procedural code
should behave as before, with users expected to check for errors.

This would be a BC break for people who are handling the constructor
returning null currently, as they would need to wrap that code with an
try/catch block.


The list of classes that show this behaviour is:
* finfo
* PDO
* Collator
* IntlDateFormatter
* MessageFormatter
* NumberFormatter
* ResourceBundle
* IntlRuleBasedBreakIterator




Constructors give warning, but are then in an unusable state
------------------------------------------------------------
Several constructors check the parameters that they are given.....and
then just give a warning when they are not acceptable. e.g.

<?php
//Reflection function is not meant to accept an array
$reflection = new ReflectionFunction([]);
//Warning: ReflectionFunction::__construct() expects parameter 1 to be
string, array given in...

var_dump($reflection->getParameters());
//Fatal error: ReflectionFunctionAbstract::getParameters(): Internal
error: Failed to retrieve the reflection object in..

?>

Again, this should be fixed by changing the constructors to throw an
exception if the input parameters are not acceptable.

Although technically this would be a BC break, I can't think of any
sane situation where people would be depending on the current
behaviour.

The list of classes that I believe have this issue is:
* UConverter
* SplFixedArray
* ReflectionFunction
* ReflectionParameter
* ReflectionMethod
* ReflectionProperty
* ReflectionExtension
* ReflectionZendExtension
* Phar
* PharData
* PharFileInfo


Constructor gives error
-----------------------

Some constructors check the parameters they are given, and then emit
either a 'Catchable fatal error' error or other error e.g.

<?php
$foo = new IntlGregorianCalendar(new StdClass);
//Output: Catchable fatal error: Object of class stdClass could not be
converted to string in..
?>

Despite the word catchable, this is not actually an exception that is
catchable, it is an error. It is only if the user has called
set_error_handler, and in the error handler they throw an exception
then the code, that the error would be catchable.

The only other class that has a similar behaviour is PDORow which
gives a 'Fatal error' which can't be intercepted, but can only be
logged through a function registered through
register_shutdown_function().


--------------

The list of classes that have these issues, is possibly not complete.
The ones I've listed above was not done through an exhaustive search.

So, questions:

i) Can anyone see a big hurdle in fixing these behaviours, other than
it being a BC break for people who are currently relying on these
behaviours ?

ii) Are there any other bad behaviours that people are aware of that
ought to be fixed at a major version?


These changes would need an RFC to be approved I presume. However
doing the RFC would probably require making a patch that implements
most, if not all, of these changes. As that would be a significant
amount of work, I thought I'd get people's feedback before starting
that.

cheers
Dan