Fix email report decimal comma locale. by mehidi258 · Pull Request #13265 · google/site-kit-wp

mehidi258 · 2026-08-05T09:40:30Z

Summary

Related issue(s):

Resolves Email report comparison badges always show 0% for locales that use a decimal comma #12867

Relevant technical choices

normalize_trends() now formats trends with number_format() (period decimal) instead of number_format_i18n(), since the locale-formatted string (e.g. 6,52%) couldn't be re-parsed by is_numeric() and silently became 0%.
parse_change_value() detects the decimal separator structurally instead of assuming one, so it parses both the new canonical format and any legacy locale-formatted values already stored.
section-metrics.php / section-page-metrics.php skip the change badge entirely when the value is null, instead of defaulting to 0.
Out of scope: build_sections() switches the locale but never restores it on success, so it can leak into later renders — tracked in Email report section builder leaks switched locale on success path #12883.
Out of scope: the badge can show -0% for a trend that rounds to negative zero — tracked in Email report change badge shows -0% for near-zero negative trends #13264.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2026-08-05T14:38:43Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

📦 Build files for 747d030:

Download development build
Download production build

🎭 Playwright reports for 747d030:

tofumatt

I'd expect some test cases with large numbers here to ensure we're handling both decimal places and thousands separators properly, and I've left some notes about comments and approach here.

tofumatt · 2026-08-06T14:03:33Z

+							$change = $data['change'] ?? null;
+						if ( null !== $change ) :


What's with the indentation here? It seems a bit off 🤔

Also, will $data[ 'change' ] ever be null of its own accord? If not:

Suggested change

$change = $data['change'] ?? null;

if ( null !== $change ) :

if ( undefined !== $data['change'] ) :

seems like it would work 🤔

tofumatt · 2026-08-06T14:07:17Z

+										$change = $data['changes'][ $index ] ?? null;
+									if ( null !== $change ) :


Similarly, is this casting to null needed? If so: can you add a comment explaining why?

tofumatt · 2026-08-06T14:08:27Z

+ * @var float $value The percentage change value. Callers must guard against
+ *                    `null` and skip rendering this part entirely when no
+ *                    comparison is available, rather than passing a
+ *                    default of `0`, which would render a misleading `0%`
+ *                    badge.


Suggested change

* @var float $value The percentage change value. Callers must guard against

* `null` and skip rendering this part entirely when no

* comparison is available, rather than passing a

* default of `0`, which would render a misleading `0%`

* badge.

* @var float $value The percentage change value. Callers must guard against

* `null` and skip rendering this part entirely when no

* comparison is available, rather than passing a

* default of `0`, which would render a misleading `0%`

* badge.

Why not have this component accept undefined/null values and choose to output nothing? 🤔

tofumatt · 2026-08-06T14:11:55Z

+			$formatted = number_format( $number, 2, '.', '' );

 			$output[] = sprintf( '%s%%', $formatted );


Is there any reason to format this as a string at all here? Why not just return the float and do the locale-formatting later on, if this one isn't locale-aware anyway? 🤔

tofumatt · 2026-08-06T14:12:25Z

+	 *
+	 * The trend is kept in a canonical, period-decimal form rather than a
+	 * locale-formatted one, since downstream consumers (the template
+	 * formatter and the change-badge template) always re-parse and
+	 * re-format the value themselves. A locale-formatted string (e.g.
+	 * `6,52%` under a decimal-comma locale) only made the value harder to
+	 * re-parse.


This looks like an AI-generated comment and is a bit too verbose. It's documenting, very specifically, the behaviour of this function in the context of this issue's fix, but it doesn't serve the reader of this documentation in-general.

Suggested change

*

* The trend is kept in a canonical, period-decimal form rather than a

* locale-formatted one, since downstream consumers (the template

* formatter and the change-badge template) always re-parse and

* re-format the value themselves. A locale-formatted string (e.g.

* `6,52%` under a decimal-comma locale) only made the value harder to

* re-parse.

tofumatt · 2026-08-06T14:20:20Z

+			// `es_CO` is not an installed test locale, but `normalize_trends()`
+			// must not localize the trend regardless of what the active
+			// locale's number format looks like (simulated above), so the
+			// locale name here only documents intent.


Suggested change

// `es_CO` is not an installed test locale, but `normalize_trends()`

// must not localize the trend regardless of what the active

// locale's number format looks like (simulated above), so the

// locale name here only documents intent.

// Simulate the `es_CO` locale, which uses commas

// for decimals (eg. `6,52`, not `6.52`).

tofumatt · 2026-08-06T14:21:04Z

+	public function data_parse_change_value() {
+		$cases  = array();
+		$values = array(
+			'6.52%'  => 6.52,
+			'-0.85%' => -0.85,
+			'6,52%'  => 6.52,
+			'-0,85%' => -0.85,
+			'0%'     => 0.0,
+		);


As mentioned earlier: some larger values with both commas and decimals should be used here to ensure they behave properly.

tofumatt · 2026-08-06T14:21:38Z

+	 * rather than relying on `switch_to_locale()`, which silently no-ops for
+	 * locales without an installed translation file.


Suggested change

* rather than relying on `switch_to_locale()`, which silently no-ops for

* locales without an installed translation file.

* rather than relying on `switch_to_locale()`, which proceeds without warnings

* for locales without an installed translation file.

tofumatt · 2026-08-06T14:22:22Z

+	 * rather than relying on `switch_to_locale()`, which silently no-ops for
+	 * locales without an installed translation file.
+	 *
+	 * @param string   $locale   Locale name (en_US, es_CO, or de_DE).


Suggested change

* @param string $locale Locale name (en_US, es_CO, or de_DE).

* @param string $locale Locale name (eg. `'en_US'`, etc.).

tofumatt · 2026-08-06T14:51:01Z

+		// The email's static <style> block always defines `.badge-positive`/
+		// `.badge-negative` CSS rules, so assert on the rendered `class`
+		// attribute rather than the bare class name.


Suggested change

// The email's static <style> block always defines `.badge-positive`/

// `.badge-negative` CSS rules, so assert on the rendered `class`

// attribute rather than the bare class name.

// The email's static <style> block always defines `.badge-positive`/

// `.badge-negative` CSS rules, so include the `class=""` wrapper to

// ensure we're testing for the existence of the badge markup and not

// just the CSS in the `<style>` tag.

mehidi258 added 4 commits August 5, 2026 03:20

Fix email report decimal comma locale.

2c6bd50

Fix change badge showing -0% and add missing @SInCE tags.

237d43c

Revert unwanted changes.

bb3f3aa

Resolve conflict.

747d030

tofumatt requested changes Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix email report decimal comma locale. - #13265

Fix email report decimal comma locale.#13265
mehidi258 wants to merge 4 commits into
developfrom
bug/12867-fix-email-report-decimal-comma-locale

mehidi258 commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

tofumatt left a comment

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

tofumatt Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$change = $data['change'] ?? null;
	if ( null !== $change ) :
	if ( undefined !== $data['change'] ) :

		$change = $data['changes'][ $index ] ?? null;
		if ( null !== $change ) :

		$formatted = number_format( $number, 2, '.', '' );

		$output[] = sprintf( '%s%%', $formatted );

		* rather than relying on `switch_to_locale()`, which silently no-ops for
		* locales without an installed translation file.

	* @param string $locale Locale name (en_US, es_CO, or de_DE).
	* @param string $locale Locale name (eg. `'en_US'`, etc.).

Uh oh!

Conversation

mehidi258 commented Aug 5, 2026

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 5, 2026 •

edited

Loading