Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:106720
MIME-Version: 1.0
References: <CAF+90c9Kc6m_bv=aJxbD3-RmnNXXLqv6mPjHp7U7qxX_ck27qw@mail.gmail.com>
 <CAJHtznyadHb5_RgjddnPiyeWXtLQyp2CDK=ix=BiEd+DEUHqLg@mail.gmail.com>
 <CAF+90c_94zAj-7XW+=YtZ2VsZEshd4DvNY8_17PyiBXvYR1gdw@mail.gmail.com> <CANS-=pepNMEj0O_zp4j7eMZAFi+BN=h0fsYjqQTS45pQaAuF0g@mail.gmail.com>
In-Reply-To: <CANS-=pepNMEj0O_zp4j7eMZAFi+BN=h0fsYjqQTS45pQaAuF0g@mail.gmail.com>
Date: Wed, 28 Aug 2019 13:57:04 +0200
Message-ID: <CAF+90c8ZN_RksLKZNVqdCo6+GsAQDpq8LxXUNtqAUr3q9Z9SPw@mail.gmail.com>
To: Lynn <kjarli@gmail.com>
Cc: Zeev Suraski <zeev@php.net>, PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000006e41c605912c1734"
Subject: Re: [PHP-DEV] [RFC] Reclassifying engine warnings
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000006e41c605912c1734
Content-Type: text/plain; charset="UTF-8"

On Wed, Aug 28, 2019 at 1:46 PM Lynn <kjarli@gmail.com> wrote:

>
>
>
> This argument makes sense for arrays and objects (and I don't promote
>> undefined index/property to exceptions for that reason), but I don't think
>> it holds any water for simple variables. Writing @$counts[$key]++ is a
>> lazy
>> way to count values and avoid ugly boilerplate for if
>> (isset($counts[$key])) { $counts[$key]++; } else { $counts[$key] = 1; }.
>> But @$foo++ is just a really bad way of writing either $foo++ or $foo = 1.
>> Outside of variable variables, the concept of a conditionally defined
>> variable just doesn't make a lot of sense.
>>
>
> The variables are not conditionally declared on purpose, but as it's code
> written 15~20 years ago, maintained, copy-pasted over and over and never
> reviewed, there's a lot of issues that remain that my trigger an error
> exception if this proposal is passed without migration path.
>

To be clear, this was in response to Zeev's particular argument, which I
don't think is valid. I am aware that accesses to undefined variables are a
thing in legacy code.

However, I feel pretty strongly that converting any of these to
deprecations is not a good idea. While there's certainly different views on
this, I've seen it often enough deprecation warning are considered an even
lower error level than notices (imagine my surprise when PEAR stopped
working completely in PHP 8 because nobody ever saw the hundreds of
suppressed deprecations). We could throw a deprecation in addition, but I
think this will make the development experience really suck for anyone who
is not actively working on a migration right now (imagine seeing lots of
warnings/notices during development twice).

I think it would be better to register an error handler that matches the
cases that will throw and can then log those. I'd be happy to provide an
implementation if you think this would be useful for your use-case. Also
has the nice advantage that you can start using it right now and don't have
to wait until you upgrade to a release that has deprecations.

Nikita


>
> ```
> // a.php
> if ($someCondition) { // comes from another file, never used, can't verify
> existance
>     $foo = 'foo';
> }
>
> // b.php
> $bar = 'bar';
>
> // scenario1.php
> include 'a.php'; // this would crash
> include 'b.php';
>
> echo $foo . $bar; // this would crash if the include didn't yet
>
> // scenario2.php
> $someCondition = true;
>
> include 'a.php'; // this would not crash
> include 'b.php';
>
> echo $foo . $bar; // this would not crash
> ```
>
> The problem with the above legacy code, which we sadly have spread through
> a spaghetti of thousands of files, is that if it starts throwing errors
> with no migration path, we simply can't update without spending possibly
> weeks debugging and trying to find scenarios where this might trigger. Now
> I'm not sure, but if I would've used `include_once` and combine this with
> function/class scopes (people loved to do that in our legacy code), all
> hell breaks loose and it will be nearly impossible to find the cases.
>
> All I'm asking is for a clear upgrade path with deprecations so that my
> code can be safely migrated over time, rather than have it crash with the
> next version. Compared to <?php vs <?, this will break a lot and is not as
> easy to fix. By having it throw deprecations which we can easily log to a
> specific file, we can gather and fix the cases when we have time to spend
> on technical debt, and by the time the warning will turn into an error
> exception, we'll have fixed probably 90%+ of the cases, making the upgrade
> possible.
>
> Regards,
> Lynn van der Berg
>

--0000000000006e41c605912c1734--