PHPCS fixes: strict comparison, rawurlencode(), sanitization

[paulschreiber]

Fixes most warnings and errors found by PHPCS with the WordPress-VIP ruleset.

[paulschreiber]

Oddly, tests fail because WordPress's functions aren't loaded:

vendor/bin/phpunit
PHPUnit 5.7.14 by Sebastian Bergmann and contributors.

.........PHP Fatal error:  Call to undefined function sanitize_text_field() in /Users/schreipa/.dev/wp-async-task/wp-async-task.php on line 160

[ericmann]

Oddly, tests fail because WordPress's functions aren't loaded

That's because the test suite uses WP_Mock to "mock" the WordPress API. Since you've added a call to sanitize_text_field(), you'll also have to add a function mock for the same.

That being said, you shouldn't be using sanitize_text_field() here. The point of that function is to sanitize user input before it's put in the database. We're not writing anything to a database here at all. The user-specified nonce is being used in a hashing function to verify that the request occurred within a valid time period. Feel free to take a look at the source of the code itself, but sanitization is unnecessary here.

[paulschreiber]

I added mocks (using wpPassthruFunction) for sanitize_text_field() and wp_unslash() and am now getting this error:

Fatal error: Call to undefined function wp_nonce_tick() in /Users/paul/wp-async-task/wp-async-task.php on line 205

Why can't it find the wp_nonce_tick() mock?

Regarding the sanitization: sure, this will work fine without it. However, if we didn't include it, we'd need a // @codingStandardsIgnoreLine to avoid PHPCS warnings. Suppressing the errors means we might not see future errors introduced on that line.

[ericmann]

Why can't it find the wp_nonce_tick() mock?

Because there isn't one. Instead, the test that's failing is trying to mock the internal verify_async_nonce method directly. Unfortunately, the code was always expecting verify_async_nonce to be called with a simple string ... putting sanitize_text_field and wp_unslash inside it means the mocking engine can't identify the right function to actually mock.

Let me step back a bit. My recommendation here is that you remove the addition of both of these functions (sanitize_text_field and wp_unslash) from the nonce check. As I already mentioned, sanitization isn't really appropriate here. Also, wp_unslash is meant for: "This should be used to remove slashes from data passed to core API that expects data to be unslashed." That's not true in this case, so it's unnecessary.

Regarding the sanitization: sure, this will work fine without it. However, if we didn't include it, we'd need a // @codingStandardsIgnoreLine to avoid PHPCS warnings. Suppressing the errors means we might not see future errors introduced on that line.

If that's what we need for the CS tests to pass, then add it. Code Sniffer is a great tool, but sometimes (like in this instance) it presents false positives because it's just pattern matching and reporting where patterns are suspect. The changes it's recommending here are unnecessary.

If you don't want to skip CS on this chunk of code, then you've got a lot of work left to do to polish things up:

• The call to sanitize_text_field( wp_unslash( $_POST['_nonce'] ) ) needs to be moved outside the invoked (and mocked) internal function. I'd personally stick it in another variable (called $nonce perhaps) and then pass the variable into verify_async_nonce directly.
• Once that's extracted, the test for an empty $_POST[ 'nonce' ] is a bit ... off. Instead, we should have one explicit test for an empty POST - then die. After that test, then populate the variable and run the nonce verification (and die if needed). Again, unfortunately, this change in architecture drastically changes the structure of the function as we now have more than one potential exit point. I really don't want to do that, but if you're sticking to your guns, it's what needs to happen here.
• Finally, you need the mocks on sanitize_text_field and wp_unslash to apply to the test_handle_postback_anon test otherwise things will fail for invalid invocations there as well.

So ... it's faster and cleaner to just note that we know CS will return a false hit on this code than to try to mangle code such that is passes CS's internal pattern filters.

[paulschreiber]

I've removed the sanitize_text_field() and wp_unslash() calls from verify_async_nonce().

[paulschreiber]

Posting this so I can learn how this works (I've removed that change):

Because there isn't one. Instead, the test that's failing is trying to mock the internal verify_async_nonce method directly. Unfortunately, the code was always expecting verify_async_nonce to be called with a simple string ... putting sanitize_text_field and wp_unslash inside it means the mocking engine can't identify the right function to actually mock.

Calling verify_async_nonce( trim( trim( $_POST['_nonce'] ) ) works as before. So the problem isn't the call stack … or that's it not a string. I'm confused.

[ericmann]

trim is a native function. The other two are not. Internally, this changes the way the verify_asnyc_nonce() function is represented at runtime and the underlying mocking engine can't match the mocked version of the function with the invoked version.