[wp-trac] [WordPress Trac] #55635: wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand" values

[WordPress Trac]

#55635: wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand"
values
-------------------------------------------------+-------------------------
 Reporter:  dmsnell                              |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  Upload                               |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  needs-unit-tests has-patch dev-      |     Focuses:
  feedback changes-requested                     |
-------------------------------------------------+-------------------------

Comment (by dmsnell):

 @costdev there are examples in that linked docs standard to single-line
 docblock comments, so maybe I will undo my change that turned them into
 multi-line statements. that is, unless some tool is simply unaware of the
 guideline and naively replaces all single-line comments it sees into `//`.
 can you confirm if it's happening automatically or will it be okay if I
 move back from multi-line to single-line?

 > without the guard in place, there's the potential that a number of the
 440 plugins out there may experience some breakages, depending on how
 they're using it. A cursory glance shows that in many cases, they seem to
 be correctly passing a string.

 That's a great tool! I manually reviewed all of the search results and
 confirmed that none are passing anything but the strings from `ini_get()`
 or other string values. Most of those search results are copying the Core
 code into non-admin contexts, which tells me that there's more good reason
 to do what @pento suggests and put this in another file, possibly `php-
 combat.php`.

 There were a few plugins that pass an optional defined string and so it's
 possible someone might `define(DEFAULT_SIZE, 15)` but there's no
 indication that this is happening. One plugin passes a default value of
 `100000kb` which unfortunately won't be what they think it is (the plugin
 is supposed to enable large uploads but it will limit uploads to 100K if
 it falls back to the default value).

 > Generally, it's discouraged to ignore rules, although I don't know of
 any strict rule against it. I don't personally find the multiline list
 distracting, but I appreciate that you do.

 I'll play around some with it. I'd ideally like to avoid solutions we know
 are adding unnecessary overhead just for the looks of the code (such as
 dynamically building the array or calling a function to get the value). I
 want this code to be lean since it's a fundamental-level operation, but at
 the same time we're not parsing these numbers frequently and hopefully
 this is never representative of the hot path, so some tradeoff in
 performance for readability is probably fine.

 > I think this change should go in a new function, rather than altering
 the behaviour of the existing function.

 originally I did this and having you say that makes me more confident to
 do that again. before building the patch for this ticket I figured it
 would be better to make a smaller change.

 > While the original intention of wp_convert_hr_to_bytes() was for quickly
 parsing PHP INI values, I suspect that it's often used for converting any
 "human readable" sizes to their integer equivalent (example).

 This is unfortunate and possibly we could let this function remain what it
 sounds like it does, but make it more robust over time. `10mb` represents
 10 MiB in Core, but `10kg` represents 10 GiB, and `10gm` represents 10 MiB
 again. It's fairly surprising (to me) how it parses numeric descriptions.

 If we have something like `wp_ini_byte_size()` (or [https://github.com/php
 /php-src/pull/8454 `ini_bytes()`]) we can move all calls to
 `wp_convert_hr_to_bytes()` to that instead.

 On that note would it be worth instead depreciating
 `wp_convert_hr_to_bytes()` in an attempt to remove any such confusion over
 its use? That confusion being mostly that it did do something but we would
 want people moving forward to not use this for parsing `ini_get()` values.
 "If you want ini byte sizes use `wp_ini_bytes()`; if you want counts from
 byte descriptions use `wp_hr_bytes()`" Or maybe `size_parse()` or
 `size_bytes()` to mirror the language of `size_format()`

 > One other note, the function should support -1 (as an integer, not a
 string), as this is commonly used for the
 WP_MEMORY_LIMIT/WP_MAX_MEMORY_LIMIT constants.

 This and others (handling integer inputs) might be good and easy with a
 new file. I'm going to try and create a PR on `wordpress-develop` with
 this in mind and pick `wp-includes/php-compat.php` as the file name unless
 anyone has a better suggestion.

 Thanks all; hopefully we can make this a bit smoother with auto test runs
 by moving to GitHub

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/55635#comment:9>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform