Skip to content

Copy and modify mb_str_pad from str_pad#2750

Merged
Girgias merged 8 commits into
php:masterfrom
youkidearitai:mb_str_pad_new_8.3
Oct 5, 2023
Merged

Copy and modify mb_str_pad from str_pad#2750
Girgias merged 8 commits into
php:masterfrom
youkidearitai:mb_str_pad_new_8.3

Conversation

@youkidearitai
Copy link
Copy Markdown
Contributor

I've mostly just copied from str_pad, so any pointers are welcome.

closes: #2701

Comment thread appendices/aliases.xml Outdated
@TimWolla TimWolla requested a review from ndossche September 7, 2023 17:45
Copy link
Copy Markdown
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, thanks for working on this.
Just some remarks.

Comment thread appendices/aliases.xml Outdated
Comment thread reference/mbstring/functions/mb-str-pad.xml Outdated
Comment thread reference/mbstring/functions/mb-str-pad.xml Outdated
Comment thread reference/mbstring/functions/mb-str-pad.xml Outdated
<varlistentry>
<term><parameter>encoding</parameter></term>
<listitem>
<para>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole paragraph may be replaced with the entity &mbstring.encoding.parameter; (the para tags should be removed too, i.e.: <listitem>&mbstring.encoding.parameter;</listitem>. (but I can't make a multi-line suggestion unfortunately)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I believe the note about the ValueError exception should be in a separate errors section.
https://www.php.net/manual/en/function.mb-strlen.php puts it in the error section, although it seems outdated because it claims a warning is raised instead of a ValueError... (https://3v4l.org/E9s0i)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed an issue for the error conditions: #2751

@youkidearitai
Copy link
Copy Markdown
Contributor Author

@nielsdos @TimWolla Thanks very much! I fixed.

<term><parameter>length</parameter></term>
<listitem>
<para>
If the value of <parameter>length</parameter> is negative,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is no ValueError emitted for negative lengths?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no ValueError throws.

$ sapi/cli/php -r 'var_dump(mb_str_pad("あいうえお", -333, "🎉", STR_PAD_BOTH));'
string(15) "あいうえお"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sad :(

Comment thread reference/mbstring/functions/mb-str-pad.xml Outdated
Comment thread reference/mbstring/functions/mb-str-pad.xml Outdated
@Girgias
Copy link
Copy Markdown
Member

Girgias commented Sep 8, 2023

This also needs an entry in reference/mbstring/versions.xml to indicate it is available as of 8.3

@youkidearitai
Copy link
Copy Markdown
Contributor Author

This also needs an entry in reference/mbstring/versions.xml to indicate it is available as of 8.3

@Girgias Thank you very much. Add to reference/mbstring/versions.xml .

@Girgias Girgias added this to the PHP 8.3 milestone Oct 5, 2023
@Girgias Girgias merged commit 4708408 into php:master Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add manual for mb_str_pad function since PHP 8.3

4 participants