Rename mysqli parameters to be more logical. #6172

Crell · 2020-09-20T01:04:59Z

autocommit()'s boolean parameter now called $enabled, not $mode.
Various parameters named _nr changed to $index, which is more self-descriptive.
attr mode changed to use a more traditional $attribute/$value pair.
s/$user/$username/ for consistency.
s/$class_name/$class`/ for consistency.
s/$string_to_escape/$string/ because seriously?
s/$dbname/$database/ for consistency.
s/$mysql_link/$mysqli/ for clarity/consistency.
s/$mysql_stmt/$stmt/ for brevity.
s/$mysqli_result/$result/ for brevity.
s/$sec/$seconds/ for clarity.
s/$usec/$microseconds/ for clarity.
Bitmask parameters standardized on $flags.
s/$flags/$mode/ on one function for correctness. (It's a single-value setting, thus not a flags bitmask.)
Various parameters in the SSL methods renamed to match their ext/ssl equivalents.

Still an open question:

$fetchtype is a bad name. $fetch_type is not much better. $result_mode is also used for the same thing, I think. PDO uses $fetch_style. We should standardize all of those, but probably in a separate PR that is cross-DB.

kocsismate

I found a couple of other parameters which should be considered to be renamed:

$user -> $username (ext/pdo also uses this)
$mysql_result -> $result
fetchtype -> $type, or at least $fetch_type (the manual uses $resulttype)
$class_name -> $class is already clear enough (discussed this during renaming Zend params)
$mysqli_link -> $link
all the parameters of mysqli_poll() (e.g. $error -> $failed_connections or something similar)
$result_mode -> $mode (maybe)
$string_to_escape -> $string
$dbname -> $database
$mysql_stmt -> $statement
maybe $types -> $type_list/$type_mask in mysqli_stmt_bind_param() (trim() uses the "mask" name in the 2nd param)
$cert to $certificate (ext/openssl already uses this)
$certificate_authority -> $ca (it's used in ext/openssl)
$cipher -> $cipher_algorithms (we have precedence for this in ext/openssl)

ext/mysqli/mysqli.stub.php

Crell · 2020-09-20T19:11:54Z

Re $fetch_type/$fetchtype, PDO uses $fetch_style. I was going to skip that for now and come back to it later to standardize across all the extensions that have that parameter, since I'm not sure which is best.

Crell · 2020-09-20T19:13:41Z

$mysql_link/$link - $link seems very generic and not entirely self-describing here. I've never seen anyone in practice actually call the connection object a "link". I agree $mysql_link is no good, but I don't think $link is an improvement. Let's come up with something better. $connection? $conn?

Crell · 2020-09-20T19:15:27Z

$mysql_stmt -> $statement - The class is called stmt, the methods and functions are all called stmt. It's a stupid abbreviation but shouldn't we favor consistency?

markrandall · 2020-09-20T23:10:42Z

Let's come up with something better. $connection? $conn?

I'm fine with $connection as most of the main written documentation refers to the connection rather than link as can be observed at https://www.php.net/manual/en/mysqli.summary.php

nikic · 2020-09-21T07:35:56Z

$mysql_stmt -> $statement - The class is called stmt, the methods and functions are all called stmt. It's a stupid abbreviation but shouldn't we favor consistency?

Yes, this should definitely be $stmt, not $statement.

Let's come up with something better. $connection? $conn?

I also like $connection. How well does that translate to other exts? E.g. #6153 currently normalizes to $link, but I think $connection works there as well.

morrisonlevi · 2020-09-23T01:19:15Z

In the docs connection is more common than link in prose, but $link is more common for the documented parameter.

Given that this only appears on the procedural APIs (right?), I think $self is a good candidate. It's short, and it's commonly named like that in at least a few languages for situations like this. What do you think about that?

Crell · 2020-09-23T01:20:57Z

$self would make a ton of sense to people used to Rust, or Python. Probably not to anyone else. I'd prefer a self-descriptive noun, whatever it is.

nikic · 2020-09-23T07:51:48Z

As suggested over on #6153, we could also use $mysqli (or $ldap there). TBH that probably lines up most with how I would actually call it in code (maybe not for $mysqli specifically where I might use $db, but for most other "resource-like" variables it does.)

dtakken · 2020-09-23T14:56:44Z

The debug() / mysqli_debug() functions have a parameter that is currently named debug_options / debug. In the documentation it is called message, which I think is a better name:

https://www.php.net/manual/en/mysqli.debug

dtakken · 2020-09-23T15:20:36Z

The kill() / mysqli_kill() functions currently have a $connection_id parameter. This parameter is actually a process identifier, not a connection identifier. It refers to an entry in the process list of the database server:

https://www.php.net/manual/en/mysqli.kill.php

I would personally choose $process here.

morrisonlevi · 2020-09-23T15:22:11Z

I still think $self is better, though apparently you guys don't. It's short, it's pretty normal for this "pass $this object as first parameter" scenario, and we don't have to this discussion again. This is the last I'll mention it; I just think it's both an easier path and happens to be short, which is nice if anyone actually does use this for named parameters, which is partly the motivation for the parameter name work.

ext/mysqli/mysqli.stub.php

Crell · 2020-09-24T01:08:36Z

Updated again per recent comments. See the issue summary for the list of what is changing now.

johannes · 2020-09-24T10:47:31Z

Yeah, many of the names are from last century and naming would be different if redone today :-)

Most of the proposed names are fine to me, but I want to point out one thing regarding this unfortunate link term: It is also used in php.ini mysqli.max_links and in mysqli_get_links_stats and in phpinfo() output. If we wouldn't have so many aliases in mysqli already I would tend to rename those as well, but I fear this makes it more of a mess.

nikic

Some notes where I couldn't place review comments:

public function debug(string $debug_options) {}
// ->
public function debug(string $options) {}

Th debug_ prefix here seems redundant.

public function query(string $query, int $result_mode = MYSQLI_STORE_RESULT) {}
// ->
public function query(string $query, int $flags = MYSQLI_STORE_RESULT) {}

As the documentation mentions, this is actually a flags parameter that also accepts MYSQLI_ASYNC. What it doesn't mention is that it also accepts MYSQLI_STORE_RESULT_COPY_DATA as a flag.

(Also applies to mysql_result::__construct().)

public function refresh(int $options) {}
// ->
public function refresh(int $flags) {}

In line with other changes.

nikic · 2020-09-25T09:13:10Z

ext/mysqli/mysqli.stub.php

@@ -120,7 +120,7 @@ public function init() {}
     * @return bool
     * @alias mysqli_kill
     */
-    public function kill(int $connection_id) {}
+    public function kill(int $process_id) {}


Suggested change

public function kill(int $process_id) {}

public function kill(int $thread_id) {}

Given that the corresponding getter is mysql_thread_id(), possibly use that term? I'll admit that the used terminology here is really inconsistent though, so I'm also fine with $process_id.

ext/mysqli/mysqli.stub.php

nikic · 2020-09-25T09:22:25Z

ext/mysqli/mysqli.stub.php

@@ -278,7 +278,7 @@ public function stmt_init() {}
     * @return mysqli_result|false
     * @alias mysqli_store_result
     */
-    public function store_result(int $flags = 0) {}
+    public function store_result(int $mode = 0) {}


I'm a bit conflicted about this change. Yes, there currently is only a single flag. Logically this is the MYSQLI_USE_RESULT | MYSQLI_STORE_RESULT | MYSQLI_STORE_RESULT_COPY_DATA bitset, just with this case hardcoding MYSQLI_STORE_RESULT, thus leaving only a single flag one can toggle.

ext/mysqli/mysqli.stub.php

dtakken · 2020-09-25T09:56:08Z

public function debug(string $debug_options) {}
// ->
public function debug(string $options) {}

Th debug_ prefix here seems redundant.

As per my previous comment, this should probably be named $message. It looks like the value is actually a message sent to a debugger.

nikic · 2020-09-25T10:00:07Z

As per my previous comment, this should probably be named $message. It looks like the value is actually a message sent to a debugger.

I don't think $message makes for a good name here. debug($message) makes it look like a debug logging function. The name should be $options, $config, or something along those lines.

Crell · 2020-09-26T15:04:38Z

Another datapoint: The pgsql extension uses $connection. 😦

Refs GH-6172.

bwoebi mentioned this pull request Sep 20, 2020

Consistent named param: Extension Tracker php/php-tasks#23

Closed

65 tasks

kocsismate reviewed Sep 20, 2020

View reviewed changes

ext/mysqli/mysqli.stub.php Outdated Show resolved Hide resolved

ext/mysqli/mysqli.stub.php Outdated Show resolved Hide resolved

ext/mysqli/mysqli.stub.php Outdated Show resolved Hide resolved

ext/mysqli/mysqli.stub.php Outdated Show resolved Hide resolved

Crell force-pushed the mysqli-params branch from 9ad915b to dce6e65 Compare September 20, 2020 22:56

Crell mentioned this pull request Sep 21, 2020

ext/ldap parameter fixes #6153

Closed

nikic mentioned this pull request Sep 23, 2020

Fix parameter names (mysqli) #6192

Closed

dtakken suggested changes Sep 23, 2020

View reviewed changes

nikic added the Parameter names label Sep 24, 2020

Crell force-pushed the mysqli-params branch from 20d7e40 to a3b7246 Compare September 24, 2020 15:46

nikic reviewed Sep 25, 2020

View reviewed changes

Rename mysqli parameters to be more logical.

baab111

Crell force-pushed the mysqli-params branch from c3c3ef2 to baab111 Compare September 27, 2020 16:26

Fix test.

e10766c

nikic approved these changes Sep 27, 2020

View reviewed changes

php-pulls closed this in 02dc9ce Sep 27, 2020

Crell deleted the mysqli-params branch September 27, 2020 22:42

php-pulls pushed a commit that referenced this pull request Sep 28, 2020

Fixed test for MySQL < 5.6

d411ea2

Refs GH-6172.

kocsismate mentioned this pull request Jan 4, 2021

Generate mysqli methodsynopses based on stubs php/doc-en#301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename mysqli parameters to be more logical. #6172

Rename mysqli parameters to be more logical. #6172

Crell commented Sep 20, 2020 •

edited

kocsismate left a comment

Crell commented Sep 20, 2020

Crell commented Sep 20, 2020

Crell commented Sep 20, 2020

markrandall commented Sep 20, 2020

nikic commented Sep 21, 2020

morrisonlevi commented Sep 23, 2020

Crell commented Sep 23, 2020

nikic commented Sep 23, 2020

dtakken commented Sep 23, 2020

dtakken commented Sep 23, 2020

morrisonlevi commented Sep 23, 2020

Crell commented Sep 24, 2020

johannes commented Sep 24, 2020

nikic left a comment

nikic Sep 25, 2020

nikic Sep 25, 2020

dtakken commented Sep 25, 2020

nikic commented Sep 25, 2020

Crell commented Sep 26, 2020

	public function kill(int $process_id) {}
	public function kill(int $thread_id) {}

Rename mysqli parameters to be more logical. #6172

Rename mysqli parameters to be more logical. #6172

Conversation

Crell commented Sep 20, 2020 • edited

kocsismate left a comment

Choose a reason for hiding this comment

Crell commented Sep 20, 2020

Crell commented Sep 20, 2020

Crell commented Sep 20, 2020

markrandall commented Sep 20, 2020

nikic commented Sep 21, 2020

morrisonlevi commented Sep 23, 2020

Crell commented Sep 23, 2020

nikic commented Sep 23, 2020

dtakken commented Sep 23, 2020

dtakken commented Sep 23, 2020

morrisonlevi commented Sep 23, 2020

Crell commented Sep 24, 2020

johannes commented Sep 24, 2020

nikic left a comment

Choose a reason for hiding this comment

nikic Sep 25, 2020

Choose a reason for hiding this comment

nikic Sep 25, 2020

Choose a reason for hiding this comment

dtakken commented Sep 25, 2020

nikic commented Sep 25, 2020

Crell commented Sep 26, 2020

Crell commented Sep 20, 2020 •

edited