Replace value casting with ! empty() check for positive value validation

[dilipbheda]

The current logic for reading $_POST['position'] can be simplified. Instead of relying on value casting or a longer conditional expression, we can use ! empty() for a more concise and readable approach.

$position = ( isset( $_POST['position'] ) && (int) $_POST['position'] ) ? (int) $_POST['position'] : '-1';

TO:

$position = ! empty( $_POST['position'] ) ? (int) $_POST['position'] : -1;

[swissspidy]

I would advise against introducing more usages of empty(), see for example ​https://localheinz.com/articles/2023/05/10/avoiding-empty-in-php/

[dilipbheda]

@swissspidy Should we use the following example instead of empty()?

( isset( $_POST['position'] ) && $_POST['position'] > 0 )

[swissspidy]

I think the current code is perfectly fine as-is

[SirLouen]

@dilipbheda if I'm not wrong its even cleaner to use the null coalescing operator

Look how clean it can be:

$position = (int) ($_POST['position'] ?? '-1');

[dilipbheda]

@SirLouen It looks good to me — I’ll update it in my PR

[dilipbheda]

@SirLouen I checked your suggestion — it allows zero as well. If we want to allow only positive numbers, we should do it like this:
( isset( $_POST['position'] ) && $_POST['position'] > 0 )

Let me know your thoughts

[siliconforks]

Note that these are not equivalent to the original code in handling invalid input.

The original code:

$position = ( isset( $_POST['position'] ) && (int) $_POST['position'] ) ? (int) $_POST['position'] : '-1';

The original code above guarantees that $position will never be zero.

$position = ! empty( $_POST['position'] ) ? (int) $_POST['position'] : -1;

The proposed code above will set $position to zero when $_POST['position'] contains a string like 'xyz'.

$position = ( isset( $_POST['position'] ) && $_POST['position'] > 0 ) ? (int) $_POST['position'] : -1;

That will also set $position to zero.

$position = (int) ($_POST['position'] ?? '-1');

Same here.

[johnbillion]

This sort of trivial tweak in't a good use of anybody's time unless it fixes an actual bug. There's more info about this policy here: ​https://make.wordpress.org/core/handbook/contribute/code-refactoring/. Thanks for the contribution, but time is best spent on real bugs and enhancements.

[SirLouen]

Replying to siliconforks:

Note that these are not equivalent to the original code in handling invalid input.

True, ​I've tested and you are right in this regard
Don't ask me why but I was trusting that this was actually a PHPCS error (something like a usage of a non-sanitized variable or something like that) that needed to be addressed. But now I'm checking, and this is not a PHPCS trouble or whatsoever…

@dilipbheda, out of curiosity, what were you looking when you got to this idea to try to fix the code?

[dilipbheda]

@dilipbheda, out of curiosity, what were you looking when you got to this idea to try to fix the code?

@SirLouen While reviewing the code, I noticed unnecessary casting in this condition, nothing else.

Here ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/ajax-actions.php#L1410

[SirLouen]

Replying to dilipbheda:

@dilipbheda, out of curiosity, what were you looking when you got to this idea to try to fix the code?

@SirLouen While reviewing the code, I noticed unnecessary casting in this condition, nothing else.

Here ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/ajax-actions.php#L1410

ngl, I'm amazed, why are you simply "reviewing" the code at random, instead of checking some reported bug or doing some tests?

It can feel paradoxical, but reviewing the code is a practice that is not recommended to be done.

Overall, these proactive tasks (like reviewing code, sorting future uncertain deprecations, etc…) are a bad idea because they don't provide use-cases and without a use-case it's very difficult to test.

To reach a conclusion, we have to rely single-handedly on the good knowledge (and very uncommon) of people like @siliconforks to consider many possible scenarios for such review without breaking the system

I'm going to publish soon an article in dev blog about this topic, I will send it to you in Slack when it’s live.

Thanks for the PR! Closing for now, as the associated Trac ticket is closed.