WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 14 months ago

Last modified 11 months ago

#19067 closed enhancement (fixed)

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

Reported by: l3rady Owned by: SergeyBiryukov
Milestone: 3.6 Priority: normal
Severity: trivial Version: 3.2.1
Component: General Keywords: has-patch 3.6-early commit needs-codex
Focuses: Cc:

Description

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().

Attachments (8)

19067.patch (1.1 KB) - added by F J Kaiser 2 years ago.
Removed wp_convert_bytes_to_hr() and replaced it with size_format() inside
19067_2.patch (1.1 KB) - added by F J Kaiser 2 years ago.
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()
19067_3.patch (1.3 KB) - added by F J Kaiser 2 years ago.
Same as previous patch, but better/more intense phpDocBlock
19067_4.patch (1.8 KB) - added by F J Kaiser 2 years ago.
moved to wp-includes/deprecated.php & added size_format() as 3rd arg. for _deprecated_function() call - else same as previous patches
19067_5.patch (1.8 KB) - added by F J Kaiser 2 years ago.
Moved to /wp-admin/includes/deprecated.php and changed phpDocBlock
19067.6.patch (2.5 KB) - added by SergeyBiryukov 16 months ago.
test_19067.patch (748 bytes) - added by soulseekah 14 months ago.
19067.function.patch (449 bytes) - added by soulseekah 14 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 F J Kaiser2 years ago

  • Cc 24-7@… added
  • Keywords has-patch added; needs-patch removed

F J Kaiser2 years ago

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

comment:2 scribu2 years ago

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

comment:3 scribu2 years ago

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 Kaiser2 years ago

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()

comment:4 follow-ups: l3rady2 years ago

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()

comment:5 in reply to: ↑ 4 F J Kaiser2 years ago

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.

comment:6 in reply to: ↑ 4 ; follow-up: scribu2 years ago

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 unrelated arguments.

Version 0, edited 2 years ago by scribu (next)

F J Kaiser2 years ago

Same as previous patch, but better/more intense phpDocBlock

comment:7 in reply to: ↑ 6 F J Kaiser2 years ago

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?

comment:8 follow-up: scribu2 years ago

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 Kaiser2 years ago

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

comment:9 in reply to: ↑ 8 F J Kaiser2 years ago

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).

Last edited 2 years ago by F J Kaiser (previous) (diff)

comment:10 scribu2 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to Future Release

comment:12 ocean902 years ago

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"
Last edited 2 years ago by ocean90 (previous) (diff)

comment:13 follow-ups: scribu2 years ago

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().

Last edited 2 years ago by scribu (previous) (diff)

comment:14 in reply to: ↑ 13 F J Kaiser2 years ago

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)").

comment:15 SergeyBiryukov2 years ago

@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 Kaiser2 years ago

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

comment:16 F J Kaiser2 years ago

  • Owner set to F J Kaiser
  • Status changed from new to assigned

comment:17 F J Kaiser2 years ago

  • Owner F J Kaiser deleted

comment:18 SergeyBiryukov18 months ago

  • Keywords 3.6-early added

Related: #22295

comment:19 F J Kaiser16 months ago

#22295 was marked as a duplicate.

comment:20 SergeyBiryukov16 months ago

#22295 was marked as a duplicate.

SergeyBiryukov16 months ago

comment:21 follow-up: SergeyBiryukov16 months ago

  • Milestone changed from Future Release to 3.6

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.

Last edited 16 months ago by SergeyBiryukov (previous) (diff)

comment:22 SergeyBiryukov16 months ago

#12915 was marked as a duplicate.

comment:23 in reply to: ↑ 21 SergeyBiryukov14 months ago

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.

comment:24 SergeyBiryukov14 months ago

In 23437:

Add missing inline descriptions. see #19067.

comment:25 SergeyBiryukov14 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from assigned to closed

In 23439:

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

comment:26 in reply to: ↑ 13 nacin14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:27 SergeyBiryukov14 months ago

In 1218/tests:

Test for wp_convert_bytes_to_hr(). see #19067.

comment:29 SergeyBiryukov14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 23440:

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

soulseekah14 months ago

comment:30 soulseekah14 months ago

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.

comment:31 ocean9014 months ago

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

comment:33 DrewAPicture11 months ago

  • Keywords needs-codex added

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

Note: See TracTickets for help on using tickets.