Fix file size calculation in modTransportPackage

[smg6511]

What does it do?

Strips size modifier (K,M,G) before calculation performed in the _bytes method.

Why is it needed?

A php warning occurs (and gets logged in MODX as an error) because the _bytes function attempts to use the ini memory_limit value in a calculation without removing the modifier string (e.g., 128M *= 1024). This triggers the “A non-numeric value encountered” warning.

How to test

1. Download any package before applying this PR fix. Note the warning in your MODX error log
2. Apply this PR and verify the warning no longer appears and that package downloads work as expected

Related issue(s)/PR(s)

Resolves #16504.

Note that the second warning re filesize could not be replicated (for my environ at least), although there needs to be a bit of redesign done in general with how files are read and stats are accessed; basically, everywhere that uses error suppression via the @ operator should be refactored to avoid these errors and warnings in general. This should be done in a different PR though.

[opengeek]

I believe we should look at the functionality of https://www.php.net/manual/en/function.ini-parse-quantity.php to implement this properly to handle all valid shorthand forms allowed which represent byte values.

[smg6511]

If you're meaning to use that method, we're not going to be able to do so until MODX's min required php is 8.2.

[opengeek]

If you're meaning to use that method, we're not going to be able to do so until MODX's min required php is 8.2.

I understand that. I meant we need to replicate the functionality of that method so that any valid value from php_ini is converted to a byte value correctly.

[smg6511]

I see, so do a polyfill of sorts—I figured you probably wouldn't blow past seeing the 8.2 requirement, but just in case ;-). But do you want to dig into that now or just do this simple little fix for the time being? If the former, then we might as well get the file stat gathering fixed up too.

[opengeek]

I'm not concerned about the file stat gathering here. My only intention is to fix the original issue without creating additional bugs, which this implementation currently does. There are several userland implementations of ini_parse_quantity out there. I'm merely suggesting that we use one of those as the implementation of _bytes() to fix the original issue and support all valid ini shorthand values that represent bytes.

[smg6511]

@opengeek - Ok, I've made a tweak to this minimal effort option. Let me preface what's coming with ... you know I'm all for the complete solution (maybe too much so at times), so I'm happy to work that up if you want to go that way.

The _bytes method, as it has stood, clearly has always assumed an incoming decimal value with (or without) a modifier, since the other valid number representations (hex, etc.) would need to be converted from their string rep (e.g., applying hexdec) to a decimal to do any calculations. That said, the simple option here at least fixes the handling of typical decimal[modifier] settings, getting rid of error/warning the referenced issue wants solved.

Back to the complete solution:

If you want to go "big" then we might want to consider creating a static conversion method in the modx class if not a polyfill for ini_parse_quantity; a couple considerations for the latter:

• Do we currently have any defined polyfills? If so, where; if not, where should they go?
• This could be nice so we can move from calling _bytes to ini_parse_quantity in code and eventually drop our _bytes method altogether. Note that the very same _bytes method is defined in modStaticResource. It's not otherwise utilized in that class, but may be in extending ones.

One thing to note, the first userland polyfill I came across on php.watch doesn't even go as far as to convert non-decimal strings to decimal. (Just pointing that one out because you'd think an example shown there would be of the most complete ones.) I confirmed though that php's implementation does.

Anywho, that was a book!

[opengeek]

I assume you mean integer above, rather than decimal? The userland implementation I was referring to is the one in Symfony's polyfill project. It is the only one that returns the same values you see in the samples from the PHP documentation for ini_parse_quantity.

See the examples at https://www.php.net/manual/en/function.ini-parse-quantity.php and the implementation at https://github.com/symfony/polyfill/blob/ecf66edfc66a2566b876ba6b909ada51182508c3/src/Php82/Php82.php#L82

We don't currently have any polyfills and I'm not keen on adding such a feature into 3.x, but I think we could (in this case) simply add some global functions to handle this issue.

BTW, this same _bytes() function implementation exists in modStaticResource. If you want, I can take a look at handling this issue.

[smg6511]

I assume you mean integer above, rather than decimal?

We're on the same page there. The nomenclature is tricky; php docs refer to the typical number representation as “decimal”—as all four reps of a number are technically an integer.

And, yeah, I saw (and mentioned above) that _bytes is defined in modStaticResource ;-)

I'll leave it to you, then. I do like the idea of just defining our own ini_parse_quantity if not defined rather than reinventing the wheel by some other method name. Totally your call though.