-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Rename mysqli parameters to be more logical. #6172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
inmysqli_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 inext/openssl
)$cipher
->$cipher_algorithms
(we have precedence for this inext/openssl
)
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. |
$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? |
$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? |
9ad915b
to
dce6e65
Compare
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 |
Yes, this should definitely be
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. |
In the docs Given that this only appears on the procedural APIs (right?), I think |
|
As suggested over on #6153, we could also use |
The |
The https://www.php.net/manual/en/mysqli.kill.php I would personally choose |
I still think |
Updated again per recent comments. See the issue summary for the list of what is changing now. |
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 |
20d7e40
to
a3b7246
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -120,7 +120,7 @@ public function init() {} | |||
* @return bool | |||
* @alias mysqli_kill | |||
*/ | |||
public function kill(int $connection_id) {} | |||
public function kill(int $process_id) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
As per my previous comment, this should probably be named |
I don't think |
Another datapoint: The pgsql extension uses |
c3c3ef2
to
baab111
Compare
$enabled
, not$mode
._nr
changed to$index
, which is more self-descriptive.$attribute
/$value
pair.$user
/$username
/ for consistency.$class_name/
$class`/ for consistency.$string_to_escape
/$string
/ because seriously?$dbname
/$database
/ for consistency.$mysql_link
/$mysqli
/ for clarity/consistency.$mysql_stmt
/$stmt
/ for brevity.$mysqli_result
/$result
/ for brevity.$sec
/$seconds
/ for clarity.$usec
/$microseconds
/ for clarity.$flags
.$flags
/$mode
/ on one function for correctness. (It's a single-value setting, thus not a flags bitmask.)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.