Skip to content
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

[8.x] Add multibyte support to string padding helper functions #41899

Merged
merged 4 commits into from Apr 11, 2022

Conversation

clarkewing
Copy link
Contributor

This PR adds support for multibyte strings to the Str::padBoth, Str::padLeft, and Str::padRight helper functions.

This is especially useful for localized strings, where accented characters or other multibyte glyphs were incorrectly padded before.

PHP has yet to implement a native mb_str_pad function so this PR adds a Str::mbStrPad helper function adapted from a comment on the PHP docs. There is an ongoing 19 year-old PHP request for this native function, but as it is yet to see any progress, it seems appropriate to implement a workaround in the framework.

@GrahamCampbell
Copy link
Member

Laravel 8 is closed to new features. This PR adds a new non-private method.

@GrahamCampbell GrahamCampbell changed the title Add multibyte support to string padding helper functions [8.x] Add multibyte support to string padding helper functions Apr 9, 2022
@clarkewing
Copy link
Contributor Author

Thanks for the speedy reply @GrahamCampbell! I made the Str::mbStrPad method protected. As it's my first time contributing to the framework, I'm open to any feedback.

@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Apr 9, 2022

It would probably be better to simply inline the change in every method instead of adding a protected helper. There are no "helper" methods for the other string methods that have multibyte support.

Only the second parameter is modified :

    public static function padRight($value, $length, $pad = ' ')
    {
        return str_pad($value, strlen($value) - mb_strlen($value) + $length, $pad, STR_PAD_RIGHT);
    }

I think this could be considered a bug fix and should target 8.x, but see what Taylor thinks.

@clarkewing
Copy link
Contributor Author

Thanks for the helpful feedback @BrandonSurowiec! I committed a fix 😊

@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Apr 9, 2022

Looks good to me. I think it's ready to go 🔥

@taylorotwell taylorotwell merged commit 53c7425 into laravel:8.x Apr 11, 2022
@clarkewing clarkewing deleted the mb-strpad branch April 13, 2022 04:26
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.

None yet

4 participants