Please consider if isset as reliable usage of an input var

[ScreamingDev]

Edit:

See comment #849 (comment)
about the common if and isset.

---

Original issue:

See this:

$type   = trim( strval( $_GET['type'] ) ); // Input var okay

Now it nags for

Detected usage of a non-validated input variable:

So I comment with

$type   = trim( strval( $_GET['type'] ) ); // WPCS: sanitization okay

Then it nags with

Detected access of super global var $_GET, probably needs manual inspection.

Help! What is it that you want from me?

[ScreamingDev]

Let me add this to the puzzle:

11 | ERROR | Detected usage of a non-sanitized input variable: $_GET
    |       | (WordPress.VIP.ValidatedSanitizedInput.InputNotSanitized)
 11 | ERROR | Missing wp_unslash() before sanitization.
    |       | (WordPress.VIP.ValidatedSanitizedInput.MissingUnslash)

[westonruter]

OK, you're using the WordPress-VIP standard? Or are you just using WordPress? If using WordPress it is a superset of all the WordPress sniffs. If you just use WordPress-Core the you won't get these extra more strict sniffs.

Also, strval isn't a sanitizing function. If you use sanitize_key you won't have to use a sanitizing whitelist comment. To whitelist the superglobal access you can use // WPCS: input var ok. The other issue you may come across is WordPress.CSRF.NonceVerification unless you are previously checking for a nonce. This should pass:

<?php
$type = null;
if ( isset( $_GET['type'] ) ) { // WPCS: input var ok; CSRF ok.
	$type = sanitize_key( wp_unslash( $_GET['type'] ) ); // WPCS: input var ok; CSRF ok.
}

You can avoid the whitelisting comments if you use a less-strict standard like WordPress-Core.

[ScreamingDev]

Thanks @westonruter . sanitize_key helped. Yes I must have VIP coding standards there.
Now it is:

		if ( ! isset( $_GET['type'] ) ) { // Input var okay
			return;
		}

		$type   = sanitize_key( $_GET['type'] ); // Input var okay

		if ( ! isset( $_GET['id'] ) || ! intval( $_GET['id'] ) ) { // Input var okay
			return;
		}

The first and last comment still nags me because it is an if with an isset. Quiet common structure.
I like to keep this issue open for:

Please consider if isset as reliable usage of an input var.

[JDGrimes]

Please consider if isset as reliable usage of an input var.

The sniff that you are referring to is intended mainly as a helper to code reviewers for the VIP platform. I think that it probably needs to remain as it is, but I always disable it in my PHPCS ruleset file:

	<rule ref="WordPress-VIP">
		<exclude name="WordPress.VIP.SuperGlobalInputUsage" />
	</rule>

[ScreamingDev]

The sniff that you are referring to is intended mainly as a helper to code reviewers for the VIP platform.

So I, as a developer, shouldn't be aware of this sniff. Is that correct?
Like: Developer might but code review must consider this rule.

If that is correct, then I would change the topic one more time to: "Please create rulesets for developers".
Feels like there can be a subset of rules that a developer must be aware of.
Or in other words: There are rules that slows developer down should not be of their concern primarily.

[dingo-d]

Well there is WordPress-Theme sniff, but it's still in the works.
You can use that as your default sniff.

I personally use modified WordPress sniffs (removed few VIP standards that are not important for me as I am not developing for VIP)...

[grappler]

So I, as a developer, shouldn't be aware of this sniff.

It depends on what your goal of using WPCS is. You can see the available rulesets and what they are meant to check. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards#rulesets

[JDGrimes]

So I, as a developer, shouldn't be aware of this sniff. Is that correct?
Like: Developer might but code review must consider this rule.

Yes, that's about right. If you are a developer performing a review of your code or running WPCS over it for the first time, you might want to use that sniff. For regular use of WPCS I just turn it off.

There has been some discussion, and maybe a ticket, to remove the whole VIP standard to be separate from WPCS, because it is aimed specifically at code being developed/reviewed for the VIP platform. However, that hasn't happened yet.

Basically though, when you run the WordPress standard over your code, you are running everything, every sniff included with WPCS. You probably want to just run WordPress-Core or WordPress-Extra, see the list of standards as @grappler suggested.

[ScreamingDev]

However, that hasn't happened yet.

Thanks for the reply. Please read this carefully as it sums up my last request:

Bring up another ruleset a bit weaker than WordPress-VIP.

This solves what you just said @JDGrimes . I know about the other rulesets. Maybe there could be a new ruleset a bit weaker than VIP but stronger than the "WordPress" rulset. I like to suggest a ruleset based on WordPress-VIP without:

• All standards that can be disabled by comment (like // Input var okay). Those are a bit arbitrary and really/only for the VIP team.
• Sniffs about the special VIP functions (for user meta etc.). Those are not of interest during development as they can be easily replaced.

I like that the VIP standards are endosed in this repo. And another one for "WordPress-VIP-dev" would be great if loose enough for developers. Anyway it would be great to have it as most developers need to do it on their own every time.

So: If you like, then I put up a merge request to bring in some more details about what I mean.
If you are not interested in this developers view and ease of developing with (most of the) VIP standards then close the issue.

[grappler]

I think the ruleset that you are looking for is WordPress-Extra

WordPress-Extra — extended ruleset for recommended best practices, not sufficiently covered in the WordPress core coding standards

Now it is not to say the WordPress-Extra is perfect. There is already an issue to include some of the checks that are under VIP in WordPress-Extra. #240 You can find the discussion here about the potentially removing VIP specific checks from WPCS. #676 (comment)

[JDGrimes]

I'm going to close this, as the original issue has been dealt with, and the remaining request is basically covered by #240.