> On Jun 15, 2015, at 6:56 AM, Anatol Belski <anatol....@belski.net> wrote:
>
> Hi Dmitry,
>
>> -----Original Message-----
>> From: Dmitry Stogov [mailto:dmi...@zend.com]
>> Sent: Monday, June 15, 2015 10:53 AM
>> To: Aaron Piotrowski
>> Cc: Anatol Belski; PHP Internals
>> Subject: Re: [PHP-DEV] [RFC] Throwable Interface
>>
>> Hi,
>>
>> I made a quick code review, and I don't see any technical problems in
>> implementation.
>>
>> 1) 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).
>>
>> 2) 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).
>>
>> 3) 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
