Update mbstring parameter names #6207

nikic · 2020-09-24T14:21:46Z

The mb_str_split change is applied to str_split() as well.

The mb_ereg functions might need another look when we update pcre, as there are some parallels there.

dtakken · 2020-09-24T19:45:13Z

ext/mbstring/mbstring.stub.php

-function mb_ereg_replace(string $pattern, string $replacement, string $string, ?string $option = null): string|false|null {}
+function mb_ereg_replace(string $pattern, string $replacement, string $string, ?string $options = null): string|false|null {}


The similarity with the pcre extension was already mentioned. In that context I would probably rename $option to $modifiers.

However that would also mean the documentation text needs updating. Which... would not be a bad idea anyways. It speaks about "options" and then lists all possible "options" which also include "modes". So options can also contain a mode. Confusing.

https://www.php.net/manual/en/function.mb-regex-set-options.php

Maybe it should speak about "behavior modifiers" and "syntax modifiers" in stead.

Right, modifiers is a possible alternative term. However, the referenced function is called mb_regex_set_options(), so that seems like the preferred term for mb_regex. (PCRE also never exposes "modifiers" in parameters, as they are part of the regex -- as such, there is no naming consistency to upkeep in that respect at least.)

dtakken · 2020-09-24T19:53:48Z

ext/mbstring/mbstring.stub.php

-function mb_convert_variables(string $to, array|string $from, mixed &$var, mixed &...$vars): string|false {}
+function mb_convert_variables(string $to_encoding, array|string $from_encoding, mixed &$var, mixed &...$vars): string|false {}


Suggestion:

$from / $to -> $decode / $encode

Not sure I understand the suggestion. Are you saying the parameter names should be $encode and $decode or $encode_encoding and $decode_encoding? Both don't really make sense to me.

My suggestion was to name them $encode and $decode. My idea was that $decode says how to decode the string while $encode says how to encode it again. It made me think of how it would be written in Python:

my_string.decode('iso-8859-1').encode('utf8')

The names are short too. To me it does make sense, but that could just be me. :)

I'm happy to leave these at just $to and $from if the _encoding suffix makes them too long.

P.S. I like their current names ^^ But unfortunately I can only have a closer look a bit later.

Just to mention a few other options: UConverter::__construct() uses $source_encoding and $destination_encoding ($target_encoding reads better for me though).

kocsismate

A few other ideas:

mb_http_input(): what about using the $type_code param name? Although, $type also works for me.
mb_parse_str(): would just $string be clear enough?
mb_output_handler(): would the $string param name make sense?
mb_substr(): it uses $start while the other similar params are called $offset. I'm not sure though which name should be preferred. Maybe $start is a tiny little bit more clear.
mb_strimwidth: Could we still use $length instead of $with, in spite of the function name? I'm ok either way though, just wanted to highlight this inconsistency compared to the other param names :)
mb_detect_encoding(): what do you think about $encodings instead of $encoding_list? I hardly ever saw the latter construct in use in php-src.
mb_ereg*(): would $matches work instead of $registers, similar to what ext/pcre uses?

kocsismate · 2020-09-27T14:28:15Z

ext/mbstring/mbstring.stub.php

@@ -65,29 +65,29 @@ function mb_list_encodings(): array {}

 function mb_encoding_aliases(string $encoding): array {}

-function mb_encode_mimeheader(string $string, ?string $charset = null, ?string $transfer = null, string $linefeed = "\r\n", int $indent = 0): string {}
+function mb_encode_mimeheader(string $string, ?string $charset = null, ?string $transfer_encoding = null, string $linefeed = "\r\n", int $indent = 0): string {}


Synonyms for "line feed" according to Wikipedia: new line, line ending, end of line, or line break. I think a few of them would qualify as a better (more well-known) name. E.g. $newline? If you don't agree, can you use $line_feed? As far as I saw it should be spelled as two words.

I went with $newline here, agree that line feed is uncommon.

kocsismate · 2020-09-27T14:35:18Z

ext/mbstring/mbstring.stub.php

-function mb_convert_variables(string $to, array|string $from, mixed &$var, mixed &...$vars): string|false {}
+function mb_convert_variables(string $to_encoding, array|string $from_encoding, mixed &$var, mixed &...$vars): string|false {}


Just to mention a few other options: UConverter::__construct() uses $source_encoding and $destination_encoding ($target_encoding reads better for me though).

kocsismate · 2020-09-27T14:38:11Z

ext/mbstring/mbstring.stub.php


-function mb_encode_numericentity(string $string, array $convmap, ?string $encoding = null, bool $is_hex = false): string {}
+function mb_encode_numericentity(string $string, array $map, ?string $encoding = null, bool $use_hex = false): string {}


hmm... So should we use the use_/return_ prefix in case of hash_hmac() et al.?

I changed this to just $hex to be consistent with what we did for other bool parameters. In this case, there doesn't seem to be a potential to misinterpret this is a non-boolean parameter.

Yeah, I also had some doubts about the use_ name in the ext/standard PR, so 👍

kocsismate · 2020-09-27T14:40:06Z

ext/mbstring/mbstring.stub.php


-function mb_decode_numericentity(string $string, array $convmap, ?string $encoding = null): string {}
+function mb_decode_numericentity(string $string, array $map, ?string $encoding = null): string {}

 function mb_send_mail(string $to, string $subject, string $message, array|string $additional_headers = [], ?string $additional_parameters = null): bool {}


WDYT?

Suggested change

function mb_send_mail(string $to, string $subject, string $message, array|string $additional_headers = [], ?string $additional_parameters = null): bool {}

function mb_send_mail(string $to, string $subject, string $message, array|string $additional_headers = [], ?string $additional_params = null): bool {}

Although, as I wrote in the PDO rename PR, we could/should set up some rules for using $param and $arg.

nikic · 2020-09-27T20:07:51Z

A few other ideas:

* `mb_http_input()`: what about using the `$type_code` param name? Although, `$type` also works for me.

I slightly prefer just $type here, because $type_code sounds like an integer to me.

* `mb_parse_str()`: would just `$string` be clear enough?

Yeah, done.

* `mb_output_handler()`: would the `$string` param name make sense?

I think so, done. The https://www.php.net/ob_start docs call this $buffer, but $string seems fine as well.

* `mb_substr()`: it uses `$start` while the other similar params are called `$offset`. I'm not sure though which name should be preferred. Maybe `$start` is a tiny little bit more clear.

This is a tricky one to which I don't have an answer. I think that generally the $start+$length terminology is good, but with the strpos family, we get the added problem that we also have strrpos, in which case $offset can also be the end of the search (if negative).

* `mb_strimwidth`: Could we still use `$length` instead of `$with`, in spite of the function name? I'm ok either way though, just wanted to highlight this inconsistency compared to the other param names :)

I think I'd prefer to keep $width here, the distinction seems sufficiently important.

* `mb_detect_encoding()`: what do you think about `$encodings` instead of `$encoding_list`? I hardly ever saw the latter construct in use in php-src.

Agreed, done.

* `mb_ereg*()`: would `$matches` work instead of `$registers`, similar to what ext/pcre uses?

I initially kept this because the "registers" terminology is part of the mb_ereg_search_getregs() API name. However, I agree that $matches is the de-facto standard for this. ext/pcre stubs currently use $subpattern, we'll want to change it there as well.

kocsismate · 2020-09-27T22:12:26Z

LGTM! :)

nikic added the Parameter names label Sep 24, 2020

nikic requested a review from kocsismate September 24, 2020 14:21

nikic mentioned this pull request Sep 24, 2020

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

Closed

65 tasks

dtakken reviewed Sep 24, 2020

View reviewed changes

nikic force-pushed the mbstring-names branch from 27da9b5 to 195dfb0 Compare September 25, 2020 08:10

kocsismate reviewed Sep 27, 2020

View reviewed changes

nikic added 2 commits September 27, 2020 21:51

Update mbstring names

d02fc96

Review updates

086f58f

nikic force-pushed the mbstring-names branch from 195dfb0 to 086f58f Compare September 27, 2020 19:59

Fix test

74f2222

php-pulls closed this in cafceea Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update mbstring parameter names #6207

Update mbstring parameter names #6207

nikic commented Sep 24, 2020

dtakken Sep 24, 2020

nikic Sep 25, 2020

dtakken Sep 24, 2020

nikic Sep 25, 2020

dtakken Sep 25, 2020

nikic Sep 25, 2020

kocsismate Sep 25, 2020

kocsismate Sep 27, 2020

kocsismate left a comment •

edited

Loading

kocsismate Sep 27, 2020

nikic Sep 27, 2020

kocsismate Sep 27, 2020

kocsismate Sep 27, 2020

nikic Sep 27, 2020

kocsismate Sep 27, 2020

kocsismate Sep 27, 2020

nikic Sep 27, 2020

nikic commented Sep 27, 2020

kocsismate commented Sep 27, 2020

		function mb_ereg_replace(string $pattern, string $replacement, string $string, ?string $option = null): string\|false\|null {}
		function mb_ereg_replace(string $pattern, string $replacement, string $string, ?string $options = null): string\|false\|null {}

		function mb_convert_variables(string $to, array\|string $from, mixed &$var, mixed &...$vars): string\|false {}
		function mb_convert_variables(string $to_encoding, array\|string $from_encoding, mixed &$var, mixed &...$vars): string\|false {}


		function mb_encode_numericentity(string $string, array $convmap, ?string $encoding = null, bool $is_hex = false): string {}
		function mb_encode_numericentity(string $string, array $map, ?string $encoding = null, bool $use_hex = false): string {}

	function mb_send_mail(string $to, string $subject, string $message, array\|string $additional_headers = [], ?string $additional_parameters = null): bool {}
	function mb_send_mail(string $to, string $subject, string $message, array\|string $additional_headers = [], ?string $additional_params = null): bool {}

Update mbstring parameter names #6207

Update mbstring parameter names #6207

Conversation

nikic commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Sep 27, 2020

kocsismate commented Sep 27, 2020

kocsismate left a comment •

edited

Loading