Re: PR for minor bugfix in compact()

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. >>> >>> 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? >>> >>> Regards >>> David >>> >> 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 > > 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