Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#19067 closed enhancement (fixed)

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

Reported by: l3rady's profile l3rady Owned by: sergeybiryukov's profile 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 12 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 12 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 12 years ago.
Same as previous patch, but better/more intense phpDocBlock
19067_4.patch (1.8 KB) - added by F J Kaiser 12 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 12 years ago.
Moved to /wp-admin/includes/deprecated.php and changed phpDocBlock
19067.6.patch (2.5 KB) - added by SergeyBiryukov 11 years ago.
test_19067.patch (748 bytes) - added by soulseekah 11 years ago.
19067.function.patch (449 bytes) - added by soulseekah 11 years ago.

Download all attachments as: .zip

Change History (41)

#1 @F J Kaiser
12 years ago

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

@F J Kaiser
12 years ago

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

#2 @scribu
12 years ago

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

#3 @scribu
12 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 Kaiser
12 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()

#4 follow-ups: @l3rady
12 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()

#5 in reply to: ↑ 4 @F J Kaiser
12 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.

#6 in reply to: ↑ 4 ; follow-up: @scribu
12 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 arguments.

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

@F J Kaiser
12 years ago

Same as previous patch, but better/more intense phpDocBlock

#7 in reply to: ↑ 6 @F J Kaiser
12 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?

#8 follow-up: @scribu
12 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 Kaiser
12 years ago

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

#9 in reply to: ↑ 8 @F J Kaiser
12 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 12 years ago by F J Kaiser (previous) (diff)

#10 @scribu
12 years ago

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

#12 @ocean90
12 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 12 years ago by ocean90 (previous) (diff)

#13 follow-ups: @scribu
12 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 12 years ago by scribu (previous) (diff)

#14 in reply to: ↑ 13 @F J Kaiser
12 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)").

#15 @SergeyBiryukov
12 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 Kaiser
12 years ago

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

#16 @F J Kaiser
12 years ago

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

#17 @F J Kaiser
12 years ago

  • Owner F J Kaiser deleted

#18 @SergeyBiryukov
11 years ago

  • Keywords 3.6-early added

Related: #22295

#19 @F J Kaiser
11 years ago

#22295 was marked as a duplicate.

#20 @SergeyBiryukov
11 years ago

#22295 was marked as a duplicate.

#21 follow-up: @SergeyBiryukov
11 years 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 11 years ago by SergeyBiryukov (previous) (diff)

#22 @SergeyBiryukov
11 years ago

#12915 was marked as a duplicate.

#23 in reply to: ↑ 21 @SergeyBiryukov
11 years 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.

#24 @SergeyBiryukov
11 years ago

In 23437:

Add missing inline descriptions. see #19067.

#25 @SergeyBiryukov
11 years 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.

#26 in reply to: ↑ 13 @nacin
11 years 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.

#27 @SergeyBiryukov
11 years ago

In 1218/tests:

Test for wp_convert_bytes_to_hr(). see #19067.

#29 @SergeyBiryukov
11 years ago

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

In 23440:

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

#30 @soulseekah
11 years 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.

#31 @ocean90
11 years ago

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

#33 @DrewAPicture
11 years 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.