Convert htmlAttrNotByEscHTML to warning or back to error for consistency with other rules

[rebeccahum]

We typically don't have any errors that are below 5 (with 5 being the default severity level).

https://github.com/Automattic/VIP-Coding-Standards/blob/60ad148168258afccd05ff0429d53a7e810270a5/WordPress-VIP-Go/ruleset.xml#L259-L261

However, on the VIP Go ruleset, we have WordPressVIPMinimum.Security.ProperEscapingFunction.htmlAttrNotByEscHTML marked as an error at a level 3. I think this could cause potential confusion and for consistency's sake, we should either mark it as a warning with a higher severity or bring it back to the default error level.

FWIW, I don't have the exact context on why it was brought down to a level 3 and the PR where it was introduced has no description.

[rebeccahum]

This also begs the question if we should audit the other rules in the Go sniff to ensure they are consistent with our code review policies.

[rebeccahum]

Opened https://github.com/Automattic/VIP-Coding-Standards/issues/602 for tracking our auditing.

[jrfnl]

As per the remark in the ruleset:

This is still safe, just sub-optimal

If you look at the code for the functions involved in WP Core, the only real difference is that they apply a different filter to the end-result, the basic escaping for esc_attr() and esc_html() is exactly the same.

The wrong filter being applied is what I interpret as the "sub-optimal" part of things.

https://core.trac.wordpress.org/browser/tags/5.6/src/wp-includes/formatting.php#L4477-L4516

Based on this, I'd recommend changing this particular error code to a warning.

[GaryJones]

I missed this one.

While the only difference currently is the different filter, that may not always hold true in the future - other changes could be introduced, that are backwards compatible for the expected usage, but not for when it's used in the wrong context.

We strongly push about including escaping, and using the right escaping in the right context, and yet the related change here moves it from an error to a warning. This has the potential to bite us in the future.

[jrfnl]

@GaryJones Agreed and plugins can add additional hooks as well. All the same, having it as a warning at severity 5 will probably give more visibility to the issue than an error at visibility 3 as the latter will be hidden in any default run.

[rebeccahum]

Re-opening for further discussion.

We strongly push about including escaping, and using the right escaping in the right context, and yet the related change here moves it from an error to a warning. This has the potential to bite us in the future.

You bring up a very good point here. However, since we already marked it down to a level 3 previously, I presumed that a warning seemed to be towards the same direction.

[rebeccahum]

@jrfnl Given how we strongly encourage using the correct escaping function in code review, I think it would be worth reverting #693 and keeping it at a default error level.

[jrfnl]

@rebeccahum #693 did two things:

1. Change the notice from an error to a warning.
2. Change the severity from 3 to the PHPCS default 5.

The first may have reduced visibility, while the second will definitely have increased visibility.

If anything, the first change could undone, but I would leave the second change (using the default severity level) in place.

[GaryJones]

I agree with moving severity from level 3 to level 5.

It's the Error to Warning that I'm concerned about - some folks make a point of only fixing Errors, and while their application isn't really more at risk (unless a plugin or their custom code does something exotic with one of the two filters), it doesn't set them up to be in a good place in the future.