-
-
Notifications
You must be signed in to change notification settings - Fork 39
Fix for WordPress. #6
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
Conversation
I'm not sure this solves the issue: https://github.com/WordPress/WordPress/blob/master/wp-includes/compat.php
When mb_strpos is not found, this will redeclare mb_strlen, thus fail, isn't it?
Yes, you are right, I was too fast. In my local install, I use
if ( ! extension_loaded('mbstring') ) {
and this works for me. I tried to make PR with minimal changes, but made is wrong. Why don't you use extension_loaded ? Should I fix PR with it?
extension_loaded has the very same issue...
the best way is to do nothing here, but to load this file before wordpress loads its compat.php file
It is simply impossible without modification of WordPress core. This is an early stage of core load, and no plugins or theme are included at this moment.
Last fix is tested in WordPress plugin.
thanks, works for me,
can you please fix the code style? we use 4 spaces for indentation
In WordPress core, on the very early stage, the file wp-includes/compat.php is loaded. It defines polyfills for some mb_ functions, including mb_strlen. After this, Symfony\Polyfill\Mbstring becomes useless in any plugin / theme. This causes fatal errors on mb_ function calls.
bd1380e to
7726569
Compare
I hope did it right :) (my phpStorm is tuned for WordPress Coding Standards). Also, squashed 4 commits.
Could you tag this in case of acceptance? I will update our repositories.
Thank you.
@kagg-design @nicolas-grekas In WP world any plugin can define (polyfill) any of the functions if it needs it. Therefore, this fix is not sufficient - you'd need to wrap every single function and constant in its IF statement. I can prepare a PR if that sounds acceptable to you.
@JanJakes not sure: this would slow down everyone. I'd prefer doing it only when there is an actual conflict spotted by the community. Long term, I'd prefer plugins stop doing this... If you know about other functions, please tell us which ones.
@kagg-design can you please reopen this PR against https://github.com/symfony/polyfill? I totally missed this was open on the wrong repo (this one is created by a bot from the main one).
@nicolas-grekas Thanks for the quick response!
not sure: this would slow down everyone.
Would it? Do you think a difference between a single function_exists and i.e. 20 of them is measurable? I don't think so. Even in online PHP testers, you can run if (function_exists('mb_strlen')) {} 20 million times within a second which would be 0.00005 ms per run which means that you could run 20 000 of them within a single millisecond. (Note that I didn't subtract the loop overload so in reality, it's even faster.)
Adding every single polyfilled item in its own IF statement would have nice advantages - you'd never have any more PR's fixing the bootstrapping logic (such as this one) and you'd avoid all conflicts by design (yes, especially in WP world that needs polyfills often, but also in other projects where devs define a few functions by themselves, for instance).
If that would sound considerable, I could prepare a PR on the main repo.
capuderg
commented
Apr 29, 2020
I would agree with @JanJakes, we are having issues with mb_strtolower and this function is not checked in this current PR.
Some other devs might have issues with some other function, so wrapping each separately would probably be the best way.
Thanks!
capuderg
commented
Apr 29, 2020
Hey,
I prepared the separate function checks in PR #7. I've changed the file for a user of our plugin to test it out and so had it prepared and decided to open a PR.
Let me know if I should change something.
Have a nice day!
Lets' continue on symfony/polyfill
Please link here when opening a PR there.
...ants (JanJakes) This PR was merged into the 1.16-dev branch. Discussion ---------- Add separate checks for all polyfilled functions and constants As originally discussed in symfony/polyfill-mbstring#6 and later in #251 in some scenarios it may be better to check for each constant/function before defining it. Such scenarios are typically those where some project or a library defines its polyfills for some parts of extension functionality but not all of it. Probably the most popular example is WordPress, defining some polyfills in `wp-includes/compat.php` but not including complete functionalities (i.e. it defines only a few `mb_` functions). Plugin developers then may need other `mb_` functions and need to load those that WordPress didn't. There may be some performance concerns but it seems that checks such as `function_exists` are [very fast](symfony/polyfill-mbstring#6 (comment)) - in fact, [PHP tries to evaluate both `function_exists` and `defined` at compile time](https://github.com/php/php-src/blob/e141592da6cdb15709e4aeaab5421c9a46564849/ext/opcache/Optimizer/pass1_5.c#L349) (see also [StackOverflow](https://stackoverflow.com/questions/21619939/does-function-exists-cache-its-queries/21620062#21620062)). This means on installs that do have the polyfilled extensions installed, there should be zero overhead (the conditions will evaluate to `true` at compile time). Note that many of the currently existing polyfills already perform checks for each function separately (most of the PHP-version polyfills). Closes #251. Commits ------- a00cffe Add separate checks for all polyfilled functions and constants
In WordPress core, on the very early stage, the file wp-includes/compat.php is loaded. It defines polyfills for some mb_ functions, including mb_strlen. After this, Symfony\Polyfill\Mbstring becomes useless in any plugin / theme. This causes fatal errors on mb_ function calls.