Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:104219
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
	s=turbo-smtp; d=php.net;
	h=Received:Received:X-TurboSMTP-Tracking:X-Gm-Message-State:X-Google-Smtp-Source:X-Received:MIME-Version:References:In-Reply-To:From:Date:X-Gmail-Original-Message-Id:Message-ID:Subject:To:Cc:Content-Type;
	b=O3CML5lQaOMVhoABu7cdnI9UgVy7j3uvu5EDovGVa3w2uJmJgBDP8A1wFRop+b
	FUlR+DNaATpOtWOO/qQVJeL5GlQtJtDnBf0uRQP0vwLEoY5Dxk7I6ywJtdEvjvbK
	HQChF0Yy8QJe3lhXDDmD9hE5aLiytgCDCQ6P4FvsX5H9Q=;
MIME-Version: 1.0
References: <CAF+90c_wZvdgVbmdHH+7J=CLx-rivDRr4vB3=HXK3K2HSNAqMg@mail.gmail.com> <CAF+90c_HaeydyEk2qyM=vxQC-_N6KKwCw_WPo1osP1Nkv99cfw@mail.gmail.com> <CAJHtznyX9P565B9S+3PHmX1m_SY32e_2eb3fjjYmsMRWue3LEA@mail.gmail.com> <CAF+90c9m-ghnr7ZNpYmJ1b14Ah8P==jHc40ZGVHLS4dgx5Q05w@mail.gmail.com>
In-Reply-To: <CAF+90c9m-ghnr7ZNpYmJ1b14Ah8P==jHc40ZGVHLS4dgx5Q05w@mail.gmail.com>
Date: Wed, 6 Feb 2019 06:42:41 +0200
Message-ID: <CAJHtznxB4MiV825pQa39X238_u_USX88vTCS8fvLZd=gjnoETw@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000232bd90581325c2e"
Subject: Re: [PHP-DEV] Re: Don't silence fatal errors
From: zeev@php.net (Zeev Suraski)

--000000000000232bd90581325c2e
Content-Type: text/plain; charset="UTF-8"

On Tue, Feb 5, 2019 at 7:19 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Tue, Feb 5, 2019 at 5:55 PM Zeev Suraski <zeev@php.net> wrote:
>
>> On Tue, Feb 5, 2019 at 5:17 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
>>
>>> On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov <nikita.ppv@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

--000000000000232bd90581325c2e--