BC Math isn't used in WordPress core

[bronsonquick]

I had one of my old colleagues freaking out about the The optional module, bcmath, is not installed, or has been disabled. which is under 4 Recommended improvements in Site Health.

@rmccue and I identified that BC Math isn't used in WordPress core. I assumed it was likely to be used in Jetpack but that wasn't the case. @webaware identified that it is optionally used in Yoast SEO so I guess that's why this is a recommend improvement but I believe this shouldn't be a recommendation and plugins such as Yoast Seo should hook into Site Health to make that recommendation if the plugin is installed and enabled.

It's been ages since I've used trac but I'll try and remember how to contribute a patch for this shortly! There' doesn't appear to be a 'Site Health' component so I'll guess I'll post this to General and let one of the amazing Trac Gardeners pop it in the right place!

[bronsonquick]

Apologies if these images don't work because it's been years since I've used Trac!

[Clorith]

Hiya,

You're right, core doesn't directly use it, but all the recommendations we put in are based on what the ​Hosting Team has put up as ​recommendations and requirements (I would presume this is because plugins or themes use it actively).

I would propose that you join in on their discussions on this, as the recommendations and requirements in the Site Health module should always reflect what they propose as good standards.

[bronsonquick]

Hey @Clorith,

I "kind of" understand the logic behind that but that being said I'm pretty sysops and devops heavy I'm wondering if I should patch WordPress core to recommend opcache, memcache and memcached too in the Site Health checks? That will add a hell of a lot more performance to WordPress sites than BC Math. Core doesn't need bcmath and it doesn't need any of the packages I mentioned as well.

Sorry if I sound horrible i just don't understand the logic behind this.

[SergeyBiryukov]

Replying to bronsonquick:

I'm wondering if I should patch WordPress core to recommend opcache, memcache and memcached too in the Site Health checks?

As noted above, that would be a good discussion to have with the Hosting team (​#hosting-community on WordPress Slack).

[johnbillion]

Let's bring this back on topic please. BC Math isn't used in core, therefore it's a bug to unnecessarily list it as a recommended extension.

The list of recommended and required extensions on the Hosting page is simply a copy of a list originally published by Pantheon and hasn't been vetted or checked for accuracy. It's certainly not a conclusive list that core should defer to.

Further reading: ​https://github.com/johnbillion/ext/issues/1

Related: #47272

[Clorith]

I still think we should follow the Hosting Teams recommendations, and I'm sure there's a reason for them putting in the recommendations they have, they would likely know this better than us.

I've put in a request for an audit of their recommendations page for completeness sake, as things may have changed, but just because core doesn't use it doesn't mean it's necessarily a bad recommendation. What core needs to work are all marked as requirements, the others are recommendations for performance that won't have an adverse effect. (I'll of course still leave this milestoned until we can get clarification on the purpose of the teams page)

[galbaras]

Since Site Health is a new thing, why not make it cover only Core, while providing ways for plugins and themes to add requirements.

Also, requirements should have different levels of importance, so that Site Health will report them differently if they're just a bit better vs. if they're critical.

As for the official PHP module list, I recently reviewed it and got feedback from my hosting provider and it really needs to be reviewed. It may be a good idea to separate the recommendations into Core vs. "popular plugins" and absolute requirements (i.e. no fallback) vs. good to have (i.e. just preferred) to make things clear.

Standardisation is a great thing. It can save lots of people lots of time and problems. Site Health is a good vehicle to drive it, so let's ride that wave.

[kirasong]

The list is not a copy, but from an audit the hosting team did as a group some time ago.

It's an original work from the team, which included one member from Pantheon. I'd appreciate not putting it down, though, regardless of who put it together.

In any case, I'm definitely open to going over it again.

Replying to johnbillion:

Let's bring this back on topic please. BC Math isn't used in core, therefore it's a bug to unnecessarily list it as a recommended extension.

The list of recommended and required extensions on the Hosting page is simply a copy of a list originally published by Pantheon and hasn't been vetted or checked for accuracy. It's certainly not a conclusive list that core should defer to.

Further reading: ​https://github.com/johnbillion/ext/issues/1

Related: #47272

[jorbin]

@mikeschroder Do you think it would be worth a post on ​https://make.wordpress.org/hosting/ (cross-posted to make/core) to discuss if the list needs to be updated? It looks like it was created 10 months ago by the team, so it may have been enough time that it is worth re-opening discussion.

In general, I think recommending modules that will allow sites to have the best possible performance makes the most sense. As this isn't a screen that people will likely visit on a very regular basis, then we need to take into account that a site is often WordPress core along with a collection of plugins and a theme, and only recommending things that core needs is doing a disservice to users being able to have the healthiest site in the long run.

[bronsonquick]

Thanks all! Sorry for the negativity in the comments

I double checked ​https://make.wordpress.org/hosting/handbook/handbook/server-environment/#php-extensions tonight and bcmath isn't listed there anymore so I'm guessing that was updated which is good.

@jorbin @mikeschroder I'd be keen to float the idea of adding some performance recommendation extensions and linking to some appropriate documentation about that kinda stuff!

I know my take on hosting is pretty different to y'all as your stuff is managed mostly globally from a hosting level. I've been working on ​https://github.com/Chassis/Chassis for years and years and it's a mostly lean dev environment so I often find out pretty quickly from developers when things look wrong. The bcmath one was an issue which is why I decided to pop in a patch for this.

[kirasong]

Thanks much @bronsonquick! Some context on the change below -- sorry, I should have followed up in here right away after the changes.

@jorbin Yes, I agree it'd be a good idea to have a post. Both because I think it'd be good to discuss what should be included on the list, but also so that it can be discussed where the source of truth is for all of this. I hadn't considered that we might want to recommend things in Site Health that are outside of what core requires, and I like that idea. Will think on it, and also that's an excellent discussion point.

@Clorith ​brought this ticket up for discussion and addition to agenda in #hosting-community, and after I replied to this ticket, we chatted about it a bit there.

@dd32 did a first pass on updating things, which is where the changes came from (thank you!). It was never intended to be a stale document that is only updated from time to time, and I'm glad for more discussion and iteration!

[galbaras]

Checking the modules required for common configurations may turn out to be time consuming and difficult, and the approach I've seen in other cases (like not adding an index on wp_options.autoload) has been to cater for core only and provide ways to extend.

[jrf]

Ok, so someone kindly pointed me to this discussion. (Thanks @jipmoors)

First off: let me make sure everyone knows what ​BCMath does: it provides calculation functions and it is the only reliable way to do calculations with floating point numbers in PHP which is available by default.

If you want more details - read the big reddish warning on the PHP manual page about floats: ​https://www.php.net/manual/en/language.types.float.php

Oh and just for fun and to understand the impact, try this code snippet:

<?php
$x = (int) ( ( 0.1 + 0.7 ) * 10 );
var_dump($x);

This means that any calculations involving divisions, percentages and money should use BCMath as otherwise they will be buggy and unreliable.

I dearly hope that plugins like WooCommerce do this correctly....

BCMath has shipped with PHP since PHP 4.0.4.

The GMP can be seen as a valid alternative, but using that with floating point numbers is more involved and it doesn't ship with PHP by default.

BC Math isn't used in core, therefore it's a bug to unnecessarily list it as a recommended extension.

If Core doesn't use it, it shouldn't be a required extension, but it surely should be a recommended extension.

Also: I wonder if there aren't places in Core where calculations are done with floats which should use BCMath.

bcmath isn't listed there anymore so I'm guessing that was updated which is good.

I'd recommend bringing it back.

Since Site Health is a new thing, why not make it cover only Core, while providing ways for plugins and themes to add requirements.

I'd fully support adding a way for plugins to hook into this.

[Clorith]

It's already marked as a recommendation, not a requirement.

There's also already a filter in place to modify the extensions the Health Check module looks for, site_status_test_php_modules, along with their severity levels, potential fallbacks and so forth.

I don't fully agree with the concept of only adding what core uses as checks. What core needs should be (and are) requirements, but some improvements from extensions are valid recommendations, as @jrf mentions in this case it's pretty much a necessity for anyone doing anything with numbers.

Plugins could add it to the list them selves, and for obscure requirements, I agree that they should, but for something as commonplace as this, we should be providing the recommendations on behalf of what's best for the users.

[johnbillion]

It's worth pointing out that bcmath is of value only if the userland code uses the bc*() functions that the extension provides. It doesn't automatically increase the precision of native floating point calculations.

It's not beneficial to the end user to recommend an extension such as this unless core is going use its functions. Literally any extension could be recommended using the logic that the extension is useful if userland code uses its functions.

[galbaras]

Replying to Clorith:

... but for something as commonplace as this, we should be providing the recommendations on behalf of what's best for the users.

The difficulty, again, is in determining just how "commonplace" something is.

In this particular case, the example given by @jrf is especially crafted to demonstrate a potential problem of PHP doing weird things with deliberate type casting (without it, the answer is correct and can be further adjusted with round() or floor()). Hopefully, the WordPress Core (and plugin/theme) developers do the latter.

[TimothyBlynJacobs]

It looks like a number of plugins make use of it:

​https://wpdirectory.net/search/01DCMD1BSSEK7GSK9DYNBCGEQP

[jrf]

In this particular case, the example given by @jrf is especially crafted to demonstrate a potential problem of PHP doing weird things with deliberate type casting (without it, the answer is correct and can be further adjusted with round() or floor()).

Not really. If you use floor(), the same would happen and congratulations! You've just given someone a $1 discount.
It's actually not a PHP thing, but to do with how C stores floats in its internal memory using binary.

It looks like a number of plugins make use of it:

@TimothyBlynJacobs I'm absolutely appalled that WooCommerce only uses the BCMath extension once... this is truly scary and this will lead to incorrect calculations.
Even if it's just one cent discount, for big stores this can add up to big $$$.

[galbaras]

Assuming WooCommerce doesn't cast to "int" or use floor(), but instead uses round($amount,2), there should be no problem.

Test:

$x = (int)( 17.535845 );
var_dump( $x );
$x = floor( 17.535845 );
var_dump( $x );
$x = round( 17.535845, 2 );
var_dump( $x );
$x = 7.95 * 0.75; // Simulate a 25% discount on an amount of money
var_dump( $x );

Results:

int(17)
float(17)
float(17.54)
float(5.9625)

All good when you avoid casting and flooring.

[bronsonquick]

I still think we should only recommend WordPress core modules.

As @Clorith mentioned there is the site_status_test_php_modules filter that plugin authors can use to add their recommendations. In 5 mins I whipped up an example plugin on how they can use it: ​https://github.com/BronsonQuick/site-health-recommendations

I think if we leave it to up to core to grep the plugins for all the common modules then it'll get a little out of hand for people to grep and update the list instead of using the filter.

For example, a lot (nay, alot haha) of the plugins will use Memcached for example:

​https://wpdirectory.net/search/01DCNNASGP55WJEMPGG0KEP17K

Should that be added in too?

[knutsp]

I suggest that "bcmath" stays as a core test, to standardize it, but disabled by a filter function returning false. Then any plugin can just remove the filter. "At least one plugin recommends ..."

[galbaras]

@bronsonquick From what I see in the list of plugins, they might be using memcached if it's there, with fallbacks. In fact, when looking at some of the search results in detail, the include readme files, .pot files and other weird things that aren't code.

[bronsonquick]

I suggest that "bcmath" stays as a core test

I don't really understand the logic here. If it's a core test then shouldn't one of the bc*() functions actually be used in WordPress core?

[desrosj]

Moving Site Health tickets into their lovely new home, the Site Health component.

[Clorith]

47454.patch​ updates the tests to reflect the current state of the Hosting Teams handbook recommendations.

This includes removing the bcmath function, and introduce some new ones used by core in various locations for certain scenarios.

It also extends the test feature to be more flexible, now also allowing for inclusion of constant and class names to be checked for. This also provides a more robust test platform, as certain attributes may exist, while others are not available, most notably the libsodium check which now looks for the SODIUM_LIBRARY_VERSION constant, instead of the sodium_compare function as the function check was unreliable for that.

[jrf]

@Clorith With an eye on #47699, shouldn't json be required => true ?

Also: the function check should be on the json_last_error function as WP polyfilled all the other functions, including json_encode(), which means the function_exists() won't work as intended when used with json_encode().

[Clorith]

Great points @jrf, I wasn't aware we were moving forward with the JSON removal yet.

I've taken those into account in 47454.2.patch​

[jrf]

Great points @jrf, I wasn't aware we were moving forward with the JSON removal yet.

Well, it may still be reverted for WP 5.3 until a way to block updates for WP majors based on the extensions has been put in place, but having json set as a required extension will allow us to move forward with it in the (near) future.

I've taken those into account in 47454.2.patch​

Thanks!

[jrf]

Related ticket: #48086 which intends to add a list of required/recommended PHP extensions to the composer.json file.

[SergeyBiryukov]

In 46268:

Site Health: Update PHP extension tests to reflect the current state of the Hosting Teams handbook recommendations.

Props Clorith, bronsonquick, jrf, johnbillion, galbaras, mikeschroder, jorbin.
Fixes #47454.