Fixed : Inconsistent behaviour when zero bytes are passed in size_format()

[narenin]

Trac ticket: https://core.trac.wordpress.org/ticket/61930

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props narenin, westonruter, martinkrcho, marian1, mukesh27, vrajadas.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[martinkrcho]

I think a better approach would be to convert the $bytes variable to float and keep the previous logic.

This would also prevent repetitive conversion to float in the foreach loop on line 491.

Would you please extend the test cases in Tests_Functions_SizeFormat::data_size_format() with this edge case? It would also be nice to cover other string values, e.g. '5', '374647' etc. These should be handled correctly as well.

[narenin]

@martinkrcho Thanks for the feedback if I will added $bytes = (float) $bytes; before line number 485 then the test case will failed in case of array( 0, 0, '0 B' ), because of strict typecasting.

It is working fine in case of e.g. '5', '374647' etc.

Please let me know if I am missing anything.

[martinkrcho]

You're right, this would break the existing handling for zero value.

Let's consider couple of edge cases to make sure no one else need to get back to this. What would you do for values such as: "0", "0.0", 0.0e3 and other strange representations of zero?

[narenin]

@martinkrcho Thanks for looking into this, I have made changes to cover up these cases, could you please give your valuable suggestions on this.

[martinkrcho]

I like your approach with the is_numeric check. I think this solution is much more robust. Thank you for sticking with this.

[IanDelMar]

Wouldn't it make sense to check for numeric input right at the start and return false for non-numeric values immediately? Currently, a non-numeric value makes it through the foreach loop and gets cast to 0 on each iteration, which will never meet the condition and ultimately results in returning false anyway -- except for non-numeric values that start with a numeric character.

Something like this might work:

function size_format( $bytes, $decimals = 0 ) {
	if (! is_numeric($bytes) && ! (is_string($bytes) && is_numeric($bytes[0]))) {
		return false;
	}

	$quant = array( /* array items */ );

	$bytes = (float) $bytes;

	if ( 0.0 === $bytes ) {
		/* translators: Unit symbol for byte. */
		return number_format_i18n( 0, $decimals ) . ' ' . _x( 'B', 'unit symbol' );
	}

	foreach ( $quant as $unit => $mag ) {
		if ( $bytes >= $mag ) {
			return number_format_i18n( $bytes / $mag, $decimals ) . ' ' . $unit;
		}
	}

	return false;
}

[mukeshpanchal27]

In https://core.trac.wordpress.org/ticket/36635 it return 0B instead of false so your suggestion re-open that issue.

[IanDelMar]

@mukeshpanchal27 All tests pass using the code from my previous comment, except for the empty string, which would have to be handled separately, e.g. by adding it to the if condition or by starting with if ('' === $bytes) { return false; }.

[IanDelMar]

However, I would argue that allowing '1 KB' to return '1 B' isn't the desired behavior, so a simple if (! is_numeric($bytes)) { return false; } would be sufficient.

[vraja-pro]

Refactoring @IanDelMar solution What if we only check if it's numeric and bail early if it isn't, and then cast it to float? That is sufficient to cover also '0' string and avoid duplication of casting. @mukeshpanchal27

function size_format( $bytes, $decimals = 0 ) {
	if ( ! is_numeric($bytes) ) {
		return false;
	}

	$quant = array( /* array items */ );

	$bytes = (float) $bytes;

	if ( 0.0 === $bytes ) {
		/* translators: Unit symbol for byte. */
		return number_format_i18n( 0, $decimals ) . ' ' . _x( 'B', 'unit symbol' );
	}

	foreach ( $quant as $unit => $mag ) {
		if ( $bytes >= $mag ) {
			return number_format_i18n( $bytes / $mag, $decimals ) . ' ' . $unit;
		}
	}

	return false;
}

[westonruter]

Why float?

Suggested change

	$bytes = is_numeric( $bytes ) ? (float) $bytes : $bytes;
	if ( 0.0 === $bytes ) {
	$bytes = (int) $bytes;
	if ( 0 === $bytes ) {

[westonruter]

Let's also make it clear what string is expected.

Suggested change

	* @return string|false Number string on success, false on failure.
	* @phpstan-param int|numeric-string $bytes

[IanDelMar]

Unless necessary, I would not narrow parameter types. PHPStan then assumes that an int|numeric-string is passed and will not report errors that may arise from passing a non-numeric string. If the type is narrowed, then why not use the public-facing @param and add a guard at the very top ensuring this type?

[westonruter]

By using int|numeric-string then it provides static analysis checks for authors if they pass something else. If they pass a plain string, then this is flagged as an error, which is useful. Otherwise, they only can learn about the mistake at runtime with such a guard.

[IanDelMar]

Narrowing the parameter type can both hide problems inside size_format() and introduce false positives at call sites:

Narrowing the parameter means that PHPStan's analysis of size_format() itself relies on the assumption that the value is always int|numeric-string. That assumption does not match the WP function reference and is unnecessary for the current implementation, as passing a non-numeric string does not affect unintended side effects.

At the moment, this does not appear to cause a problem. However, if code is later added based on the narrower assumption and behaves incorrectly for non-numeric strings, PHPStan will not catch it. As mentioned earlier, ensuring that the input is int|numeric-string at the top of the function would minimise this risk.

The narrowed type also shifts the burden to callers and to the accuracy of other functions' return types. For example, if a function actually returns int|numeric-string but is documented as int|string, passing its return value to size_format() will produce a false positive.