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

[PHP 8.3] Polyfill mb_str_pad() #435

Merged
merged 1 commit into from Jul 28, 2023

Conversation

IonBazan
Copy link
Contributor

@IonBazan IonBazan commented Jul 6, 2023

Polyfills the mb_str_pad() function added in PHP 8.3: https://wiki.php.net/rfc/mb_str_pad

Test cases were taken from the RFC implementation and adapted to PHPUnit.

@IonBazan IonBazan force-pushed the feature/mb_str_pad branch 3 times, most recently from fbad789 to a70e0af Compare July 6, 2023 05:37
@Ayesh
Copy link
Contributor

Ayesh commented Jul 6, 2023

I'm sorry if I'm missing something documented elsewhere, but I wanted to ask if this should belong to Mbstring or Php83? Perhapas, before declaring polyfilled mb_str_pad, we should see if the mbstring exists either as the C extension or in the form of a polyfill by checking function_exists('mb_substr')?

@IonBazan
Copy link
Contributor Author

IonBazan commented Jul 6, 2023

Good question - I wasn't sure because we don't check it for PHP 7.2 polyfill (mb_chr and mb_ord) but we do check it for PHP 7.4 (mb_str_split):

if (!function_exists('mb_str_split') && function_exists('mb_substr')) {
function mb_str_split($string, $length = 1, $encoding = null) { return p\Php74::mb_str_split($string, $length, $encoding); }
}

I guess we can add a check for mb_substr if needed here.

@stof
Copy link
Member

stof commented Jul 6, 2023

the new function should be added in 2 places

  • in the php 8.3 polyfill if other mbstring functions are available
  • in the mbstring polyfill (so that it is available for someone using PHP 8.3 natively without the native mbstring extension)

@IonBazan
Copy link
Contributor Author

PHP 7.2 and 7.1 don't seem to handle UTF-32 strings correctly when mbstring is missing 🤔
@nicolas-grekas Should I skip those tests?

@nicolas-grekas
Copy link
Member

PHP 7.2 and 7.1 don't seem to handle UTF-32 strings correctly when mbstring is missing

that's quite possible, let's skip these yes (or fix the polyfill, but nobody uses utf32 so 🤷 )

@IonBazan IonBazan force-pushed the feature/mb_str_pad branch 3 times, most recently from 4a3c5fd to b90b63b Compare July 28, 2023 08:58
@IonBazan
Copy link
Contributor Author

Thanks for the suggestion, @nicolas-grekas - I rebased this PR and skipped the tests for UTF-32 on PHP <7.3

@nicolas-grekas
Copy link
Member

Thank you @IonBazan.

@nicolas-grekas nicolas-grekas merged commit c38c303 into symfony:main Jul 28, 2023
7 of 9 checks passed
@IonBazan IonBazan deleted the feature/mb_str_pad branch July 28, 2023 09:06
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