On Tue, Feb 5, 2019 at 7:19 PM Nikita Popov <nikita....@gmail.com> wrote:
> On Tue, Feb 5, 2019 at 5:55 PM Zeev Suraski <z...@php.net> wrote: > >> On Tue, Feb 5, 2019 at 5:17 PM Nikita Popov <nikita....@gmail.com> wrote: >> >>> On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov <nikita....@gmail.com> >>> wrote: >>> >>> >>> I'd like to move forward with this change. I think the overall reception >>> here has been positive, although in the discussion some other >>> possibilities >>> that avoid/reduce the BC aspect have been discussed. I think now that we >>> have a PHP 8 branch, it would make sense to apply this as-is. The BC >>> break >>> is quite minor (compared to the other changes in PHP 8) and I think this >>> is >>> the cleanest way to solve the problem, as it only changes the list of >>> silences errors, without introducing any new error handling concepts or >>> mechanisms. >> >> >> I don't think that other changes that may or may not make it into PHP 8 >> should influence our decision - BC breaks accumulate and the more you have >> of them, the more difficult it is to migrate. I'm also present unaware of >> anything we already decided to 'break' in PHP 8 as of now (with the >> exception of the removal of deprecated 7.x features). >> >> That said - I think we all agree the BC breakage scope is small - but at >> the same time, it may be quite fatal for those that are affected. Those >> who have display_errors on (which is both horrible for production and at >> the same time fairly popular) are risking exposing sensitive data that >> beforehand, they were safely and explicitly hiding using @. >> > > I can see the general concern here, but I'm having a hard time imagining > that this will be an issue in practice. If you have display_errors=on you > are at risk of leaking information in error messages with or without this > change. The prime example of leaking information, which is passwords > contained in the exception stack trace of a failed PDO connection, isn't > even affected by this, because the silencing will be removed as part of > exception unwinding anyway. > > Well, there are all sorts of information leaks that can happen as a result of this, including filesystem layout and even the fact that the server is running PHP in the first place. Is there any reason *not* to do it in such a way that provides a gentler >> migration path? Introduce a new error level that would be a part of E_ALL >> in 7.4, but outside of E_ALL in 8.0 - that would govern whether fatal >> errors are silenced or not. >> > > The reason not to do it is pretty much the same as always: Adding that new > error level is basically the same as adding a new ini setting (and if we do > want that, then I think it should be it's own ini setting and not hacked in > as part of error_reporting), and you know our usual opinion on that topic. > Furthermore, in this particular case it would also defeat the purpose of > the change. If we set the option such that fatals are silenced by default, > then we may as well not make the change, because the people who benefit > most from it (non-expert users) are not going to change that default. > Conversely, if we do not silence fatals by default, then we don't solve the > issue with display_errors=on you mentioned above (as it requires explicitly > setting an option, in which case they could just set display_errors=off > instead.) > While I don't really agree that adding a new error level is equivalent to adding a new INI entry, and I would go for having it as a part of the error levels and not as a separate INI entry if we were to add this functionality - I think you're fundamentally right that this approach doesn't truly bring value in making the migration smoother. I can't really think of an elegant way to handle this given the unique situation where this whole change is in the context of error suppression - which means deprecation notices aren't helpful. As long as we have a prominent warning about this in our migration guide alerting people to the associated risk in setups where display errors is on - I can live with the change as-is. How do we ensure that it doesn't get lost in the clutter given that it has no RFC? Zeev
