Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86682
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Nikita Popov'" <nikita.ppv@gmail.com>,
	"'Dmitry Stogov'" <dmitry@zend.com>
Cc: "'Aaron Piotrowski'" <aaron@icicle.io>,
	"'PHP Internals'" <internals@lists.php.net>,
	"'Ferenc Kovacs'" <tyra3l@gmail.com>,
	"'Kalle Sommer Nielsen'" <kalle@php.net>
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> <CAF+90c8StLG8JeTC2ZzJ3412s9BwDZBSv9DRomK47GxEFutSKQ@mail.gmail.com>
In-Reply-To: <CAF+90c8StLG8JeTC2ZzJ3412s9BwDZBSv9DRomK47GxEFutSKQ@mail.gmail.com>
Date: Mon, 15 Jun 2015 23:02:42 +0200
Message-ID: <01c901d0a7ae$a18a06f0$e49e14d0$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQFtSLKjM4Kf9ceVOz7o+CSYMVJiZgIhwnePAYrvRlYBpwkcHAJFBRC9Ao7iPHsBQ/CyewGDLtqyApwDvJgBUwH6vwIaLHCJAWoVwW8Bfqqbu53FxRqg
Content-Language: en-us
Subject: RE: [PHP-DEV] [RFC] Throwable Interface
From: anatol.php@belski.net ("Anatol Belski")



> -----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 Interface
>=20
> On Mon, Jun 15, 2015 at 4:53 PM, Dmitry Stogov <dmitry@zend.com> =
wrote:
>=20
> >
> >
> > 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 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=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 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 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.
> >
>=20
> I'm fine with these changes. Patch looks okay to me as well.
>=20
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