Review the WordPressVIPMinimum.Security.Mustache sniff

[jrfnl]

Review the WordPressVIPMinimum.Security.Mustache sniff for the following in as far as relevant to that sniff:

• Code style independent sniffing / Correct handling of quirky code
  Typical things to add tests for and verify correct handling of:
  • Nested function/closure declarations
  • Nested class declarations
  • Comments in unexpected places
  • Variables being assigned to via list statements
  • Multiline text strings
  • Text strings provided via heredoc/nowdoc
  • Use of short open tags
  • Using PHP close tag as end of statement
  • Inline control structures (without braces)
• Code simplifications which can be made using PHPCSUtils
• Sniff stability improvements which can be made using PHPCSUtils
• Correct handling of modern PHP code
  Typical things to add tests for and verify correct handling of (where applicable):
  • PHP 5.0 Try/catch/finally (PHP 5.5) and exceptions
  • PHP 5.3 Namespaced code vs code in the global namespace
  • PHP 5.3 Use import statements, incl aliasing
  • PHP 5.3 Short ternaries
  • PHP 5.3 Closures, incl closure use
  • PHP 5.4 Short arrays
  • PHP 5.5 Class name resolution using ::class
  • PHP 5.5 List in foreach
  • PHP 5.5/7.0 Generators using yield and yield from
  • PHP 5.6 Constant scalar expressions
  • PHP 5.6 Importing via use function/const
  • PHP 7.0 Null coalesce
  • PHP 7.0 Anonymous classes
  • PHP 7.0 Scalar and return type declarations
  • PHP 7.0 Group use statements
  • PHP 7.1 Short lists
  • PHP 7.1 Keyed lists
  • PHP 7.1 Multi-catch
  • PHP 7.1 Nullable types
  • PHP 7.3 List reference assignments
  • PHP 7.4 arrow functions
  • PHP 7.4 numeric literals with underscores
  • PHP 7.4 null coalesce equals
  • PHP 7.4 Typed properties
  • Various versions: trailing comma's in function calls, group use, function declarations, closure use etc

Other:

• Review violation error vs warning
• Review violation severity
• Review violation message, consider adding a link
• Check open issues related to the sniff
• Review PHPDoc comments

Sniff basics, but changes need to be lined up for next major release:

• Inappropriate use of public properties (Audit Public Sniff Properties #234)
• Modular error codes (unique error code for each distinct message)

Once PHPCS/PHPCSUtils supports this:

• PHP 8.0 Constructor property promotion
• PHP 8.0 Union types
• PHP 8.0 match expressions
• PHP 8.0 Nullsafe operator
• PHP 8.0 Named arguments
• PHP 8.0 Single token namespaced names

[jrfnl]

I've had a quick initial look at this sniff with an eye on #552 / PHPCS 4.x.

The sniff currently examines both PHP and JS files, but does not have a test case file for the JS side of things, nor any JS syntax specific sniff code.

Questions: As PHPCS 4.x may still be a year or more before it is released:

1. Should a JS test case file be added to verify that JS is currently actually being examined correctly ?
   If no, I'd propose to stop sniffing js files.
2. If yes and if necessary, should support for properly checking JS code be added for the time being ?
   If yes, this should probably get priority to have the benefit of these checks for as long as possible (until support for PHPCS 3.x is dropped).

[BrookeDot]

It recently came up that this rule gets invoked when scanning Block Editor Templates, for example:

<div class="wp-block-group"><!-- wp:heading {"textAlign":"center","style":{"spacing":{"margin":{"top":"0","right":"0","bottom":"var:preset|spacing|medium","left":"0"}}}} --> 

will return:

| WARNING | Found Mustache unescaped output notation: "{{{}}}".
| | (WordPressVIPMinimum.Security.Mustache.OutputNotation)

[GaryJones]

Docs.

I wonder if only checking for {{{, or looking for both {{{ and }}} in the same statement/scope, or not checking inside HTML comments, would be beneficial here.

Another extreme option would be to either scrap this sniff (I don't think Mustache is being used by many customers) or move it into a new package to be used by those who do want it.

[jrfnl]

or looking for both {{{ and }}} in the same statement/scope

I think this would be the simplest solution (for now), which should allow to get rid of the false positives.

Shall we add this to the 3.0 milestone ?

[GaryJones]

Shall we add this to the 3.0 milestone ?

Yes please.