PHPCompatibility
PHP 8.3: deprecate dba_fetch()
Related to #1589
Analysis
Recap of existing PHP 8.2 behavior based on the RFC:
// Signature 1: called with 2 or 3 arguments.
function dba_fetch(string|array $key, resource $dba, int $skip = 0): string|false {}
// Signature 2: called with exactly 3 arguments, and $dba trades places with $skip
function dba_fetch(string|array $key, int $skip, resource $dba): string|false {}
Updated behavior for each version:
// Signature 1: unchanged
function dba_fetch(string|array $key, resource $dba, int $skip = 0): string|false {}
// Signature 2: deprecated in PHP 8.3
function dba_fetch(string|array $key, int $skip, resource $dba): string|false {}
Detection in PHP 8.2
dba_fetch(...)with 3rd argument of type int, see "Syntax Variations" below.
Detection in PHP 8.3
dba_fetch(...)with 2nd argument of type int, see "Syntax Variations" below.
Top 2000 Packages
Found 2 usages of dba_fetch in the top 2000 packages. Neither uses the deprecated signature.
Examples in james-heinrich/getid3:
dba_fetch(getID3::VERSION, $this->dba) != getID3::VERSION
$result = dba_fetch($key, $this->dba);
Syntax Variations
dba_fetch(..., 1, ...)✅ Can be handled by PHPCompatibility.dba_fetch(..., $skip, ...)❌ Unreliable, as $skip could be defined anywhere.
3v4l.org
dba_fetch() is not available on 3v4l.org
References
- RFC: Deprecate functions with overloaded signatures
- PR: Implement "Deprecate functions with overloaded signatures" RFC for PHP 8.3
- PHP Manual:
@jrfnl This function is seldom used with the deprecated signature (Top 2000 Packages). It's also hard to sniff as it relies on types (Syntax Variations). You could basically swap the order of the last 2 args before the deprecation.
Understood. It may still be worth it to check for a hard-coded value which evaluates to an integer as the second/$skip argument ?
Between the AbstractFunctionCallParameterSniff and the TokenGroup::isNumber() helper, nearly all the logic needed for that is already available, so it would just be a case of wiring that together.
Edit: actually - as the $skip argument could also be the third argument, we'd need to explicitly look for the second argument and disregard the argument if it is a named parameter in the function call.
Upon a closer look, even though it might logically make sense to look at the second argument because integers are easier to infer, the php-src commit actually checks for the type of the 3rd argument. On 8.3 that would be a resource, and on 8.4 that would be a Dba\Connection. If we write a sniff to check for an int, it won't technically be correct and might even be wrong with future PHP changes to that signature.
https://github.com/php/php-src/pull/11703/commits/2398cd10a35b2018b3250e37b1bf13b442d73c6c
Not easy with PHPCompatibility. Definitely doable with Psalm though.
If we write a sniff to check for an int, it won't technically be correct and might even be wrong with future PHP changes to that signature.
While I hear you on the "technically incorrect", in practice looking for a hard-coded value which evaluates to an integer in the second parameter will be able to flag the current issue, while looking at the third parameter will never allow us to flag anything, as if it's an integer, the signature is correct and if the third parameter is a variable, it may be either a resource or a value for $skip and we can't tell.
Future signature changes will need to be dealt with in the future and is not something which should block us from detecting a currently incompatible signature, so I'm okay with checking the parameter value of the second parameter to detect this change.
I'm not sure how much the DBA extension is used though. Feel free to (de-)prioritize if you have insights into that (based on your experience or there may be some stats available on the Exakat blog ?).
I don't have stats beyond the public packages, but this one will probably be in proprietary code. I've never seen it in the wild personally.
I'm fine with detecting int in 2nd position as a proxy. I'll try to focus on all the research first.
@jrfnl Anything else you need from me on this ticket before classifying it?