Skip to content

Conversation

@capuderg
Copy link

As discussed in this PR: symfony/polyfill-mbstring#6, this would be a potential fix for any corner cases.

Currently, we have issue with mb_strtolower not being polyfilled, because the current check only checks if mb_strlen exists.

In WordPress the mb_strlen function is polyfilled before this package can be required, but the mb_strtolower is not (don't know why WP is polyfilling just a few).

With this change, all mb_* functions can be polyfilled separately.

@stof
Copy link
Member

stof commented Apr 29, 2020

If we decide that we want to go that road, we should do it for all polyfills.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 29, 2020

If we decide that we want to go that road, we should do it for all polyfills.

I agree - and then, what is now a few tenths of extra checks might become hundreds. We should definitely run some benchmarks before doing this.

@JanJakes
Copy link
Contributor

JanJakes commented Apr 29, 2020

@nicolas-grekas In many cases (like PHP version polyfills) you won't add much, almost nothing in fact, since most of the IFs are already there (i.e. https://github.com/symfony/polyfill/blob/master/src/Php54/bootstrap.php).

And then some polyfills may not need this at all (classes? not sure which ones).

And I guess even if you add hundreds it will still be a fraction of a millisecond but yes, a benchmark would be good to prove such a guess.

@stof
Copy link
Member

stof commented Apr 29, 2020

@nicolas-grekas isn't function_exists optimized by OPCache, so that it is optimized-away entirely when you have the extension loaded anyway ?

@JanJakes
Copy link
Contributor

I prepared a PR handling this across all polyfills - #252. Let's see where it leads us.

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.

4 participants