Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86650
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain icicle.io designates 199.38.81.6 as permitted sender)
Content-Type: multipart/alternative; boundary="Apple-Mail=_04714ED2-B231-4A3A-A68C-5B65BC6EA9BF"
Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\))
In-Reply-To: <008701d0a762$4348c320$c9da4960$@belski.net>
Date: Mon, 15 Jun 2015 08:55:04 -0500
Cc: internals@lists.php.net
Message-ID: <36F7469F-7D86-43F6-ABC7-9252DF707BFC@icicle.io>
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>
To: Anatol Belski <anatol.php@belski.net>,
 Dmitry Stogov <dmitry@zend.com>
Subject: Re: [PHP-DEV] [RFC] Throwable Interface
From: aaron@icicle.io (Aaron Piotrowski)

--Apple-Mail=_04714ED2-B231-4A3A-A68C-5B65BC6EA9BF
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On Jun 15, 2015, at 6:56 AM, Anatol Belski <anatol.php@belski.net> =
wrote:
>=20
> Hi Dmitry,
>=20
>> -----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 Interface
>>=20
>> Hi,
>>=20
>> I made a quick code review, and I don't see any technical problems in
>> implementation.
>>=20
>> 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).
>>=20
>> 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).
>>=20
>> 3) In ext/simplexml/tests/SimpleXMLElement_xpath_3.phpt =
EngineExcpetion is
>> not renamed into Error.
>>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20


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=E2=80=99t have such =
issues
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 am strongly against naming something _____Exception if it doesn=E2=80=99=
t 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 =
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


--Apple-Mail=_04714ED2-B231-4A3A-A68C-5B65BC6EA9BF--