[wp-trac] [WordPress Trac] #55635: wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand" values
noreply at wordpress.org
Thu Apr 28 17:22:52 UTC 2022
#55635: wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand"
Reporter: dmsnell | Owner: (none)
Type: defect (bug) | Status: new
Priority: normal | Milestone: Awaiting
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-
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
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
More information about the wp-trac