Hi internals,
I have opened a new RFC proposing improved error handling for invalid
integer-valued filter and flags options in the Filter extension.
The proposal introduces a deprecation period in PHP 8.7.
RFC: https://wiki.php.net/rfc/invalid_filter_options
Feedback and review are welcome.
Thanks,
Muhammed Arshid
Hi internals,
I have opened a new RFC proposing improved error handling for invalid integer-valued filter and flags options in the Filter extension.
The proposal introduces a deprecation period in PHP 8.7.
RFC: https://wiki.php.net/rfc/invalid_filter_options
Feedback and review are welcome.
Thanks,
Muhammed Arshid
Looks reasonable at first glance. My additional suggestions for
follow-on RFCs would be:
- The filter types could be turned into an enum. Would be a pretty
simple RFC. - Get rid of SAPI-side input filters for the superglobals; they
complicate code that works with them a bit. The INI option that
controls them is already deprecated, so, an RFC to get rid of them
completely?
I do think ext/filter is kind of weird in that the API is very pre-OO
and has a bunch of hardcoded filters and assumptions on how people used
the superglobals, but if we have it, it's still worth cleaning up.
Looks reasonable at first glance. My additional suggestions for
follow-on RFCs would be:
- The filter types could be turned into an enum. Would be a pretty
simple RFC.- Get rid of SAPI-side input filters for the superglobals; they
complicate code that works with them a bit. The INI option that
controls them is already deprecated, so, an RFC to get rid of them
completely?I do think ext/filter is kind of weird in that the API is very pre-OO
and has a bunch of hardcoded filters and assumptions on how people used
the superglobals, but if we have it, it's still worth cleaning up.
You're correct. I agree that the filter types would be better handled as a
separate, focused RFC for enums.
I'll close this RFC and pursue the enum proposal separately. I have already
opened a PR for the implementation:
https://github.com/php/php-src/pull/24111
Thanks for the suggestion!
- Get rid of SAPI-side input filters for the superglobals; they
complicate code that works with them a bit. The INI option that
controls them is already deprecated, so, an RFC to get rid of them
completely?
I will reiterate that removing these defaults was a mistake. The
intention of the filter extension was to pre-clean all data coming
into the superglobals through these filters, and then access the raw
(but checked/filtered) data through the extensions filter_get() and
friends methods.
Removing these defaults cripled the extension.
If anything, that deprecation ought to be undone, before the extension
gets refreshed.
cheers,
Derick
--
https://derickrethans.nl | https://xdebug.org | https://xdebug.cloud
Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support
mastodon: @derickr@phpc.social @xdebug@phpc.social
I will reiterate that removing these defaults was a mistake. The
intention of the filter extension was to pre-clean all data coming
into the superglobals through these filters, and then access the raw
(but checked/filtered) data through the extensions filter_get() and
friends methods.Removing these defaults cripled the extension.
If anything, that deprecation ought to be undone, before the extension
gets refreshed.
Thank you for explaining this. I understand your concern, and I appreciate
the historical context.
My intention with the proposed enums in
https://github.com/php/php-src/pull/24111 was only to modernize the API and
make filter usage more explicit, rather than to further weaken the
extension’s original purpose.
I’ll take a closer look at the history and rationale behind the removal of
the defaults before proceeding. If the deprecation should be reconsidered
as part of refreshing the filter extension, I agree that this is worth
discussing first.
I will reiterate that removing these defaults was a mistake. The
intention of the filter extension was to pre-clean all data coming
into the superglobals through these filters, and then access the raw
(but checked/filtered) data through the extensions filter_get() and
friends methods.Removing these defaults cripled the extension.
If anything, that deprecation ought to be undone, before the extension
gets refreshed.cheers,
Derick
Yeah, with the deprecation of the superglobal filter INI options,
ext/filter is left with an awkward filter_input function that's not much
useful than filter_var. If the deprecation is kept and furthered to a
removal, then I don't know if it makes much sense to still have that
function anymore (or more likely, simplify its implementation since it
can't do superglobal filtering - since there are quite a few users of it
after all, ~258k per a quick GH code search).
Regardless, there's still merit in cleaning up flags and using enums
regardless what path is taken.
Hi everyone,
I’ve updated the RFC based on the discussion and the implementation in PR
#24111.
The proposal now focuses on adding Filter\Filter and Filter\Flag enums to
the Filter extension.
RFC: https://wiki.php.net/rfc/invalid_filter_options
I’d appreciate your feedback on the updated proposal.
Thanks,
Muhammed Arshid
Hi
The proposal now focuses on adding Filter\Filter and Filter\Flag enums
to
the Filter extension.
Enums are strongly typed values by themselves. Requiring the use of a /
the backing value for some functionality (in this case: Passing multiple
options as a bitmask) is an abuse of enums, nullifies the type safety
and makes them “fancy constants” in practice.
With regarding to the “formal parts” of the RFC, I'm noting that the
“Voting Choices” section is unchanged from the template. In particular,
the voting widget's title needs to be updated.
Best regards
Tim Düsterhus
Thanks for the feedback!
Enums are strongly typed values by themselves. Requiring the use of a /
the backing value for some functionality (in this case: Passing multiple
options as a bitmask) is an abuse of enums, nullifies the type safety
and makes them “fancy constants” in practice.With regarding to the “formal parts” of the RFC, I'm noting that the
“Voting Choices” section is unchanged from the template. In particular,
the voting widget's title needs to be updated.
I understand the concern about type safety. However, the real-world usage
of filter_var() is worth considering too. Many use cases only require a
filter ID, without any flags. Sourcegraph search:
https://sourcegraph.com/search?q=context:global+lang:php+filter_var%5C%28&patternType=regexp&sm=0
If combined bitmasks are uncommon in practice, a Filter\Flag enum could
still improve the API for individual flags. I agree that we should weigh
this benefit against the loss of type safety when combining flags.
I’ve updated the “Voting Choices” section.
Best regards
Muhammed Arshid
Hi
I understand the concern about type safety. However, the real-world
usage
offilter_var()is worth considering too. Many use cases only require a
filter ID, without any flags. Sourcegraph search:
https://sourcegraph.com/search?q=context:global+lang:php+filter_var%5C%28&patternType=regexp&sm=0If combined bitmasks are uncommon in practice, a Filter\Flag enum could
still improve the API for individual flags. I agree that we should
weigh
this benefit against the loss of type safety when combining flags.
An enum would slightly improve the API for the filter_var() use case
without any flags. But for filter_var_array() it already would no
longer work and for filter_var() with flags, it would result in a
weird mixture of a strong enum with “free form” options field.
I believe that introducing enums right now is not doing enough to
meaningfully improve the developer experience. As an example, if we
already had Tagged Unions (https://wiki.php.net/rfc/tagged_unions), the
per-filter options could be provided directly with the union case:
filter_var($foo, Filter\Int(allowOctal: true, allowHex: true));
Introducing the enum right now likely would block off that path.
Best regards
Tim Düsterhus