Hello,
I’ve created an RFC for modifying the exception hierarchy for PHP 7, adding Throwable interface and renaming the exceptions thrown from fatal errors. The RFC is now ready for discussion.
RFC: https://wiki.php.net/rfc/throwable-interface https://wiki.php.net/rfc/throwable-interface
Pull Request: https://github.com/php/php-src/pull/1284 https://github.com/php/php-src/pull/1284
Aaron Piotrowski
Hello, I’ve created an RFC for modifying the exception hierarchy for
PHP 7, adding Throwable interface and renaming the exceptions thrown
from fatal errors. The RFC is now ready for discussion. RFC:
https://wiki.php.net/rfc/throwable-interface
https://wiki.php.net/rfc/throwable-interface Pull Request:
https://github.com/php/php-src/pull/1284
https://github.com/php/php-src/pull/1284
I like this RFC! It's much more intuitive as the current BaseException
approve.
Some small notes:
Backwards Compatibility
|Throwable|, |Error|, |TypeError|, and |ParseError| will no longer be
available in the global namespace.
Where are the new classes and the interface located if it's not in the
global namespace or do I muss something?
If I remember right the TypeException/TypeError will be thrown in some
cases of internal functions with strict argument count.
In my opinion a missing argument or to much arguments has nothing to do
with the type and should have it's own Exception/Error class.
Aaron Piotrowski
Marc
Where are the new classes and the interface located if it's not in the global namespace or do I muss something?
Sorry if what I wrote wasn’t clear. Throwable, Error, TypeError, and ParseError will be built-in interfaces/classes in the root namespaces. So users will not be able to make classes with those exact names, but classes with those names in a non-global namespace (e.g., \Vendor\Package\TypeError) will still be possible. I’ve updated the RFC to make this clearer.
If I remember right the TypeException/TypeError will be thrown in some cases of internal functions with strict argument count.
In my opinion a missing argument or to much arguments has nothing to do with the type and should have it's own Exception/Error class.
I believe this actually generates a warning, it does not throw an exception. However, this does remind me of another point: It is possible that more classes extending Error could be created in the future for different error reasons. Anything that throws an Error could later be changed to throw an object extending Error without a BC break. This is a separate issue though and could be discussed in a future RFC.
Aaron Piotrowski
Where are the new classes and the interface located if it's not in the global namespace or do I muss something?
Sorry if what I wrote wasn’t clear. Throwable, Error, TypeError, and ParseError will be built-in interfaces/classes in the root namespaces. So users will not be able to make classes with those exact names, but classes with those names in a non-global namespace (e.g., \Vendor\Package\TypeError) will still be possible. I’ve updated the RFC to make this clearer.
ThanksIf I remember right the TypeException/TypeError will be thrown in some cases of internal functions with strict argument count.
In my opinion a missing argument or to much arguments has nothing to do with the type and should have it's own Exception/Error class.
I believe this actually generates a warning, it does not throw an exception. However, this does remind me of another point: It is possible that more classes extending Error could be created in the future for different error reasons. Anything that throws an Error could later be changed to throw an object extending Error without a BC break. This is a separate issue though and could be discussed in a future RFC.
Ok than I remember wrong.
Aaron Piotrowski
Marc
Hi Aaron,
I’ve created an RFC for modifying the exception hierarchy for PHP 7,
adding Throwable interface and renaming the exceptions thrown from fatal
errors. The RFC is now ready for discussion.RFC: https://wiki.php.net/rfc/throwable-interface <
https://wiki.php.net/rfc/throwable-interface>;
Pull Request: https://github.com/php/php-src/pull/1284 <
https://github.com/php/php-src/pull/1284>;
Does this include internal function type errors?
e.g.
$ php -r 'var_dump(mt_srand("99999999999999999999999999999999999"));'
PHP Warning: mt_srand() expects parameter 1 to be integer, string given in
Command line code on line 1
NULL
If not, please make these exceptions rather than E_WARNING.
We need consistency here.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Aaron,
I’ve created an RFC for modifying the exception hierarchy for PHP 7,
adding Throwable interface and renaming the exceptions thrown from fatal
errors. The RFC is now ready for discussion.RFC: https://wiki.php.net/rfc/throwable-interface <
https://wiki.php.net/rfc/throwable-interface>;
Pull Request: https://github.com/php/php-src/pull/1284 <
https://github.com/php/php-src/pull/1284>;Does this include internal function type errors?
e.g.$ php -r 'var_dump(mt_srand("99999999999999999999999999999999999"));'
PHP Warning:mt_srand()expects parameter 1 to be integer, string given in
Command line code on line 1
NULLIf not, please make these exceptions rather than E_WARNING.
We need consistency here.
This would be great for strict_types mode as there it results in a fatal
error.
http://3v4l.org/vHl8K
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Does this include internal function type errors?
e.g.$ php -r 'var_dump(mt_srand("99999999999999999999999999999999999"));'
PHP Warning:mt_srand()expects parameter 1 to be integer, string given in
Command line code on line 1
NULLIf not, please make these exceptions rather than E_WARNING.
We need consistency here.
Changing a Warning to an Exception is a much bigger deal than changing
an Error to an Exception. While an Exception can be caught, the
fundamental nature of it is that it is fatal if it is not caught. So
you would be changing an advisory warning to a fatal error.
There might be cases where this would be sensible, but this has nothing
to do with a) changing Errors to Exceptions (which has already passed)
or b) changing the hierarchy to add a Throwable interface (the subject
of this thread).
Regards,
--
Rowan Collins
[IMSoP]
Does anyone have any questions, comments, or concerns about the Throwable Interface RFC?
http://wiki.php.net/rfc/throwable-interface
The proposed exception hierarchy:
interface Throwable
⊢ Exception implements Throwable
∟ Error implements Throwable (replaces EngineException)
⊢ TypeError (replaces TypeException)
∟ ParseError (replaces ParseException)
I’d like to complete a vote before too many alpha releases are made.
I’ll be happy to resolve the merge conflicts in the PR if the vote passes.
Regards,
Aaron Piotrowski
Does anyone have any questions, comments, or concerns about the Throwable Interface RFC?
http://wiki.php.net/rfc/throwable-interface
The proposed exception hierarchy:
interface Throwable
⊢ Exception implements Throwable
∟ Error implements Throwable (replaces EngineException)
⊢ TypeError (replaces TypeException)
∟ ParseError (replaces ParseException)I’d like to complete a vote before too many alpha releases are made.
I’ll be happy to resolve the merge conflicts in the PR if the vote passes.
My only objections have already been raised, but I'll reiterate them briefly:
- Having both Error and Exception makes little sense, especially
since we have historically used error to refer to an error that wasn't
an exception (something that triggered the error handler). - The name "Error" is going to have a fairly high collision rate
with user code. - I think they should all use Exception as the root instead having
a new root with multiple children (and yes, I am aware of the impact
of this, and it has already been discussed on this list).
My only objections have already been raised, but I'll reiterate them briefly:
- Having both Error and Exception makes little sense, especially
since we have historically used error to refer to an error that wasn't
an exception (something that triggered the error handler).- The name "Error" is going to have a fairly high collision rate
with user code.- I think they should all use Exception as the root instead having
a new root with multiple children (and yes, I am aware of the impact
of this, and it has already been discussed on this list).--
http://www.php.net/unsub.php
I outlined the reasons for the Error name choice in the RFC. While it may cause some confusion, I feel it is a
better choice than obfuscating the different exception branches with similar class names. Having a class
named TypeException that does not extend Exception is unintuitive. I think users will be confused by
catch (Exception $e) not catching TypeException. Using a different suffix makes it clearer that
catch (Error $e) is necessary to catch TypeError.
Error will collide with some user code, but changing the name of a class to make code compatible with PHP 7
is a relatively easy change to make, largely accomplished with a find-and-replace. Hopefully most user code is
using namespaces and collisions will be avoided.
Suggestions are certainly welcome for a different name, but honestly I think Error makes a lot of sense.
Non-fatal errors in PHP will trigger notices and warnings, while fatal errors can be thrown. Users likely will use
the term ‘Uncaught Error’ when searching, minimizing the overlap with information about non-fatal errors.
Perhaps it would be best if the vote was separated:
-
Replace BaseException with Throwable interface. Hierarchy otherwise remains as-is, with EngineException
implementing Throwable, TypeException extending EngineException, and ParseException implementing
Throwable. -
If (1) is accepted, also change EngineException to Error, TypeException to TypeError, and ParseException
with ParseError, with TypeError and ParseError extending Error.
Regards,
Aaron Piotrowski
Does anyone have any questions, comments, or concerns about the
Throwable Interface RFC?http://wiki.php.net/rfc/throwable-interface
The proposed exception hierarchy:
interface Throwable
⊢ Exception implements Throwable
∟ Error implements Throwable (replaces EngineException)
⊢ TypeError (replaces TypeException)
∟ ParseError (replaces ParseException)I’d like to complete a vote before too many alpha releases are made.
I’ll be happy to resolve the merge conflicts in the PR if the vote
passes.My only objections have already been raised, but I'll reiterate them
briefly:
- Having both Error and Exception makes little sense, especially
since we have historically used error to refer to an error that wasn't
an exception (something that triggered the error handler).- The name "Error" is going to have a fairly high collision rate
with user code.
I also like EngineException more than Error.
- I think they should all use Exception as the root instead having
a new root with multiple children (and yes, I am aware of the impact
of this, and it has already been discussed on this list).
This done on purpose. To prevent catching of new exceptions by old PHP code.
If we won't accept this now (before 7.0 release), it would make more
troubles in the future.
I didn't make deep code review, and don't know all possible consequences.
Anyway, I would make a chance to accept this now or never.
RM, your thoughts?
In case we decide to vote for this RFC, I'll make code review and will help
with possible problems.
Thanks. Dmitry.
- I think they should all use Exception as the root instead having
a new root with multiple children (and yes, I am aware of the impact
of this, and it has already been discussed on this list).This done on purpose. To prevent catching of new exceptions by old PHP code.
We have code that previously triggered error handlers or got caught by
exceptions and now will not trigger the handler nor get caught. If
only fatal errors had been converted this would not be an issue, but
some non-fatals were converted (TypeException) and now we have this
issue. Why is that less of an issue than catching all exceptions?
(I'm seriously asking, that is not rhetorical)
Levi Morrison wrote on 10/06/2015 15:37:
We have code that previously triggered error handlers or got caught by
exceptions and now will not trigger the handler nor get caught.
AFAIK, the only things which are going to not inherit from Exception
were things which didn't inherit from Exception in the first place.
The only edge case is E_RECOVERABLE, which previously triggered an error
handler, and now will require a different mechanism (catch ( Error $e )
or catch ( Throwable $e )).
some non-fatals were converted (TypeException)
There was no equivalent to TypeException in previous versions of PHP, so
in what sense has it been "converted"? Maybe I'm being thick and there's
a situation that has been, but the only type-related error I know of was
type-hinting classes, which were fatal.
Regards,
Rowan Collins
[IMSoP]
some non-fatals were converted (TypeException)
There was no equivalent to TypeException in previous versions of PHP, so in
what sense has it been "converted"? Maybe I'm being thick and there's a
situation that has been, but the only type-related error I know of was
type-hinting classes, which were fatal.
When an argument was provided that did not fulfill the parameter's
type requirement an E_RECOVERABLE was emitted. This meant that the
error handler was triggered. Now it will not be triggered, nor will it
get caught by catch (Exception).
I also like EngineException more than Error.
I think EngineException has a couple of problems with it:
-
EngineException doesn’t really accurately represent the reason for the error. For
example, passing the wrong type to a function isn’t an ‘engine' problem, it’s an
error in user code. In the future it may be desirable to add more classes in this
exception branch, and I think a more general name would be more desirable. -
The name EngineException implies it descends from Exception. I worry that this
will cause a lot of confusion for users thinking they have to be catching Exceptions
when the problem is actually an error in their code. Discussing why you should not
catch Error objects is easier than trying to explain why certain Exception objects
should not be caught.
Error is best choice for the name of the second exception branch. It has precedent
in other languages and is less likely to cause confusion. Most user code that would
collide with the name is likely very outdated, but still could easily be updated in
only a few minutes.
Regards,
Aaron Piotrowski
Hi Dmitry,
I would go by accepting this. Furthermore – if you feel that the implementation is stable enough and does not BC, I would suggest to have it already in the alpha2.
As there seems to be at all no resistance in the votes (no even single “no” voter yet), nor strong objection here on the lists. The minimal voting period is 1 week, so theoretically if it were ended on Wed (the voting RFC doesn’t disallow this) – there were still some time to do extensive testing and fixes. Alpha2 is the time where a) a lot of users will be able to test it and b) it still can be reverted in the worst case.
What do you think?
Regards
Anatol
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Wednesday, June 10, 2015 9:37 AM
To: Levi Morrison; Aaron Piotrowski; Nikita Popov
Cc: internals; Anatol Belski; Kalle Sommer Nielsen
Subject: Re: [PHP-DEV] [RFC] Throwable Interface
Does anyone have any questions, comments, or concerns about the Throwable Interface RFC?
http://wiki.php.net/rfc/throwable-interface
The proposed exception hierarchy:
interface Throwable
⊢ Exception implements Throwable
∟ Error implements Throwable (replaces EngineException)
⊢ TypeError (replaces TypeException)
∟ ParseError (replaces ParseException)I’d like to complete a vote before too many alpha releases are made.
I’ll be happy to resolve the merge conflicts in the PR if the vote passes.
My only objections have already been raised, but I'll reiterate them briefly:
- Having both Error and Exception makes little sense, especially
since we have historically used error to refer to an error that wasn't
an exception (something that triggered the error handler). - The name "Error" is going to have a fairly high collision rate
with user code.
I also like EngineException more than Error.
- I think they should all use Exception as the root instead having
a new root with multiple children (and yes, I am aware of the impact
of this, and it has already been discussed on this list).
This done on purpose. To prevent catching of new exceptions by old PHP code.
If we won't accept this now (before 7.0 release), it would make more troubles in the future.
I didn't make deep code review, and don't know all possible consequences.
Anyway, I would make a chance to accept this now or never.
RM, your thoughts?
In case we decide to vote for this RFC, I'll make code review and will help with possible problems.
Thanks. Dmitry.
Hi Dmitry,
I would go by accepting this. Furthermore – if you feel that the implementation is stable enough and does not BC, I would suggest to have it already in the alpha2.
As there seems to be at all no resistance in the votes (no even single “no” voter yet), nor strong objection here on the lists. The minimal voting period is 1 week, so theoretically if it were ended on Wed (the voting RFC doesn’t disallow this) – there were still some time to do extensive testing and fixes. Alpha2 is the time where a) a lot of users will be able to test it and b) it still can be reverted in the worst case.
What do you think?
Regards
Anatol
I will have some time to resolve the merge conflicts later today, so I will be happy to take care of that.
Regards,
Aaron Piotrowski
Hi,
I made a quick code review, and I don't see any technical problems in
implementation.
-
Anyway, I think it's a bad idea to rename "EngineException" (and others)
into "Error"(s).
This will prevent using class "Error" in applications, and may potentially
break some of them.
I also don't like renaming in ~440 tests (I didn't review all these
changes). -
I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but should be
added now or in the future). -
In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt EngineExcpetion is
not renamed into Error.
Thanks. Dmitry.
On Jun 14, 2015, at 11:31 AM, Anatol Belski anatol.php@belski.net
wrote:Hi Dmitry,
I would go by accepting this. Furthermore – if you feel that the
implementation is stable enough and does not BC, I would suggest to have it
already in the alpha2.As there seems to be at all no resistance in the votes (no even single
“no” voter yet), nor strong objection here on the lists. The minimal
voting period is 1 week, so theoretically if it were ended on Wed (the
voting RFC doesn’t disallow this) – there were still some time to do
extensive testing and fixes. Alpha2 is the time where a) a lot of users
will be able to test it and b) it still can be reverted in the worst case.What do you think?
Regards
Anatol
I will have some time to resolve the merge conflicts later today, so I
will be happy to take care of that.Regards,
Aaron Piotrowski
Hi Dmitry,
-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Monday, June 15, 2015 10:53 AM
To: Aaron Piotrowski
Cc: Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi,
I made a quick code review, and I don't see any technical problems in
implementation.
Anyway, I think it's a bad idea to rename "EngineException" (and others) into
"Error"(s).
This will prevent using class "Error" in applications, and may potentially break
some of them.
I also don't like renaming in ~440 tests (I didn't review all these changes).I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but should be added
now or in the future).In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt EngineExcpetion is
not renamed into Error.
Thanks for the review. I've also tested the branch which has today's master merged in, by now it doesn't show any functional regression.
Actually I part your first concern about Error. Spent some time phishing for "class Error" on github and found three apps doing this without namespace. But that's from 100 search result pages. And those three apps are forked zero to 1 times, so pretty much low usage. This will likely break more in the world, we hardly know.
The current RFC can't be changed while in voting. But how it looks like, the principle of the Trowable RFC is something everyone agrees on. Maybe do another RFC in parallel for better names? Still I'd stand for taking it into alpha2 - if we want to have it, better to arrange it in a way that it doesn't cause unnecessary delays in the release process.
Regards
Anatol
Hi Dmitry,
-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Monday, June 15, 2015 10:53 AM
To: Aaron Piotrowski
Cc: Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi,
I made a quick code review, and I don't see any technical problems in
implementation.
Anyway, I think it's a bad idea to rename "EngineException" (and others) into
"Error"(s).
This will prevent using class "Error" in applications, and may potentially break
some of them.
I also don't like renaming in ~440 tests (I didn't review all these changes).I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but should be added
now or in the future).In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt EngineExcpetion is
not renamed into Error.Thanks for the review. I've also tested the branch which has today's master merged in, by now it doesn't show any functional regression.
Actually I part your first concern about Error. Spent some time phishing for "class Error" on github and found three apps doing this without namespace. But that's from 100 search result pages. And those three apps are forked zero to 1 times, so pretty much low usage. This will likely break more in the world, we hardly know.
The current RFC can't be changed while in voting. But how it looks like, the principle of the Trowable RFC is something everyone agrees on. Maybe do another RFC in parallel for better names? Still I'd stand for taking it into alpha2 - if we want to have it, better to arrange it in a way that it doesn't cause unnecessary delays in the release process.
Hi Dmitry and Anatol,
I fixed three more tests that were missed when merging, as I only checked those
that failed after the merge. I should have used find and replace again after the
merge, which is how I changed the majority of the tests. Only a few tests required
manual changes, mostly because of hard-coded string lengths, and one or two
tests used the class name Error. Note that Nikita recently changed nearly all
these tests and many others when updating the phrasing on uncaught exception
messages. While many tests were changed, users won’t have such issues
because they aren’t catching these exceptions yet.
Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is that
something you’d take care of after the merge?
I am strongly against naming something _____Exception if it doesn’t extend
Exception. This was most of the point of the RFC, as I find code such as
catch (Exception $e) { … } catch (EngineException) { … } very unintuitive and I
feel it will lead to confusion for users.
I sifted through search results on GitHub looking for usage of Error before
proposing the RFC and also found only a couple of actual uses from generally
unused projects. Most of the other results are forks of an old version of PHPUnit.
Namespaces have been around long enough that most libraries and apps will not
have such a class in the root namespace. As far as BC breaks go, renaming a
class in an app is a fairly trivial one to make, and one that very, very few people
will have to do. I feel having a simple name such as Error is more important than
avoiding naming conflicts with a very small amount of code.
Regards,
Aaron Piotrowski
Hi Dmitry,
-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com dmitry@zend.com]
Sent: Monday, June 15, 2015 10:53 AM
To: Aaron Piotrowski
Cc: Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi,
I made a quick code review, and I don't see any technical problems in
implementation.
Anyway, I think it's a bad idea to rename "EngineException" (and
others) into
"Error"(s).
This will prevent using class "Error" in applications, and may potentially
break
some of them.
I also don't like renaming in ~440 tests (I didn't review all these
changes).I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but should be
added
now or in the future).In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt EngineExcpetion is
not renamed into Error.Thanks for the review. I've also tested the branch which has today's
master merged in, by now it doesn't show any functional regression.Actually I part your first concern about Error. Spent some time phishing
for "class Error" on github and found three apps doing this without
namespace. But that's from 100 search result pages. And those three apps
are forked zero to 1 times, so pretty much low usage. This will likely
break more in the world, we hardly know.The current RFC can't be changed while in voting. But how it looks like,
the principle of the Trowable RFC is something everyone agrees on. Maybe do
another RFC in parallel for better names? Still I'd stand for taking it
into alpha2 - if we want to have it, better to arrange it in a way that it
doesn't cause unnecessary delays in the release process.Hi Dmitry and Anatol,
I fixed three more tests that were missed when merging, as I only checked
those
that failed after the merge. I should have used find and replace again
after the
merge, which is how I changed the majority of the tests. Only a few tests
required
manual changes, mostly because of hard-coded string lengths, and one or two
tests used the class name Error. Note that Nikita recently changed nearly
all
these tests and many others when updating the phrasing on uncaught
exception
messages. While many tests were changed, users won’t have such issues
because they aren’t catching these exceptions yet.Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is
that
something you’d take care of after the merge?
I don't care about this a lot. I just think it's better.
If you don't see any problems, please, move the code.
I am strongly against naming something _____Exception if it doesn’t extend
Exception. This was most of the point of the RFC, as I find code such as
catch (Exception $e) { … } catch (EngineException) { … }very
unintuitive and I
feel it will lead to confusion for users.I sifted through search results on GitHub looking for usage of Error before
proposing the RFC and also found only a couple of actual uses from
generally
unused projects. Most of the other results are forks of an old version of
PHPUnit.
Namespaces have been around long enough that most libraries and apps will
not
have such a class in the root namespace. As far as BC breaks go, renaming a
class in an app is a fairly trivial one to make, and one that very, very
few people
will have to do. I feel having a simple name such as Error is more
important than
avoiding naming conflicts with a very small amount of code.
Nikita, what is your opinion?
If you don't care, I won't as well.
Thanks. Dmitry.
Regards,
Aaron Piotrowski
Hi Dmitry,
-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com dmitry@zend.com]
Sent: Monday, June 15, 2015 10:53 AM
To: Aaron Piotrowski
Cc: Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi,
I made a quick code review, and I don't see any technical problems in
implementation.
Anyway, I think it's a bad idea to rename "EngineException" (and
others) into
"Error"(s).
This will prevent using class "Error" in applications, and may
potentially break
some of them.
I also don't like renaming in ~440 tests (I didn't review all these
changes).I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into
zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but should be
added
now or in the future).In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt EngineExcpetion is
not renamed into Error.Thanks for the review. I've also tested the branch which has today's
master merged in, by now it doesn't show any functional regression.Actually I part your first concern about Error. Spent some time phishing
for "class Error" on github and found three apps doing this without
namespace. But that's from 100 search result pages. And those three apps
are forked zero to 1 times, so pretty much low usage. This will likely
break more in the world, we hardly know.The current RFC can't be changed while in voting. But how it looks like,
the principle of the Trowable RFC is something everyone agrees on. Maybe do
another RFC in parallel for better names? Still I'd stand for taking it
into alpha2 - if we want to have it, better to arrange it in a way that it
doesn't cause unnecessary delays in the release process.Hi Dmitry and Anatol,
I fixed three more tests that were missed when merging, as I only checked
those
that failed after the merge. I should have used find and replace again
after the
merge, which is how I changed the majority of the tests. Only a few tests
required
manual changes, mostly because of hard-coded string lengths, and one or
two
tests used the class name Error. Note that Nikita recently changed nearly
all
these tests and many others when updating the phrasing on uncaught
exception
messages. While many tests were changed, users won’t have such issues
because they aren’t catching these exceptions yet.Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is
that
something you’d take care of after the merge?I don't care about this a lot. I just think it's better.
If you don't see any problems, please, move the code.I am strongly against naming something _____Exception if it doesn’t extend
Exception. This was most of the point of the RFC, as I find code such as
catch (Exception $e) { … } catch (EngineException) { … }very
unintuitive and I
feel it will lead to confusion for users.I sifted through search results on GitHub looking for usage of Error
before
proposing the RFC and also found only a couple of actual uses from
generally
unused projects. Most of the other results are forks of an old version of
PHPUnit.
Namespaces have been around long enough that most libraries and apps will
not
have such a class in the root namespace. As far as BC breaks go, renaming
a
class in an app is a fairly trivial one to make, and one that very, very
few people
will have to do. I feel having a simple name such as Error is more
important than
avoiding naming conflicts with a very small amount of code.Nikita, what is your opinion?
If you don't care, I won't as well.
I'm fine with these changes. Patch looks okay to me as well.
Nikita
-----Original Message-----
From: Nikita Popov [mailto:nikita.ppv@gmail.com]
Sent: Monday, June 15, 2015 10:36 PM
To: Dmitry Stogov
Cc: Aaron Piotrowski; Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi Dmitry,
-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com dmitry@zend.com]
Sent: Monday, June 15, 2015 10:53 AM
To: Aaron Piotrowski
Cc: Anatol Belski; PHP Internals
Subject: Re: [PHP-DEV] [RFC] Throwable InterfaceHi,
I made a quick code review, and I don't see any technical problems in
implementation.
Anyway, I think it's a bad idea to rename "EngineException" (and
others) into
"Error"(s).
This will prevent using class "Error" in applications, and may
potentially break some of them.
I also don't like renaming in ~440 tests (I didn't review all these
changes).I think it's better to move "zend_ce_throwable"
definition/initialization from zend_interfaces.h/c into
zend_exception.h/c.
At least, this will allow arg_info reuse. (it's missed now, but
should be added now or in the future).In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt
EngineExcpetion is not renamed into Error.Thanks for the review. I've also tested the branch which has today's
master merged in, by now it doesn't show any functional regression.Actually I part your first concern about Error. Spent some time
phishing for "class Error" on github and found three apps doing this
without namespace. But that's from 100 search result pages. And those
three apps are forked zero to 1 times, so pretty much low usage. This
will likely break more in the world, we hardly know.The current RFC can't be changed while in voting. But how it looks
like, the principle of the Trowable RFC is something everyone agrees
on. Maybe do another RFC in parallel for better names? Still I'd
stand for taking it into alpha2 - if we want to have it, better to
arrange it in a way that it doesn't cause unnecessary delays in the release
process.Hi Dmitry and Anatol,
I fixed three more tests that were missed when merging, as I only
checked those that failed after the merge. I should have used find
and replace again after the merge, which is how I changed the
majority of the tests. Only a few tests required manual changes,
mostly because of hard-coded string lengths, and one or two tests
used the class name Error. Note that Nikita recently changed nearly
all these tests and many others when updating the phrasing on
uncaught exception messages. While many tests were changed, users
won’t have such issues because they aren’t catching these exceptions
yet.Would you like me to move zend_ce_throwable to zend_exceptions.h/c or
is that something you’d take care of after the merge?I don't care about this a lot. I just think it's better.
If you don't see any problems, please, move the code.I am strongly against naming something _____Exception if it doesn’t extend
Exception. This was most of the point of the RFC, as I find code such as
catch (Exception $e) { … } catch (EngineException) { … }very
unintuitive and I
feel it will lead to confusion for users.I sifted through search results on GitHub looking for usage of Error
before
proposing the RFC and also found only a couple of actual uses from
generally
unused projects. Most of the other results are forks of an old version of
PHPUnit.
Namespaces have been around long enough that most libraries and apps will
not
have such a class in the root namespace. As far as BC breaks go, renaming
a
class in an app is a fairly trivial one to make, and one that very, very
few people
will have to do. I feel having a simple name such as Error is more
important than
avoiding naming conflicts with a very small amount of code.Nikita, what is your opinion?
If you don't care, I won't as well.I'm fine with these changes. Patch looks okay to me as well.
I would then suggest Aaron to stick to the minimal voting period (announcing this as early as possible), if the voting passes - then merge the branch on Wednesday. This way we would have nearly one week to test and do fixes in master.
Kalle, Ferenc, how do you feel about this?
Regards
Anatol
I would then suggest Aaron to stick to the minimal voting period (announcing this as early as possible), if the voting passes - then merge the branch on Wednesday. This way we would have nearly one week to test and do fixes in master.
Kalle, Ferenc, how do you feel about this?
Is changing the voting dates on the RFC to run only through tomorrow
allowed? If so, I can make that change so it can be merged this week.
Regards,
Aaron Piotrowski
Hi,
2015-06-15 19:25 GMT-03:00 Aaron Piotrowski aaron@icicle.io:
I would then suggest Aaron to stick to the minimal voting period (announcing this as early as possible), if the voting passes - then merge the branch on Wednesday. This way we would have nearly one week to test and do fixes in master.
Kalle, Ferenc, how do you feel about this?
Is changing the voting dates on the RFC to run only through tomorrow
allowed? If so, I can make that change so it can be merged this week.Regards,
Aaron Piotrowski
I second that. Right now many projects are awaiting to move towards
php7 testing due to issues like
https://github.com/sebastianbergmann/phpunit/issues/1748. It would be
great to get the next alpha with the fixed exception hierarchy, in
case the release managers agree.
Márcio
On Tue, Jun 16, 2015 at 1:41 AM, Marcio Almada marcio.web2@gmail.com
wrote:
Hi,
2015-06-15 19:25 GMT-03:00 Aaron Piotrowski aaron@icicle.io:
On Jun 15, 2015, at 4:02 PM, Anatol Belski anatol.php@belski.net
wrote:
I would then suggest Aaron to stick to the minimal voting period
(announcing this as early as possible), if the voting passes - then merge
the branch on Wednesday. This way we would have nearly one week to test and
do fixes in master.Kalle, Ferenc, how do you feel about this?
Is changing the voting dates on the RFC to run only through tomorrow
allowed? If so, I can make that change so it can be merged this week.Regards,
Aaron PiotrowskiI second that. Right now many projects are awaiting to move towards
php7 testing due to issues like
https://github.com/sebastianbergmann/phpunit/issues/1748. It would be
great to get the next alpha with the fixed exception hierarchy, in
case the release managers agree.Márcio
--
I would be okay with shortening the voting period, given the extensive
discussion, having Dmitry and Nikita reviewing the changes and seeing how
strong the support for this rfc is(a reasonable amount of votes with
unanimously in favor of the change).
I think that even if something comes up later with the implementation, we
can fix it on the way before the RC phase starts.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is
that
something you’d take care of after the merge?I don't care about this a lot. I just think it's better.
If you don’t see any problems, please, move the code.
Moving the definition of zend_ce_throwable to zend_exceptions.h/c
requires either:
-
Moving #define REGISTER_ITERATOR_INTERFACE from
zend_interfaces.c to zend_interfaces.h so it is available in
zend_exceptions.c (I could take the opportunity to rename this macro
to REGISTER_INTERFACE). -
Defining a macro in zend_exceptions.c that is identical to
REGISTER_ITERATOR_INTERFACE.
Which option sounds better?
Aaron Piotrowski
I think REGISTER_INTERFACE() macro in zend_interfaces.h is better.
Actually, it's not related to ITERATORs at all.
Thanks. Dmitry.
Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is
that
something you’d take care of after the merge?I don't care about this a lot. I just think it's better.
If you don’t see any problems, please, move the code.Moving the definition of zend_ce_throwable to zend_exceptions.h/c
requires either:
Moving #define REGISTER_ITERATOR_INTERFACE from
zend_interfaces.c to zend_interfaces.h so it is available in
zend_exceptions.c (I could take the opportunity to rename this macro
to REGISTER_INTERFACE).Defining a macro in zend_exceptions.c that is identical to
REGISTER_ITERATOR_INTERFACE.Which option sounds better?
Aaron Piotrowski
Hi!
Does anyone have any questions, comments, or concerns about the Throwable Interface RFC?
http://wiki.php.net/rfc/throwable-interface
The proposed exception hierarchy:
interface Throwable
⊢ Exception implements Throwable
∟ Error implements Throwable (replaces EngineException)
⊢ TypeError (replaces TypeException)
∟ ParseError (replaces ParseException)
Don't see anything wrong with it. I think we can put this to a vote.
--
Stas Malyshev
smalyshev@gmail.com
Hi all,
On Tue, May 26, 2015 at 3:02 AM, Rowan Collins rowan.collins@gmail.com
wrote:
Does this include internal function type errors?
e.g.$ php -r 'var_dump(mt_srand("99999999999999999999999999999999999"));'
PHP Warning:mt_srand()expects parameter 1 to be integer, string given
in
Command line code on line 1
NULLIf not, please make these exceptions rather than E_WARNING.
We need consistency here.Changing a Warning to an Exception is a much bigger deal than changing an
Error to an Exception. While an Exception can be caught, the fundamental
nature of it is that it is fatal if it is not caught. So you would be
changing an advisory warning to a fatal error.There might be cases where this would be sensible, but this has nothing to
do with a) changing Errors to Exceptions (which has already passed) or b)
changing the hierarchy to add a Throwable interface (the subject of this
thread).
Error and unhandled exception differs a lot. I agree.
Personally, I don't mind at all if internal function raises exception for
type errors, but
others may be disturbed a lot probably.
We need a roadmap to clean this mess up. How about add
- Change internal function type error to exception by PHP 7.1 (or PHP 7.2
if a year is not enough)
to the RFC for smoother migration and future consistency?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hello :-),
At Hoa, we are totally in favor or this RFC. We tried to make our
exception hierarchy compatible with PHP7 with success but it was very
difficult to make it backward compatible. With this proposal, all our
problems are going to be solved, so +1 for us!
Hello,
I’ve created an RFC for modifying the exception hierarchy for PHP 7, adding Throwable interface and renaming the exceptions thrown from fatal errors. The RFC is now ready for discussion.
RFC: https://wiki.php.net/rfc/throwable-interface https://wiki.php.net/rfc/throwable-interface
Pull Request: https://github.com/php/php-src/pull/1284 https://github.com/php/php-src/pull/1284Aaron Piotrowski
--
Ivan Enderlin
Developer of Hoa
http://hoa-project.net/
PhD. computer scientist
Hacker
http://mnt.io/