> -----Original Message----- > From: Nikita Popov [mailto:nikita....@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 > > 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. > 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
