# PR for minor bugfix in compact() - Thread: https://externals.io/message/114229 - Original archive: https://news-web.php.net/php.internals/114229 - Started by David Gebler on 2021-04-28T11:12:37+00:00 - Messages: 7 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #114229 — David Gebler — 2021-04-28T11:12:37+00:00 URL: https://externals.io/message/114229#114229 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 --- ## #114230 — G. P. B. — 2021-04-28T11:17:56+00:00 URL: https://externals.io/message/114229#114230 · In reply to: #114229 > 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 --- ## #114231 — David Gebler — 2021-04-28T11:24:29+00:00 URL: https://externals.io/message/114229#114231 · In reply to: #114230 Ah I see, thanks for the clarification! I only did it this way because when I did my RFC for fsync, I recall one or two people mentioned they weren't keen on the bit where it could raise a warning rather than a TypeError - but I appreciate that one was a completely new function and not a change to an existing one (and it went through with the warning condition in the end, anyway). > > > > […] > > 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 --- ## #114289 — Ben Ramsey — 2021-05-07T01:44:45+00:00 URL: https://externals.io/message/114229#114289 · In reply to: #114230 > > > > […] > > 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. Cheers, Ben --- ## #114291 — Christian Schneider — 2021-05-07T07:06:38+00:00 URL: https://externals.io/message/114229#114291 Am 07.05.2021 um 03:44 schrieb Ben Ramsey : > > > […] > > 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 --- ## #114296 — Ben Ramsey — 2021-05-07T23:13:36+00:00 URL: https://externals.io/message/114229#114296 · In reply to: #114291 Christian Schneider wrote on 5/7/21 02:06: > 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 > Not a lot of chatter about this on the list, but I also agree that this should be an `E_WARNING` in 8.1 and promoted to a TypeError in PHP 9. This is our standard process. David, please update the PR, and we'll get it merged. Cheers, Ben --- ## #114297 — David Gebler — 2021-05-08T00:07:22+00:00 URL: https://externals.io/message/114229#114297 Great, that's been updated, build in progress (relevant tests passing locally). I'd have preferred TypeError but appreciate there being a consistent convention to follow. These kind of small fixes are also about me becoming more familiar with typical conventions in the engine - some of this stuff is just not stuff you'd find out until you stick up a PR and get feedback from more experienced contributors so it's all good to know. Dave > Christian Schneider wrote on 5/7/21 02:06: > > […] > changed to a TypeError in PHP 9. > > […] > threads about tightening type rules IMHO. > > […] > it. > > […] > > Not a lot of chatter about this on the list, but I also agree that this > should be an `E_WARNING` in 8.1 and promoted to a TypeError in PHP 9. This > is our standard process. > > David, please update the PR, and we'll get it merged. > > Cheers, > Ben