On Mon, Jun 15, 2015 at 4:55 PM, Aaron Piotrowski <aa...@icicle.io> wrote:
> > 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 <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 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 > >
