Closed TysonAndre closed 3 years ago
@TysonAndre I've been doing this task for quite a long while (we were above 300 UNKNOWN
default values initially), and I'd say most of the remaining UNKNOWN
defaults cannot be changed (easily) in PHP 8.0, or the change isn't worth, mostly because of the following reasons:
mixed
parameters, like in stream_filter_prepend()
DatePeriod::__construct()
not long ago in https://github.com/php/php-src/pull/6081/commits/9adde72f2b57dd97ea643260011960f916ad3633#diff-7b738accc3d60f74c259da18588ddc5dR4128 but neither Nikita nor Derick found it useful. But most of the listed ext/pgsql functions are similar)On the other hand, I haven't checked ext/dba and ext/imap since I try to ignore these extensions, and it seems to me that both ext/ldap
and ext/odbc
could benefit from another round of review :) And as far as I see, the most remaining UNKNOWN
defaults could be fixed via deprecations (either on the function or on the parameter level) in a next version.
I’m working on a PR for ext/ldap
For ext/odbc
see https://github.com/php/php-src/pull/6154
For ext/pdo_pgsql
: https://github.com/php/php-src/pull/6159
And I'll directly commit the fix for xmlwriter_write_dtd_entity()
, since its ZPP already has been fixed recently, but I only updated the stub for its method counterpart.
I'll try to handle IMAP as I'm already working on it
Zend functions as well as SAPI functions are not included in this list, but I've just submitted two PRs for them to fix the last remaining UNKNOWN
default values (where possible):
I tried my hand at the rand()
and rand_mt()
functions:
We're done here, right? I think everything either has an open PR or is intentional.
@nikic I also think we're done. The only functions I haven't looked at are the ones in ext/dba, but apart from that we should be good.
I'm now linking my last PR: https://github.com/php/php-src/pull/6176
Okay, I'm closing this one then. I looked at ext/dba and think the UNKNOWN in dba_open/dba_popen could be made nullable, but given the state of that code, I don't think it's worth the bother.
Related to #23
See the discussion in https://externals.io/message/111878
Currently, for ldap_sasl_bind,
null
is coerced to the empty string forstring $bind_rdn = UNKNOWN
with strict_types=0, and is a TypeError in strict_types=1, and the stubs useUNKNOWN
.If it's safe to change the default to the empty string or null, it'd be an improvement to do so - I'm not familiar with the ldap implementation (etc) in detail.
Mentioned by @MCMic - I assume it'll be possible until 8.0 is stable if done in a backwards compatible way
EDIT: It seems like most other things are correct other than ldap. Documenting the reasons why UNKNOWN had to be used in a line comment above the stub would help contributors or PECL maintainers writing new stubs. E.g. https://www.php.net/dateperiod has multiple signatures.