[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 costdev):

 Hi @dmsnell, I did remove the PHPDoc data from the single-line comments
 based on the [https://developer.wordpress.org/coding-standards/inline-
 documentation-standards/php/#5-inline-comments Docs Standards]. However,
 I'm not opposed to their existence in the code since `/** @var` exists in
 722 locations elsewhere in Core.

 > Is this an error from real code or possibly just from a test?

 This seems to only be in the test suite, appearing almost immediately, so
 it might exist in the test suite's bootstrap.

 > We can always add extra runtime type-checking and casting, though it
 seems like the intention here is to only be called with data from
 ini_get() which only returns string values (or false, which I suppose
 would be good to guard against). I checked and I don't believe any code in
 Core should be passing an integer so **maybe there's buggy test code out
 there doing that thinking that this function wants an integer?**

 It's possible, and the function ''is'' documented to receive `(string)
 $value`. However, without the guard in place, there's the potential that a
 number of the [https://wpdirectory.net/search/01G1Q34ZM2HRM2C8FYB9JK10ZB
 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.

 > Concerning the list of $digits do we have a policy for excluding the
 formatting rule? I personally found it more distracting as a large multi-
 line list and so am hoping we can exclude that array for the sake of not
 distracting what's going on in the rest of the function.

 There are `phpcs:ignore` comments in Core, but none for the rule in
 question:

 `WordPress.Arrays.ArrayDeclarationSpacing.ArrayItemNoNewLine`

 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.

 Given that these two shorter alternatives aren't as clear, or as
 performant, as the original patch or the multiline list, I think it's
 either a case of a multiline list or a `phpcs:ignore` comment. I defer to
 consensus on that should the array be in the final patch.

 {{{#!php
 <?php
 $digits = range( 0, 9 );
 $digits = array_merge(
         $digits,
         array(
                 'A' => 10,
                 'a' => 10,
                 'B' => 11,
                 'b' => 11,
                 'C' => 12,
                 'c' => 12,
                 'D' => 13,
                 'd' => 13,
                 'E' => 14,
                 'e' => 14,
                 'F' => 15,
                 'f' => 15,
         )
 );

 // OR

 $digits = array_merge(
     range( 0, 9 ),
     array_combine(
         array( 'A', 'a', 'B', 'b', 'C', 'c', 'D', 'd', 'E', 'e', 'F', 'f'
 ),
         array( 10, 10, 11, 11, 12, 12, 13, 13, 14, 14, 15, 15 )
     )
 );
 }}}

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