[RFC] Add mb_ucfirst and mb_lcfirst functions

[youkidearitai]

Ref: #13075

Adds mb_ucfirst and mb_lcfirst functions.

RFC: https://wiki.php.net/rfc/mb_ucfirst

[nielsdos]

Nice work, preliminary feedback 🙂

[Ayesh]

@youkidearitai - Thanks for changing the case conversion to titlecase, and resetting the RFC vote. Although the process has restarted, I think you made the right call.

[youkidearitai]

@Ayesh Thanks for your support.

[youkidearitai]

This RFC has been accepted, change to Normal Pull Request.

[Ayesh]

Congratulations on passing this RFC!
Before merging, please add the entries to UPGRADING and NEWS files.

[youkidearitai]

@Ayesh Thank you! Added.

[nielsdos]

Looks pretty good, good work!
I have some minor comments. I also don't see an obvious way of making this more performant.
Maybe Alex knows though.

[alexdowad]

@youkidearitai Great job!

A good way to optimize this code for performance would be to first check if the first character is already in the desired case. If it is, just use zend_string_copy to increment the refcount of the input string and return it.

[youkidearitai]

@alexdowad

A good way to optimize this code for performance would be to first check if the first character is already in the desired case. If it is, just use zend_string_copy to increment the refcount of the input string and return it.

Thank you. I tried it. Please confirm.

[nielsdos]

Lgtm

[youkidearitai]

@alexdowad Could you review this pull request at free time?

[alexdowad]

@alexdowad Could you review this pull request at free time?

@youkidearitai Certainly. 😄 I am sorry for the delay.

The code looks great. I would just like to ask if you can kindly add some more test cases. As a start, how would it be to test all ASCII chars, both for mc_ucfirst and mc_lcfirst?

You have tried empty strings, which is good. Maybe also try some slightly longer strings, string with NUL bytes...

Oh, and although you allow passing an explicit $encoding parameter, there are no test cases for that. I suggest adding test cases for at least several different explicit $encoding values.

[youkidearitai]

@alexdowad Thank you for review! Added test case.

[Ayesh]

(Please feel free to completely ignore this very minor nitpick)

I find the tests a little bit difficult to grok. I think it will be much easier if we have a series of strict string comparisons, so both input and expected strings are in the same line:

var_dump(mb_ucfirst('abc') === 'Abc');
var_dump(mb_ucfirst('đắt quá!') === 'Đắt quá!');
var_dump(mb_ucfirst("\u{01CA}\u{01CA}") === "\u{01CB}\u{01CA}");

They are now compared against bool(true) var_dump outputs.

This will make test diffs not as verbose because they no longer show the actual difference, but I doubt they will be useful in the first place because the diffs will be in various encodings and some of the character differences are not so easily seen on terminal fonts.

I made a polyfill for this function, and the tests are like this.

[youkidearitai]

@Ayesh
Yes, You have a point.
But I wrote this .phpt file because I think to easy to see if test fails to .diff files.
Thank you.

[alexdowad]

Great job! Thanks very much!

[alexdowad]

@youkidearitai I am sorry that I took some time to review. Your work is really appreciated.

[youkidearitai]

@youkidearitai I am sorry that I took some time to review. Your work is really appreciated.

@alexdowad No problem! Thank you very much to approved!

[nielsdos]

Merged, thanks!

[youkidearitai]

Thanks very much!