Page counts and related calculations typically expect and document ints but are actually floats

[lev0]

Should be an int, not calculated using rounding, preventing off-by-one errors.

[lev0]

Hello?

[lev0]

How about this updated, less "magic" version?

wp-admin/includes/class-wp-list-table.php

@@ -284 +284 @@
                );
 
                if ( ! $args['total_pages'] && $args['per_page'] > 0 ) {
-                       $args['total_pages'] = ceil( $args['total_items'] / $args['per_page'] );
+                       $remainder = $args['total_items'] % $args['per_page'];
+                       if ( $remainder ) {
+                               $args['total_pages'] = 1 + ( $args['total_items'] - $remainder ) / $args['per_page'];
+                       } else {
+                               $args['total_pages'] = $args['total_items'] / $args['per_page'];
+                       }
                }
 
                // Redirect if page number is invalid and headers are not already sent.

[sabernhardt]

Thanks for the report!

The ​ceil() function *should* always round up any remainder.

1. Is there a way that the current ceil() function calculates an incorrect page count?
2. If the return value is always correct but needs conversion to the integer type, could adding the (int) type cast suffice?

   $args['total_pages'] = (int) ceil( $args['total_items'] / $args['per_page'] );
   

[lev0]

I understand ceil() function, and I also understand how floating point numbers work.

1. It's possible if someone uses an arbitrary number for $per_page with this class.
2. No, see ​the warning under the "From floating point numbers" heading about casting to integers. ceil() returns a float, which may be fine for display (because PHP assumes you want an 8 when you output a variable that contains 7.9999999999999991118), but not for numeric comparisons. Casting a number like 23.999999999999999999999… to an int gives 23, not 24, which is worse than just leaving it as the float.

The number will always be as close as possible to the integer when not exact, but it's still not exact. The ethos of WordPress includes extensibility, so why knowingly give data that can cause edge cases? Arguing for retaining an error-prone float calc is very reminiscent of the Bad Old Days™ of PHP.

[SergeyBiryukov]

Thanks for the patch!

Stumbled upon this in comment:19:ticket:38266. I agree it would be nice to clean it up :)

[jrf]

I've reviewed the patch and am not a fan.

Using ( (bool) $over ) and then trusting PHP to do the type juggling to integers is not a good idea.

1. It's "clever" code which is ​strongly discouraged. It requires more cognitive load than is necessary.
2. While it may work now, with PHP becoming stricter all the time, this may well stop working soon enough.

Something like ( $over > 0 ? 1 : 0 ) would already make the code a lot more self-explanatory and is not prone to the same issues.

Otherwise, an alternative solution would be:

<?php
$args['total_pages'] = (int) round( ceil( $args['total_items'] / $args['per_page'] ), 0); // Using round to prevent floating point issues.

To be fair, I've also ran tests with the old versus the new code with a large number of variations of "total items" and "per page", I could not get it to find a combination where we'd get a rounding error if just using (int) ceil(), so unless a specific case can be identified where we would get a rounding error, the simplest solution - i.e. (int) ceil() - would be the best one.

Note: if you want precision calculations with floats, the ​BCMath extension should always be preferred.

[lev0]

Replying to jrf:

I've reviewed the patch and am not a fan.

The corrected patch is in comment:3

Otherwise, an alternative solution would be:

<?php
$args['total_pages'] = (int) round( ceil( $args['total_items'] / $args['per_page'] ), 0); // Using round to prevent floating point issues.

Please no. This is exactly problem, both are functions that return floats where people expect ints, and are still vulnerable to integer truncation. Also, the round() does nothing except waste cycles because ceil() has already found the float value nearest to the expected int.

[lev0]

wp-list-table-integer-total-pages corrected

[jrf]

Please no. This is exactly problem, both are functions that return floats where people expect ints, and are still vulnerable to integer truncation. Also, the round() does nothing except waste cycles because ceil() has already found the float value nearest to the expected int.

The thing is: there is no problem other than the return type being unexpected.

The "problem" you suggest only exist for calculations with floats with decimals. As the base values being used don't have decimals, the problem you suggest this code has, doesn't actually exist.

Note: making the code more complex and adding more calculation steps (like in the original patch) does cause for the code to become subject to the problem.

And please, I'd love to be wrong, but as I said, show me one test case where with this use-case and code, the rounding problem would actually exist...

[jrf]

Oh and as I said before, if you want to actually eliminate the possibility that this problem would, for some future iteration of this code, crop up - your patches do not safeguard this.

In that case, the code would need to switch to using BCMath.

[lev0]

@jrf I don't understand the attitude over such a trivial fix, and that's thrice now you've not actually read this thread (first a missed patch, then missed the actual problem, then suggested a fix that was already explicitly reasoned against). I'm sure you're a busy person but not fully engaging is wasting your time, too.

The problem is that the type does not compare to integers as expected, which is how I discovered the bug. I wrote a class that extends WP_List_Table, as have many, many coders before me. In my class, I compared page totals with an integer setting, and got odd behaviour because they were floats. I worked around the issue by overriding the set_pagination_args() method and passing a correct value to the parent method.

As previously stated, the existing type is okay for display but not for extensibility.

Oh and as I said before, if you want to actually eliminate the possibility that this problem would, for some future iteration of this code, crop up - your patches do not safeguard this.

In that case, the code would need to switch to using BCMath.

How so? Do you mean if total_items and per_page are not integers? If so, when would they not be? Or do you mean if those values aren't <= PHP_INT_MAX? This patch is not intended to guard against bad input (and nor does the current version), but rather to produce better output. It also eliminates the overhead of a function call and introduces no new dependencies. Is WordPress planning on using BCMath here? If not, why suggest it?

[jrf]

that's thrice now you've not actually read this thread (first a missed patch, then missed the actual problem, then suggested a fix that was already explicitly reasoned against). I'm sure you're a busy person but not fully engaging is wasting your time, too.

@lev0 I do not appreciate your tone nor the content of your message. Please do not make incorrect and offensive assumptions about what I have read or not.

I FULLY understand the problem and the underlying intricacies of floating point calculations, but by your response you show that YOU don't.

Please study up on floating point calculations, BCMath and what the actual implications are in this case before making random and nonsensical claims and accusations.

[lev0]

Replying to jrf:

I FULLY understand the problem and the underlying intricacies of floating point calculations, but by your response you show that YOU don't.

Your suggested solution indicated the opposite:

Otherwise, an alternative solution would be:

<?php
$args['total_pages'] = (int) round( ceil( $args['total_items'] / $args['per_page'] ), 0); // Using round to prevent floating point issues.

You answered none of my questions about comment:10. Accept the patch or don't, I can work around the bug.

[johnbillion]

Can we get a failing test case that demonstrates the existing bug please? That would help clear things up.

[lev0]

I don't have access to the affected code any more. All I remember is that I used the total at the end of the table for something related to a setting read from the options table, and it worked better using ints. I was surprised that the type wasn't an int, so I wrote the patch.

From @SergeyBiryukov's comment:19:ticket:38266, it looks like WordPress could do with a simple function to keep the code DRY, and replace ceil() calls where one would expect an int returned, and avoid casting.

[audrasjb]

Since no decision was taken and it still needs unit tests, let's move it to next release.

[lev0]

Add and utilise a standard function for this common calculation.

[jrf]

@lev0 Thanks for the additional patch.

I like the idea of having a common function for this, though I'm not sure the function should be called wp_total_pages() as in not all cases it is about pages, but naming things is hard, so I also don't have a alternative name in mind.

Having said that, the new patch does not contain any unit tests covering this change for the existing functions, let alone contain any tests for the new function. Which also means that it also still doesn't contain a test which would proof the need for this fix.
In any case, the patch should probably be pulled to GitHub, so at least the results of a run of the existing unit tests would be available for this patch via GH Actions.

The function also, as I pointed out before, still doesn't solve your problem of it potentially returning a float.

Let me try and explain it one more time:

1. ​`ceil()` will return an integer as a float by design. As long as the inputs (and by extension the result) is below PHP_INT_MAX, it should be perfectly safe to cast this float to an integer without loss of precision.
2. If you still fear loss of precision, ​`round()` can be used around the call to ceil(). Again, this will return a float, but as long as the inputs (and by extension the result) is below PHP_INT_MAX, it should be perfectly safe to cast this float to an integer without loss of precision.
3. As per ​the manual, the division operator /:

   ... returns a float value unless the two operands are integers (or strings that get converted to integers) and the numbers are evenly divisible, in which case an integer value will be returned.
   

The same remark I made for the previous two about PHP_INT_MAX also applies.

4. Next, we have the PHP 7.0+ intdiv function. Aside from the fact that we can't use it yet unless we would polyfill it (PHP 5.6 minimum), this will return an integer, but will also:
   • throw a DisionByZeroError exception if the second operant is 0;
   • throw a AritmeticError exception is the first operant is PHP_INT_MIN and the second is -1;
   • throw a TypeError if either of the operants passed can't be cast to integers;
   • and will cast passed non-integer operants which can be cast to integers to integers before the operation.
5. Lastly, there is ​`bcdiv()` which expects two arbitrary precision numbers passed as strings as input and will return the result of the division as a string (or null if the second operant is 0). Again, as long as the integer value of the returned string is below PHP_INT_MAX, it should be perfectly safe to cast this string to an integer without loss of precision. The problem with using bcdiv() is that, even though PHP ships with BCMath by default, it is an extension which will only be available if PHP was build with --enable-bcmath, so it would add a new requirement to WP.

In the case of the last two (three) options, we still need to calculate the modulo as well and add either 0 or 1 to the results, which, in the case of bcdiv() will recast the result to integer/float.

You can have a look at ​these cheatsheets to see what the return value is when using /, intdiv()or bcdiv() with a wide variety of input values and types.

Or have a look at this 3v4l and study the results on various PHP versions carefully: ​https://3v4l.org/V6jdD

Hints:

• Without input validation (are both parameters integers ? is the second operant not a 0 ?), the logic as currently proposed in wp_total_pages() is liable to cause more problems than it solves.
• As soon as the calculations are done with one of the numbers being a number above PHP_INT_MAX, the results become unreliable, no matter what.
• (int) ceil() would be a perfectly valid solution (unless you can come up with a test case which proves differently).

[lev0]

• I'm not married to the name at all, I'm sure there's a better choice, perhaps something generic about splitting into groups.
• I totally get your point of about it not being guaranteed to return a float and I'm fully aware that's what the last patch does. I took your previous feedback about the (albeit uncommon) potential for the number to be greater than PHP_INT_MAX which is why I submitted code that deliberately avoids casting. If you feed it ints/int-like strings (as documented) it gives you an int unless an input is outside the range PHP can store. As it is being fed these input types in the core code, so should the return be as expected.
• I don't understand your point about wrapping ceil in round, my understanding is that ​this is a no-op because they both return floats.
• I know how / works, which is why I've suggested from the beginning that the operation should be gently coerced into producing an int by giving it ints and ensuring no remainder.
• As stated previously "This patch is not intended to guard against bad input (and nor does the current version)". WordPress already prevents most division by zero where the ceil( $x / $y )-style calls are currently. The same exceptions and errors can be thrown from the current code.
• I didn't feel this change would have any success if it introduced a dependency on bcmath; I don't see any use of it in core currently (correct me if I'm wrong). If WordPress deems introducing it to be a good idea, that's fine.
• You bring up many points about casting to an integer but also that this may not be desirable in case of overflow, which I agree with. Hence this patch is, as it was, intended to produce an int by default, not corrupt data. I think this causes the least disruption.

I put what I thought was a more concise function in the last patch but it could be (a little uglier) as before to reduce that slim chance of overflow:

wp-includes/functions.php

@@ -7866 +7866 @@
 function wp_fuzzy_number_match( $expected, $actual, $precision = 1 ) {
        return abs( (float) $expected - (float) $actual ) <= $precision;
 }
+
+/**
+ * Calculate the number of pages a set of results can be split into.
+ *
+ * This is similar to using `ceil()` on division but returns an integer.
+ *
+ * @since 5.8.0
+ *
+ * @param int $total_items The count of items to paginate.
+ * @param int $per_page    The maximum number of items per page.
+ * @return int The total number of pages.
+ */
+function wp_total_pages( $total_items, $per_page ) {
+       $remainder = $total_items % $per_page;
+       if ( $remainder ) {
+               return 1 + ( ( $total_items - $remainder ) / $per_page );
+       }
+       return $total_items / $per_page;
+}

I deliberately didn't submit this via GitHub as I figured I'd get feedback here about whatever else needed to be included, test-wise, etc. to make it worthwhile. I can submit it there instead if that is preferred.

[chaion07]

Thanks to @lev0 for reporting this. We recently reviewed this during a recent [bug-scrub session]​https://wordpress.slack.com/archives/C02RQBWTW/p1623097955369200. With Beta 1 coming up in a day we're punting it to Future Releases. Milestone has been updated. Thanks

refresh and added tests

Trac ticket: https://core.trac.wordpress.org/ticket/46346