[wp-trac] [WordPress Trac] #55635: wp_convert_hr_to_bytes() report correct byte sizes for php.ini "shorthand" values
WordPress Trac
noreply at wordpress.org
Thu Apr 28 02:01:53 UTC 2022
#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 dd32):
I absolutely hate the idea of writing PHP in the C style, but ultimately
love the reason for doing so.
However, I have some concerns, specifically that this is a bit over the
top for what it's trying to do.
My notes:
- `GB_IN_BYTES` can technically be changed, but if someone does that they
deserve it.
- The leading whitespace checks aren't working, as it's using single-
quoted strings, so looking for a literal '\t' 2-character string
- Isn't written in PHP coding style
- I don't think it needs to mirror PHP's parsing for a specific config
[attachment:"55635.3.diff"] is my take on the original issue, I haven't
run it against the unit tests, but I think it'll resolve the underlying
intention.
You can see the evolution in this gist:
https://gist.github.com/dd32/b6d757bf26f7e417621076fe06925ea5/revisions
where I took [attachment:"55635.diff"] and slowly migrated it back to PHP,
before coming to the conclusion that `inval( $s, 0 )` is almost the same,
albeit with additional binary notation support (which PHP ignores for ini
settings, but that's fine IMHO)
It's worth noting, that just because PHP parses it one way, it's not
necessarily strictly required that `wp_convert_hr_to_bytes()` should
handle those extreme edge-cases.. but there's some obvious improvements
that can be made here.
--
Ticket URL: <https://core.trac.wordpress.org/ticket/55635#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list