Hi Aaron, > -----Original Message----- > From: Aaron Piotrowski [mailto:aa...@icicle.io] > Sent: Monday, July 27, 2015 5:32 AM > To: Sammy Kaye Powers <m...@sammyk.me>; Scott Arciszewski > <sc...@paragonie.com> > Cc: Anatol Belski <anatol....@belski.net>; la...@garfieldtech.com; Jakub > Kubíček <kelerest...@gmail.com>; Stanislav Malyshev > <smalys...@gmail.com>; sc...@paragonie.com; rowan.coll...@gmail.com; > pierre....@gmail.com; Dean Eigenmann <dean.eigenm...@icloud.com>; > Yasuo Ohgaki <yohg...@ohgaki.net>; PHP Internals <internals@lists.php.net> > Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7 > > > > I must have overlooked a detail here. > > > > According to > > https://github.com/tpunt/PHP7-Reference#throwable-interface > > there are Throwables called Error, as a separate designation from an > > exception. I didn't see this in the engine exceptions RFC, so I was > > unaware that was even a thing. > > > > In this case, yes, as long as you can wrap it in try/catch blocks, > > SecurityError which extends Error and/or implements Throwable is an > > excellent suggestion. > > > > Previously, I thought the suggestion was to stick to triggering errors > > (E_ERROR, E_RECOVERABLE_ERROR, etc.). > > > > Scott Arciszewski > > Chief Development Officer > > Paragon Initiative Enterprises <https://paragonie.com> > > > > -- > > PHP Internals - PHP Runtime Development Mailing List To unsubscribe, > > visit: http://www.php.net/unsub.php > > > > I believe some were suggesting triggering an E_ERROR, though most E_ERRORs > in the engine have been replaced with thrown Error exceptions, so I think using > E_ERROR in this case would be inappropriate. > > As I suggested in my prior email, I think throwing an instance of Error would be > appropriate when the functions random_bytes() and random_int() fail. > > There are several conditions that already cause the engine to throw an Error (or > subclass thereof): > > (1)->method(); // Throws Error > declare(strict_types=1); array_map(1, 1); // Throws TypeError require 'file-with- > parse-error.php'; // Throws ParseError eval("$a[ = 1;"); // Throws ParseError > 1 << -1; // Throws ArithmeticError > intdiv(1, 0); // Throws DivisionByZeroError > 1 % 0; // Throws DivisionByZeroError > > Of particular interest in the above examples is intdiv(), an internal function that > can throw an instance of Error if the denominator is zero. > > I propose that random_bytes() and random_int() should throw an instance of > Error if the parameters are not as expected or if generating a random number > fails. (To avoid further debate about a subclass, the function should throw just a > plain instance of Error, it can always be subclassed later without BC concerns). > > random_bytes() and random_int() failing closed is very important to prevent > misguided or lazy programmers from using false in place of a random value. A > return of false can easily be overlooked and unintentionally be cast to a zero or > empty string. A thrown instance of Error must be purposely caught and ignored > to produce the same behavior. As Larry pointed out, it is a very common error > for programmers to not do a strict check using === against false when calling > strpos(). > > Does anyone have a strong objection to the above proposal? If not, then I think > Sammy should update his PRs to throw an Error so they can be merged before > the next beta release. > Yes, there is an objection at least on my side. As we was discussing the PR on the github page, the primary goal of the suggestion to move the discussion to the intarnals list was to create an approach about exceptions in functions. This seems not be the case here. It is not the question whether exceptions should be thrown in functions - it's almost obvious that they should now after it's the engine behavior. But it is a huge question of the consistency with anything else. That was also the reason why me and Kalle have changed our minds about your PR converting fatals in the functions.
The only case where exception is thrown in a function is intdiv() now. But also as mentioned elsewhere - it's something tightly coupled with the behavior of div/mod in the engine. IMHO it is not building any base for randomly adding exceptions elsewhere. Neither I don't see as a base that the CSPRNG functions are new. Neither the argument "we can change it later to xyz". The main concern is still not addressed, and even wasn't started to be addressed. It is for nothing to have new "nice" functions with nice and correct behavior while leaving the old "ugly" functions ugly. IMO we should stop building special cases and move to the conceptualization as first. The more special cases exist, the harder it will be in the future to bring the behaviors inline without BC breaks. Till now I haven't seen even any gentle hint about such conceptualization work going on, no RFC page still exists, but it's being continued pushing on a special case. Solving an immediate issue might be tactically justifiable, but without a strategical thinking will lead to even a bigger issue. It is not something we want to leave behind us for the next generations. I would kindly call therefore, to think about this in a more global way, discuss and show at least a rough draft about what should be done regarding the existing functions, new functions, backward compatibility, exact warning/error types and how they should be converted, etc. before pushing further on this case. Thanks Anatol -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
