On Mon, Jun 15, 2015 at 4:53 PM, Dmitry Stogov <dmi...@zend.com> wrote:
> > > 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. > I'm fine with these changes. Patch looks okay to me as well. Nikita
