Security/EscapeOutput: fix false negatives when handling anonymous classes

[rodrigoprimo]

This PR fixes false negatives when the sniff WordPress.Security.EscapeOutput handles readonly anonymous classes and anonymous classes with attributes that are part of a throw statement.

When stepping over tokens after T_THROW to find the T_OPEN_PARENTHESIS of the exception creation function call/class instantiation, the sniff was not considering that it might need to step over T_READONLY tokens or attribute declarations when dealing with anonymous classes.

Fixes #2552

[jrfnl]

Hi @rodrigoprimo Thank you for working on this.

Unfortunately, this is not sufficient for attributes as there can be multiple attributes attached to a class:

throw new
    #[Attribute1]
    #[Attribute2('text', 10]
    class( $param ) {};

You should be able to find various examples of skipping over attributes if you search for T_ATTRIBUTE (especially in the PHPCS repo).

[rodrigoprimo]

Thanks for the review, @jrfnl.

I thought about multiple attributes, but only when declared using the same T_ATTRIBUTE token. I forgot to consider that they could also be declared using more than one T_ATTRIBUTE token.

I pushed a commit adding support for this. As you suggested, I checked the PHPCS code for inspiration on how to skip over attributes. The examples that I find there use a do/while loop. I opted to implement the solution here using a while loop as the resulting code seems more straightforward to me (one less nesting level). Please let me know if there is a reason to use do/while that I'm missing.

This PR is ready for another review.

[jrfnl]

The examples that I find there use a do/while loop. I opted to implement the solution here using a while loop as the resulting code seems more straightforward to me (one less nesting level). Please let me know if there is a reason to use do/while that I'm missing.

The reason is to avoid the code duplication which is now being introduced....

[rodrigoprimo]

The reason is to avoid the code duplication which is now being introduced....

I'm sorry, but it is not clear to me which code duplication you are referring to.

Here is how I implemented the code using while:

while ( \T_ATTRIBUTE === $this->tokens[ $next_relevant ]['code'] ) {
	if ( isset( $this->tokens[ $next_relevant ]['attribute_closer'] ) === false ) {
		return;
	}

	$next_relevant = $this->phpcsFile->findNext(
		$ignore,
		( $this->tokens[ $next_relevant ]['attribute_closer'] + 1 ),
		null,
		true
	);

	if ( false === $next_relevant ) {
		return;
	}
}

And here is how I would implement it using do/while:

do {
	if ( \T_ATTRIBUTE === $this->tokens[ $next_relevant ]['code'] ) {
		if ( isset( $this->tokens[ $next_relevant ]['attribute_closer'] ) === false ) {
			return;
		}

		$next_relevant = $this->phpcsFile->findNext(
			$ignore,
			( $this->tokens[ $next_relevant ]['attribute_closer'] + 1 ),
			null,
			true
		);

		if ( false === $next_relevant ) {
			return;
		}

		continue;
	}

	break;
} while ( true );

What is the code duplication that you are referring to? Or maybe there is a simpler way to implement this solution using do/while that I'm not seeing?

[jrfnl]

The reason is to avoid the code duplication which is now being introduced....

I'm sorry, but it is not clear to me which code duplication you are referring to.

What is the code duplication that you are referring to? Or maybe there is a simpler way to implement this solution using do/while that I'm not seeing?

Line 225-228 is basically duplicated in the new code block. With a do-while, the line 225-235 would move into the control structure and should remove the code duplication.

[jrfnl]

While in a call with @rodrigoprimo we discussed the while vs do-while and I added a commit with an example of how it could be done. Not a definitive commit, just something for @rodrigoprimo to think about.

[rodrigoprimo]

Thanks for your help during the call, @jrfnl. The commit you added looks good to me as is. I just added another commit as we discussed including a new test that will make the condition in the do/while loop evaluate to false. This PR should be ready for a final review.

[jrfnl]

Thanks @rodrigoprimo ! This looks good to me.

Notes for other reviewers:

• The actual code change will be easier to review while ignoring whitespace changes.
• Please SQUASH-MERGE this PR!

[jrfnl]

@rodrigoprimo Would you mind rebasing this PR to solve the conflicts ?

@dingo-d Got a chance to review this PR ? If you approve, please squash-merge.

[rodrigoprimo]

@rodrigoprimo Would you mind rebasing this PR to solve the conflicts ?

Sure, I rebased this PR and solved the conflicts.