Fix return values for mysqli

[kamil-tekiela]

mysqli::init() doesn't return any values. It just calls the mysqli constructor.

mysqli_connect() returns an object or false. I can't see any way that it would ever return null. Am I wrong?

[cmb69]

php-src/ext/mysqli/mysqli_api.c

Lines 1449 to 1467 in 269936e

	if (is_method && (Z_MYSQLI_P(getThis()))->ptr) {
	return;
	}
	mysql = (MY_MYSQL *)ecalloc(1, sizeof(MY_MYSQL));
	#ifndef MYSQLI_USE_MYSQLND
	if (!(mysql->mysql = mysql_init(NULL)))
	#else
	/*
	We create always persistent, as if the user want to connect
	to p:somehost, we can't convert the handle then
	*/
	if (!(mysql->mysql = mysqlnd_init(MYSQLND_CLIENT_KNOWS_RSET_COPY_DATA, TRUE)))
	#endif
	{
	efree(mysql);
	RETURN_FALSE;
	}

If (Z_MYSQLI_P(getThis()))->ptr could be NULL, and mysqli_init()/mysqlnd_init() would fail, the method returned false. If (Z_MYSQLI_P(getThis()))->ptr could not be NULL, calling the method would make no sense, and as such having the method would not.

[kamil-tekiela]

@cmb69 So if I understand you correctly the only possible return values for mysqli::init() would be void|false?
I have honestly no idea under what circumstances I would be able to force this method to return false.

[cmb69]

The point is whether (Z_MYSQLI_P(getThis()))->ptr is guaranteed to be set when php_mysqli_init() is called with is_method != 0. If so, that should be backed up by an assertion. If not, at least mysql_init() could fail due to OOM (not sure about mysqlnd_init()), causing the method to return false. Since void|false is not a valid type, the return type would be false|null. The method certainly does never return mysqli, indeed. (By the way, I find that "resource" wording pretty confusing.)

[kamil-tekiela]

@cmb69 The check for (Z_MYSQLI_P(getThis()))->ptr I assume is to prevent the object from being reinitialized once it already has been initialised.

class A extends mysqli{
    public function __construct() {
        var_dump(parent::init());
        $this->real_connect();
        var_dump(parent::init()); // <- this no longer does anything
    }
}

Either way, I really think we should remove this method. It's so useless.

[codecov-io]

Codecov Report

Merging #6540 (269936e) into master (bbae3dd) will decrease coverage by 0.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6540      +/-   ##
==========================================
- Coverage   72.66%   72.37%   -0.30%     
==========================================
  Files         800      816      +16     
  Lines      310075   311041     +966     
==========================================
- Hits       225329   225104     -225     
- Misses      84746    85937    +1191     

Impacted Files	Coverage Δ
ext/mbstring/libmbfl/filters/mbfilter_cp51932.c	0.00% <0.00%> (-96.73%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_sjis_mac.c	0.00% <0.00%> (-92.03%)	⬇️
ext/sodium/sodium_pwhash.c	6.17% <0.00%> (-69.14%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_euc_jp.c	49.39% <0.00%> (-46.88%)	⬇️
...xt/mbstring/libmbfl/filters/mbfilter_sjis_mobile.c	64.38% <0.00%> (-33.57%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_sjis_2004.c	25.85% <0.00%> (-28.34%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_cp932.c	75.33% <0.00%> (-21.50%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_sjis.c	77.45% <0.00%> (-20.26%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_utf8.c	77.70% <0.00%> (-17.04%)	⬇️
ext/mbstring/libmbfl/filters/mbfilter_ucs2.c	51.85% <0.00%> (-14.06%)	⬇️
... and 149 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbae3dd...514983f. Read the comment docs.

[cmb69]

The check for (Z_MYSQLI_P(getThis()))->ptr I assume is to prevent the object from being reinitialized once it already has been initialised.

That makes sense. However, that also implies that (Z_MYSQLI_P(getThis()))->ptr is not necessarily set when the method is called, and as such the return type in the stub matches the implementation.

Either way, I really think we should remove this method. It's so useless.

I generally find it confusing if dual APIs work differently for function and method. In this case, mysqli::init() should likely have been a static method, with the same semantics as mysqli_init(). Anyhow, removing the method or making it static is a BC break. :(