Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114291
Content-Type: text/plain;
	charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.80.0.2.43\))
Date: Fri, 7 May 2021 09:06:38 +0200
References: <CA+p1Faa1sg4pKLixrzXo1F8D8zRx1GoMCqCDDgh9GhCyhEuSOw@mail.gmail.com>
 <CAFPFaMJhU02akee_Rw-F6X8cCp=s0gmyGBECtyB+FFqmYgS0NQ@mail.gmail.com>
 <cmu-lmtpd-11612-1620351896-0@darkcity>
To: PHP Internals <internals@lists.php.net>
In-Reply-To: <cmu-lmtpd-11612-1620351896-0@darkcity>
Message-ID: <480BB785-0064-46D4-B705-B5E71D2DC0CC@cschneid.com>
Subject: Re: [PHP-DEV] PR for minor bugfix in compact()
From: cschneid@cschneid.com (Christian Schneider)

Am 07.05.2021 um 03:44 schrieb Ben Ramsey <ramsey@php.net>:
> On 4/28/21 06:17, G. P. B. wrote:
>> On Wed, 28 Apr 2021 at 12:12, David Gebler <davidgebler@gmail.com> =
wrote:
>>> Hi internals,
>>> I've opened a PR to cause compact() to throw a TypeError if its =
parameters
>>> are not valid, which I consider to be a fix for what is effectively =
a bug
>>> whereby logical errors in user code can be silently swallowed.
>>>=20
>>> GPB has done an initial review and left a comment
>>> https://github.com/php/php-src/pull/6921#pullrequestreview-646848902 =
in
>>> which he suggests I open this up to the floor, so here it is, =
seeking
>>> your feedback kindly. Also if anyone can clarify what is meant by a =
warning
>>> "will be promoted in PHP 9", I am not familiar with what changes are
>>> planned for the next major version?
>>>=20
>>> Regards
>>> David
>>>=20
>> Hey David,
>> What I meant is that we usually don't introduce an exception without =
prior
>> warning to existing functionality.
>> So making this a Warning in the PHP 8 series which gets promoted to a
>> TypeError in PHP 9,
>> similarly to how most of the internal functions went from returning =
null +
>> warning to throwing a TypeError,
>> or many of the warnings that got promoted to ValueErrors in 8.0 =
because
>> it's a major release.
>> Personally I don't mind introducing the TypeError immediately in PHP =
8.1,
>> because compact() should be rather rare
>> and mostly used on an array, but others might feel differently about =
this.
>> Best regards,
>> George P. Banyard
>=20
> Does anyone have any thoughts or concerns about this being in 8.1? I'm =
trying to decide whether to merge the PR, and I'd like to make sure =
anyone who wants to speak up has a chance to do so.



I agree with George that it should be an E_WARNING first and then =
changed to a TypeError in PHP 9.
This should be the default process for reasons given in many other =
threads about tightening type rules IMHO.

So no, I'd prefer if this PR to be changed to E_WARNING before merging =
it.

- Chris