Security/EscapeOutput: false negatives when handling anonymous classes

[rodrigoprimo]

While reviewing #2550, @jrfnl reported false negatives when the WordPress.Security.EscapeOutput sniff handles anonymous classes:

• EscapeOutput - code in the case \T_THROW block will yield false negatives for readonly anonymous classes and anonymous classes with attributes for code like throw new [...] class( $unescaped ) {};.

My approval is for the PR as-is and presumes the missing tests for EscapeOutput will be addressed in a separate PR.

I'm opening this issue to check a few details instead of discussing them on #2550.

I can see how an anonymous class with attributes will yield false negatives. For example, the code snippets below should trigger errors, but that doesn't happen:

throw new #[MyAttribute] class( $unescaped ) extends Exception {};
throw new #[MyAttribute1(FirstParam::CONST, 'another parameter'), MyAttribute2] class( $unescaped ) extends Exception {};

I'm unsure about readonly anonymous classes. I can see how something like the snippets below are false negatives:

throw new readonly class( $unescaped ) {};
throw new readonly class( $unescaped ) extends Exception {};

But they are also fatal errors. The anonymous class needs to extend Exception or Error, but that is not possible for an anonymous readonly class, as Exception and Error are non-readonly.

@jrfnl, should the sniff trigger an error when handling readonly anonymous classes in a throw statement, even though it is a fatal error? Or is there an example that is not a fatal error that I'm missing?

[jrfnl]

I'm unsure about readonly anonymous classes. I can see how something like the snippets below are false negatives:

throw new readonly class( $unescaped ) {};
throw new readonly class( $unescaped ) extends Exception {};

But they are also fatal errors. The anonymous class needs to extend Exception or Error, but that is not possible for an anonymous readonly class, as Exception and Error are non-readonly.

In my opinion, it is irrelevant that it's a fatal error. The sniff is not about checking whether the PHP code is correct, it is just and only about whether things are escaped correctly.

@jrfnl should the sniff trigger an error when handling readonly anonymous classes in a throw statement, even though it is a fatal error? Or is there an example that is not a fatal error that I'm missing?

I can't currently think of an example that wouldn't be a fatal, but if it were up to me, I'd just adjust to skip over the readonly keyword as for the purposes of the sniff, the keyword is irrelevant.