Duplicate functionality in core: size_format() and wp_convert_bytes_to_hr()

[l3rady]

I was looking through core to see if there was any function I could utilize to format byte int to HR. I found the following two functions:

size_format() - /wp-includes/functions.php
wp_convert_bytes_to_hr() - /wp-admin/includes/template.php

They both do the same kind of thing but slightly differently. Is there any need for wp_convert_bytes_to_hr()? from what I can see wp_convert_bytes_to_hr() is only used once in core and where it is used could be replaced with size_format().

[F J Kaiser]

Links to 3.2. core:

​size_format();

​wp_convert_bytes_to_hr();

[F J Kaiser]

Removed wp_convert_bytes_to_hr() and replaced it with size_format() inside

[scribu]

For backwards compatibility, we'll have to make wp_convert_bytes_to_hr() a wrapper to size_format() and deprecate it.

[scribu]

Also note that there's a wp_convert_hr_to_bytes() function, but it can't actually parse what wp_convert_bytes_to_hr() produces, so I'm fine with deprecating the later.

[F J Kaiser]

Made wp_convert_bytes_to_hr() deprecated (added phpDocBlock @deprecated and call to _deprecated_function for @version 3.3) and replaced wp_convert_bytes_to_hr() with size_format()

[l3rady]

possibility that we can add a third argument to size_format() and adjust function to be able to do the job of wp_convert_hr_to_bytes()? That way we can depreciate both wp_convert_bytes_to_hr() and wp_convert_hr_to_bytes()

[F J Kaiser]

Replying to l3rady:

possibility that we can add a third argument to size_format() and adjust function to be able to do the job of wp_convert_hr_to_bytes()? That way we can depreciate both wp_convert_bytes_to_hr() and wp_convert_hr_to_bytes()

Should be task of another ticket. If you create one, please link over here.

[scribu]

Replying to l3rady:

possibility that we can add a third argument to size_format() and adjust function to be able to do the job of wp_convert_hr_to_bytes()?

Negative. Single-purpose functions are better than big functions with lots of arguments.

[F J Kaiser]

Same as previous patch, but better/more intense phpDocBlock

[F J Kaiser]

Replying to scribu:

Negative. Single-purpose functions are better than big functions with lots of arguments.

Wouldn't say it's then a big function with lots of arguments. IMHO abs. valid request as a) only called twice and b) would maybe bring up enough use cases and c) cover the whole spectrum of this task(s).

Ad patch) Seen & agree?

[scribu]

A counter example:

Instead of get_option() and update_option(), why not have a single option() function, with an $action parameter?

Re 19067_3.patch:

• needs to be moved to wp-includes/deprecated.php
• _deprecated_function() should mention size_format() as the replacement

[F J Kaiser]

moved to wp-includes/deprecated.php & added size_format() as 3rd arg. for _deprecated_function() call - else same as previous patches

[F J Kaiser]

Replying to scribu:

A counter example:

Instead of get_option() and update_option(), why not have a single option() function, with an $action parameter?

Good example. But size_format doesn't have any sort of clear name that says "we convert small sizes to bigger ones for display". I'd expect that it works in both directions. Anyway: It shouldn't be discussed here as it's not part of this ticket.

Re 19067_3.patch:

• needs to be moved to wp-includes/deprecated.php
• _deprecated_function() should mention size_format() as the replacement

Done (and thanks for help & explanation).

[ocean90]

Related: #4240, #12915

[ocean90]

There could be a problem, because the output of both functions is different.

size_format adds a space between number and unit and see also this examples:

var_dump( size_format( 42353254 ) ); // string(5) "40 MB"
var_dump( wp_convert_bytes_to_hr( 42353254 ) ); // string(14) "40.391210556MB"

var_dump( size_format( 4.23 ) ); // string(4) "4 B "
var_dump( wp_convert_bytes_to_hr( 4.23 ) ); // string(5) "4.23B"

var_dump( size_format( 1073741823 ) ); // string(4) "1,024 MB "
var_dump( wp_convert_bytes_to_hr( 1073741823 ) ); // string(5) "1023.99999905MB"

[scribu]

The output from size_format() is preferred, since it's meant to be read by users.

If we want to be super-cautious, we could deprecate wp_convert_bytes_to_hr(), but keep it's current code, instead of making it a wrapper for size_format().

[F J Kaiser]

Replying to scribu:

The output from size_format() is preferred, since it's meant to be read by users.

Agree, as more readable (I guess the _hr doesn't stand for human readable). If we really need to keep the output of wp_convert_bytes_to_hr();, it's not too hard to do some str_replace() logic wrapped around the outcoming of size_format();. Speaking for myself: I'm satisfied how the patch works (but we could move to another ticket and replace "B" with "Byte(s)").

[SergeyBiryukov]

@deprecated in favor of size_format()

@deprecated tag usually shows the WP version which the function was deprecated in. For most functions in deprecated.php, there are two tags, for example:

@deprecated 3.3
@deprecated Use size_format()

[F J Kaiser]

Moved to /wp-admin/includes/deprecated.php and changed phpDocBlock

[SergeyBiryukov]

Related: #22295

[F J Kaiser]

#22295 was marked as a duplicate.

[SergeyBiryukov]

#22295 was marked as a duplicate.

[SergeyBiryukov]

In [22073], wp_convert_bytes_to_hr() was moved to wp-includes/media.php along with wp_convert_hr_to_bytes(). Unlike the latter, which is used in wp_max_upload_size(), the former is only used in wp_import_upload_form(), so it can still go to wp-admin/includes/deprecated.php rather than wp-includes/deprecated.php.

Refreshed after [22073], fixed a typo in line 867 of the previous patch.

[SergeyBiryukov]

#12915 was marked as a duplicate.

[SergeyBiryukov]

Replying to SergeyBiryukov:

so it can still go to wp-admin/includes/deprecated.php rather than wp-includes/deprecated.php.

On second thought, wp-includes/deprecated.php might be safer.

[SergeyBiryukov]

In 23437:

Add missing inline descriptions. see #19067.

[SergeyBiryukov]

In 23439:

Deprecate wp_convert_bytes_to_hr() in favor of size_format(). props F J Kaiser. fixes #19067.

[nacin]

Replying to scribu:

The output from size_format() is preferred, since it's meant to be read by users.

If we want to be super-cautious, we could deprecate wp_convert_bytes_to_hr(), but keep it's current code, instead of making it a wrapper for size_format().

Because of the output change, I think this is a good idea. A unit test asserting the outputs of these functions would additionally be a good idea.

I like the simpleness of size_format(), but it is kind of lame to deprecate wp_convert_bytes_to_hr() when we have a sanely named inverse: wp_convert_hr_to_bytes(). That's part of the reason why they were both moved to wp-includes/media.php, because it would be weird for one to be accessible in one context, but not the other. Deprecating only one is a bit goofy too. I do understand the reason, though — size_format() is a better output.

[SergeyBiryukov]

In 1218/tests:

Test for wp_convert_bytes_to_hr(). see #19067.

[SergeyBiryukov]

We already have a test for size_format():
​http://core.trac.wordpress.org/browser/tests/trunk/tests/functions.php?rev=1192#L41

[SergeyBiryukov]

In 23440:

Restore the original wp_convert_bytes_to_hr() code. fixes #19067.

[soulseekah]

Two issues: on 64-bit PHP (PHP_INT_SIZE == 8) floating point does not match expected output (gets 1022.9990234375MB). The last digits in floating point should not be depended upon due to these inaccuracies. Rounding in PHP < 5.3 is not an option either, since rounding mode was introduced later and different systems get different results (round up vs. round down by default). So perhaps looking at error and discrepancy would be a decent solution (maybe even less error 0.00001?).

And wp_convert_bytes_to_hr's (int)double(NAN) result from feeding -1 and 0 results in power being a huge negative number instead of 0. This results in both a Warning being thrown, and the "B" missing in tests, i.e. expected "NANB" != "NAN" and expected "0B" != "0".

Tests pass with patches applied.

[ocean90]

soulseekah, can you please open a new ticket for this?

[soulseekah]

#23626

[DrewAPicture]

[23439] deprecated wp_convert_bytes_to_hr() in favor of size_format()