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
Hi internals,
I've opened a PR to causecompact()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
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).
Hi internals,
I've opened a PR to causecompact()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
DavidHey 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,
becausecompact()should be rather rare
and mostly used on an array, but others might feel differently about this.Best regards,
George P. Banyard
Hi internals,
I've opened a PR to causecompact()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
DavidHey 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,
becausecompact()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