You say “very little” styling errors
I will run the plugin tomorrow so I can remove them as well.
The other errors you mention would be what we would call “false alarms” most likely and a comment would of course help the reviewer to see that you are aware of them, and help to decide much quicker about false/true alarm
I’m not sure about the domain thingy.
I’ve seen many don’t use it… honestly no idea if it’s something we want or not.
Thanks for trying it!
I don’t see this as something people don’t want, just something we don’t want forced.
I think the line endings check might be important to leave in there. It seems like it affected SVN due to the way the properties were set up. While we are not using SVN, it seems like it might be a problem with other VCS. Also, I know the users of a popular theme used to have a big issue with the byte order mark of certain files. Some editors would put it and some would ignore it, but the code didn’t like it.
There are coding standards for JS also, and WP uses ESLint (I think).
Are you in favor of requiring basic unit tests?
How about screenshots of desktop and mobile?
How about proof of testing on Mac and Windows and Android?
How about multisite? How about screen reader tests?
How about testing on a range of PHP versions?
It seems to me that if you want to improve code quality, you would make people do testing of their code.
In WP, there are many more translations of the default domain. Omitting the domain argument means it uses the default domain, which may or may not have an exactly matching string. Since you have no control of those strings, and they could be changed at any time, it’s better if you use only one domain for all the text in your plugin.
For people like me using phpcs this is very easy… like:
phpcs . --standard=PHPCompatibility --runtime-set testVersion 5.6
At this point I’ll not discuss what I’m in favor.
I’ll only work on things that are related to security.
Screenshot, mobile testing, unit testing, and end of line code are all totally unrelated to what we are working on here meanwhile.
No one uses svn either in cp. and if they do, it’s not on us forcing their freedom, right?
Everyone’s free to run into a wall of concrete 
We just need to make sure security is ok. Which so far the amended sniff does.
It doesn’t force anyones freedom
However - if we also can’t enforce that, because of “freedom”, I’m out of here for good.
It’d mean we willingly drop security because of “freedom”
If you want your plugin listed on the directory it has to be following security standards.
The sniff at this point focuses only on that
I leave other “recommendations” to the folks who have time to talk to the wall.
Freedom to style their code, not freedom to be stupid and ignore security 
Style doesn’t affect end user, security does. That’s what we can enforce to protect end users.
@Simone thanks for sharing these plugins to test with.
I have removed a couple more rules (the ones relating to style standards), so now when scanning those 2 plugins, you get only non-style related warnings and errors.
I guess, the more plugins we review, the more “styles” we will see and the more we can remove.
Anticipating the question “why do you not just exclude the entire Squiz or PEAR or PSR2?” - the reason is, while many of those rules are just for styling doctrines, some are actually about (at least perceived) security - and there is no pattern to exclude something like /Squiz.PHP.THIS_IS_A_STYLE_RULE.rule because the rules are indeed named arbitrary and do not have a “prefix” indicating that it is a style, security or else related rule.
Update
some rules are part of subsets and can be excluded in bulk. The XML is updated with this in mind.
The sub-rules that the bulk removal removes, are still in the XML for reference but commented. (since we are experimenting at this stage, it might be wise to keep some sort of log/reference)
So… we can (correct me if wrong) not exclude the entire ruleset, without losing rules that we want to keep (like not using eval, for example, or at least warn about its usage in a codebase).
The changes are in the file I shared earlier, so re-scanning with that new ruleset should remove the leftover “unwanted” warnings and leave only the ones we want.
A word on escaping nonces.
While not entirely related, a while ago WPCS told me to escape third argument of wp_die().
And it is actually a valid request. The function is not escaping or sanitising it for us.
Similar, a nonce comes from a form input, even if you put it there, someone can edit the value in the hidden fields in the form.
That value then gets submitted and compared using verify nonce. Verify nonce does not escape or sanitise the input you give it, it just typecasts it to string. Additionally it is seems to be pluggable.
While obviously the nonce would fail, if not matching, and also after typecasting to string there is little (or none?) chance for anything in it to compromise your code even if plugged, it is not escaped neither sanitised explicitly for us.
So the warning is correct.
Whether false alarm or not, is above my level of understanding but I do follow that rule and do unslash and sanitize it (with sanitize_key).
I prefer to be safe, if I do not fully understand the process (which I often do not), rather than leaving it to Stackoverflow examples (or even WP Doc examples, which in many cases fully miss to notify you about the real need of escaping or sanitising).
Also see this discussion on which I based my decision.
What I learned from that discussion (and many other similar ones I read) is that … even security aspects get personalised and opinionated, and not even very experienced folks do really have the full confidence to say “no” or “yes”
And anyone telling you differently, and claiming to know so well, first has to prove me that they are right by making WPCS Devs actually remove the rule from their ruleset (and that goes with all other such rules that folks often discuss as not-needed, and often are really false alarms).
If it is in WPCS, I follow it. Because, as for example the shared discussion shows, there is reasoning behind it and not even the gurus are confident enough to remove it. So why would I ignore it?
It’s not that I have to work so much more to satisfy that rule. And I can sleep at night, knowing I did the best I could with the little knowledge I have
Of course, folks will disagree with this and have all sorts of deep insight to share. Yet I have to see folks who actually contribute to these things daily to take a stance where they say “no, this is not needed, we remove it”. Then, I will remove it too.
Also, often it gets said that “these functions slow down the application”. While that might be true in a range of milliseconds, I then wonder how the same folks who say that, go and add 10 gazillion lines of “wow JS” to their website, 10 tracking tools, 2 chats, 4000 pixel large images, and complain about a sanitise function slowing down the app
It’s… well, this is a good read on the topic (old but IMO still actual) wp_kses Performance. It mainly is about wp_kses but also shows results of other functions, which should show very nicely that the performance argument is … 
I now also added in a PHP compatibility ruleset.
To use this, follow GitHub - PHPCompatibility/PHPCompatibility: PHP Compatibility check for PHP_CodeSniffer to install.
The sniff is not enabled by default in the custom XML.
To enable it, uncomment this section
<!-- PHP Version Test -->
<config name="testVersion" value="5.2-"/>
<rule ref="PHPCompatibility">
<include-pattern>*\.php$</include-pattern>
</rule>
With this sniff, for example one of my plugins got flagged with Only variables can be passed to empty() prior to PHP 5.5. (while I am passing a method and miss to declare a minimum required PHP version)
I am not sure anymore what PHP CP supports officially but it can be amended in the XML:
value="5.2-"
(more to that ruleset here GitHub - WordPress/WordPress-Coding-Standards: PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions)
With this, we can also check if the plugin is compatible with the PHP it claims to be. While not a security related issue, I think it is a valid sniff that is very close to security since it might flag things that got removed due to security, who knows.
Also since disabled by default, it won’t disturb … meh… Freedom.
Going on with Vars.
Here I don’t want to validate input, so a comment tells people I’m aware of this and a directive tells phpcs to ignore just a test on this line, so I have no to look again at this when I review the code.
// Var can contain almost anything so here we trust our user (default capability to edit this is manage options).
parse_str( wp_unslash( $_POST['allvars'] ), $testvars ); //phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
My opinion is that I must review translations, so there is no need to sanitize them but let’s follow WPCS.
/* translators: 1 is the number of user. 2 is the list of users. You can use only the <i> tag in translation */
$usermessage = sprintf( _n( '%1$d user has <i>%3$s</i> capability and so can change vars: %2$s.', '%1$d users have <i>%3$s</i> capability and so can change vars: %2$s.', $count, 'vars' ), $count, rtrim( $userlist, ', ' ), get_option( 'vars-whocanedit', 'manage_options' ) );
echo wp_kses($usermessage, array('i'));
Here phpcs can’t know that I’ve sanitized my input elsewhere. It also reminds me that if I call this function elsewhere I must sanitize errors before passing to the function.
// As long as $errors is passed from vars_save_security_settings is sanitized there
echo $errors; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
Opinions?
My opinion, and some of the topics on that are very infinite in possibilities and also not fully clarified by doc, for example, but still, I think it might help:
The sniff I created actually should ignore your PHPCS annotation, because devs might think to be smart and bypass things by adding the ignore statement. So we want to see them anyway.
It is absolutely correct to add a comment.
The annotation should however not work when using the sniff shared earlier (but of course if you have your own sniff setup to not ignore those, then yes it will hide the error).
I would still recommend that this is exactly a case where you want to be very sure about what comes in.
While cap is one thing that makes things safer… the very “right” thing would be a custom kses maybe, that allows a range of things you might expect, but not others.
However, of course, the reviewer does not know what you intend to do with that input, so if it is entirely impossible to validate, I guess it would be fine (given it is only for manage option cap users)
Strictly speaking however that is not enough. I am not sure this would pass WP Repo review.
Do you expect HTML in that $var?
If not, sanitise it so that it can’t hold any.
Or, do you expect special tags, but not others? Then you can use kses to allow some, not others.
However the context would show more I guess.
It is one of those areas where I personally, if reviewing this, would likely ask someone to help me review just to be sure.
My opinion is that I must review translations, so there is no need to sanitize them but let’s follow WPCS.
This is not only your opinion, as a matter of fact this is something that seems the more wide idea here at CP. I disagree with it, but the idea is not entirely wrong.
However, as long we do not have a centralised translation management also for Plugins that gets reviewed… it should be sanitised/escaped I think - not only because of security but also because stuff might just simply by mistake hold things that can break your code. Generally speaking that is.
Here phpcs can’t know that I’ve sanitized my input elsewhere.
That is a classic one which I hate because it puts the entire “escape late, sanitise early” talk into a perspective of “how please?!”, and can be found through out the entire core code.
Often your $var has complex HTML and not even wp_kses will help you… so Yes, in those cases, escape on echo is plain simple not possible. Nothing we can do here.
The best approach I think here is to try avoiding this kind of $var output whenever possible and output chunks insetead, but it is simply not possible sometimes (hey, looking at you the_content()!).
And yes a comment elaborating helps and should be enough in this case.
If it is not obvious from the previous few lines, an @see annotation also helps, so the other dev can quickly see whatever other function or code does the actual escaping or sanitisation.
I would expect to get a “you forgot to sanitise” feedback from reviewers (especially the busy ones like at WP will not bother long about reading code), and then I would have to feedback with a detailed explanation as of why it is safe and OK.
That should hopefully not happen here as we are not that busy with reviews, but still might happen. So whenever delivering such code, just be prepared to have a well backed up reasoning (if not already put in code comment)
PS:
I am not the most experienced security tool here (I am a tool, but not really a security tool 
).
So I have hopes that some experienced people could also feedback on it, but I think most what will be the result is “it depends on the context” (which puts me as noob back to the thinking pad trying to avoid any such case where context needs to be interpreted so I can avoid any “ambiguity”)
Hope it helps
PS2
this thread might also help about those $vars that get echoed without escaping.
Yes, I’ve noticed. This is why I added phpcs:ignore RULE-XXX on the failing rule.
So it’s not a trick to bypass reviews but something that speeds up a future code review.
Nearly everything. With a proper filter it can also execute php code…
I think wp_kses can be a good compromise.
Vars is really a nice one to play with:
About the “can execute php”, I’m relatively sure this is something wp wouldn’t allow.
So I guess this would need to be decided for us here at cp if we allow it or not.
I personally don’t have an issue with it, I guess the concern here is about “people don’t know what can happen” similar to svg support (even if for admins only).
I recall they don’t even allow storing js in custom plugins. Something that pissed me off at some point because I had worked on a forms plugin allowing you to store js for each form, and shows why some plugins then go off-repo and deliver from their own website or server instead of listing in the repo.
The official solution is I believe to store js in files and not in database
Same with “custom code snippet plugins”, I believe that’s not allowed if they are stored in the database but it’s allowed if stored as a file
It’s something we’d need to define, I think, in order to close the case for future as well.
And for that we’d need some more community feedback (I do agree on the “admins should know what they do” part. But… it kind of is one of those conflicts. For example “Disable theme code editor” but allowing to store or execute code same time in a plugin is biting each other)
Got that, what I meant was that it should not really affect the review sniff (since that simply should not listen to those annotations).
However, I discovered my rule doesnt work anyway, whether it is added or not to the XML.
I have had this issue before.
Back then I discussed this in WP Slack core standards channel, and while phpcs:ignore should work, it did not, in my case back then // @codingStandardsIgnoreLine did the trick, but this does not seem to work here either.
It does work in the IDE (where I use WPCS default standards), but not in the terminal with custom XML (with or without custom rule enabled).
I am happy with it, however interesting quirk.
I also noticed that the rule I added in the XML probably refers to @ silencing and not sniffer silencing.
In either case, the review doesnt seem to suffer from the annotation (which is all we want from that perspective).
It is definitely related to the XML, since when I sniff in terminal using WordPress standards, it is indeed ignored.
This was already discussed here.
The very first goal of Vars was to do things like © <?php echo date("Y"); ?>.
At the beginning there was a flag to eval.
Then the eval was removed from the plugin, and the filter vars_output was added, so in my websites I use this filter to eval 
.
On the command line phpcs:ignore is bypassed by your rules.
The rule affecting it is <arg name="ignore-annotations" />.
I don’t know my own added rules lol fffffff*****
Thanks for the correction, I looked at the completely wrong rule 
Confirmed it works as intended and indeed is now ignored if I were to remove that rule.
If I understand this correctly (and yes you actually mentioned that earlier already), it does not allow to execute code unless you as dev add a filter allowing it.
So that means we do not allow this “in a plugin”, like by default, but we allow it (well, can’t be avoided, really) thru a filter. Makes sense!
<exclude name="WordPress.Security.ValidatedSanitizedInput.InputNotValidated"/>
This rule does not check something that is strictly speaking security related.
It checks upon validation, which is not sanitization or security, it rather avoids errors.
Yet, it is in a group called Security… confusing.
What is the “liberal” wing’s opinion on this?
In my very conservative opinion, it should be in the rules used for evaluation and not be ignored.
Yet, if we do enforce that, there is quite a number of errors more a dev needs to “fix”.
As it is not a strict security issue and the dev is free to let his/her plugin fail with errors, I would not object to the freedom of producing errors. It does not put others security at risk. It will only upset them 
Right now, in the XML that rule is passed to the “ignore” group, thus no code is flagged when not validated.
However as I just noticed it is part of security group, thus now I am not sure anymore if we really should ignore it.
Improper input validation definitely has the potential to be a security issue: for example, if it is then saved to the database.
The only warning I’ve ever seen from that sniff is “accessing XY which maybe is undefined”
I am happy to add it back as sniff, however… I am not sure this is going to affect a database if some things are not validated (definitely sanitize is a problem, but the validation sniff seems merely a “is it defined” sniff)
The problem I see if I add it back is, since I see that error like too many times, folks will consider it “overkill”
And since we aren’t asked to check if their plugin actually works… I don’t see why a review would spend time convincing the dev that it’s safer to check if a $_POST variable is set or not (which is what that sniff more or less does)
Again, I’m definitely in the wing of those who would want it but I’m just trying to see from the point of view I’d those who do not like standards (or these standards)