Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86677
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLvjXPB99VxcgNZjjCPZ+eDkndx+2c-N+Uyka03uLdZYbA@mail.gmail.com>
References: <971AB39D-1E20-43E8-9CF1-A7F67E3C14C3@icicle.io>
	<CAGa2bXb8CaJsJorx0HTzSgEeAGXJN7DoW5vi=m+wFiUXNw+tng@mail.gmail.com>
	<556363D3.1040902@gmail.com>
	<12C3389A-AFF5-42CA-8190-E4227309DAED@icicle.io>
	<CAFMT4NppuzY1dhPVcVhFFB-0373AkuB4-dh-mynY0p6R9T76tw@mail.gmail.com>
	<CA+9eiLszbdRS5kKU73HSvvB4zqayS8RKAioX8cJZKqreejKfyQ@mail.gmail.com>
	<016201d0a6bf$96e37dc0$c4aa7940$@belski.net>
	<0A4CC87D-2E54-4202-9AA2-22856F0B6011@icicle.io>
	<CA+9eiLtmDQNCL3YJ9ahJpvkY9YU2RpPR=aPNChi1OSamCZ+Q3w@mail.gmail.com>
	<008701d0a762$4348c320$c9da4960$@belski.net>
	<36F7469F-7D86-43F6-ABC7-9252DF707BFC@icicle.io>
	<CA+9eiLvjXPB99VxcgNZjjCPZ+eDkndx+2c-N+Uyka03uLdZYbA@mail.gmail.com>
Date: Mon, 15 Jun 2015 22:36:17 +0200
Message-ID: <CAF+90c8StLG8JeTC2ZzJ3412s9BwDZBSv9DRomK47GxEFutSKQ@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: Aaron Piotrowski <aaron@icicle.io>, Anatol Belski <anatol.php@belski.net>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=047d7bf0bfc2ea1c23051894685a
Subject: Re: [PHP-DEV] [RFC] Throwable Interface
From: nikita.ppv@gmail.com (Nikita Popov)

--047d7bf0bfc2ea1c23051894685a
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 15, 2015 at 4:53 PM, Dmitry Stogov <dmitry@zend.com> wrote:

>
>
> On Mon, Jun 15, 2015 at 4:55 PM, Aaron Piotrowski <aaron@icicle.io> wrote=
:
>
>>
>> On Jun 15, 2015, at 6:56 AM, Anatol Belski <anatol.php@belski.net> wrote=
:
>>
>> 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 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 b=
e
>> 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 checke=
d
>> 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 test=
s
>> 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 nearl=
y
>> all
>> these tests and many others when updating the phrasing on uncaught
>> exception
>> messages. While many tests were changed, users won=E2=80=99t have such i=
ssues
>> because they aren=E2=80=99t catching these exceptions yet.
>>
>> Would you like me to move zend_ce_throwable to zend_exceptions.h/c or is
>> that
>> something you=E2=80=99d 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=E2=80=
=99t extend
>> Exception. This was most of the point of the RFC, as I find code such as
>> `catch (Exception $e) { =E2=80=A6 } catch (EngineException) { =E2=80=A6 =
}` 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 o=
f
>> PHPUnit.
>> Namespaces have been around long enough that most libraries and apps wil=
l
>> not
>> have such a class in the root namespace. As far as BC breaks go, renamin=
g
>> 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

--047d7bf0bfc2ea1c23051894685a--