Add defensive coding in posts per page sniff

[dingo-d]

This will prevent the undefined offset internal error.

Closes #2390

[jrfnl]

@dingo-d Thanks for trying to take this one on.

Unfortunately, this is not the correct fix.

Think of it this way: if the $val is an empty string will a numeric comparison ever have any value ? No, it won't.

So instead, this sniff needs to do a check of $stripped_val against an empty string, straight after the quotes have been stripped and the function should bow out and return false in that case.

The patch also needs a few tests to cover the change - the test from the report + a test with an empty string passed to posts_per_page when set as an array.

Probably also a good thing to add a similar test to the DB/SlowQuery sniff, which doesn't have the same issue due to the difference in code for callback, but still.

$query = 'foo=bar&meta_key=&meta_value=bar';

[dingo-d]

Fixed the PR according to the comments, much appreciated, should have taken a better look into the sniff.

For the DB/SlowQuery sniff I'll check this in a separate PR 👍🏼

[jrfnl]

For the DB/SlowQuery sniff I'll check this in a separate PR

The SlowQuery sniff only needs the test I provided. The callback doesn't contain logic, so is not susceptible to the same issue. The test would only safeguard that if logic would be added at some point, we won't forget to guard against this edge case.

[jrfnl]

LGTM ✅

Note: of the tests, only the tests on line 128 and 134 would trigger the notice previously. All the same, the additional tests do test the extra added logic, so 👍🏻 .

Would be nice to see the extra test for SlowQuery added as well, but happy to ship this as is.

[jrfnl]

@dingo-d P.S.: feel free to squash the commits.

[dingo-d]

@dingo-d P.S.: feel free to squash the commits.

Do you want me to squash all into one, or separate the test ones from the sniff changes ones?

[jrfnl]

Do you want me to squash all into one, or separate the test ones from the sniff changes ones?

No, this should be one commit. The tests belong with the functional change, so should be included in the same commit (makes life a lot easier when checking history).

[GaryJones]

✅