Re-categorize sniffs in the VIP folder#344
Conversation
…s and add logic in RestrictedFunctionsSniff to check that it's actually a function we're looking for
GaryJones
left a comment
GaryJones
left a comment
There was a problem hiding this comment.
Awesome work - only a few minor amendments, then looks good to go.
Local composer test passes fine.
Will need a big update to the audit (which I can handle)!
WordPress-VIP-Go/ruleset.xml
Outdated
| <message>Having more than 100 posts returned per page can lead to severe performance problems.</message> | ||
| </rule> | ||
| <rule ref="WordPressVIPMinimum.Filters.RestrictedHook.UploadMimes"> | ||
| <rule ref="WordPressVIPMinimum.Hooks.RestrictedHook.UploadMimes"> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| 'get_children', | ||
| ], | ||
| ], | ||
| 'deprecated' => [ |
There was a problem hiding this comment.
This check for create_function is tipping into PHPCompatibility territory (as was the previous sniff).
However, thinking ahead, if function x() got deprecated in PHP 7.3, then we'd either have to generalise the message to not contain the version, or add in another deprecated (but with a different name) category.
There are lots of deprecations in PHP 7.2 - so why does this one get special treatment?
There was a problem hiding this comment.
I'm thinking that it gets special treatment because of: https://vip.wordpress.com/documentation/vip-go/code-review-blockers-warnings-notices/#eval-and-create_function
I'll update the error message to match Squiz.PHP.Eval.Discouraged also with a notification that it's deprecated.
There was a problem hiding this comment.
Fair point @rebeccahum.
@tessaneedham Do you know why create_function gets attention in the Docs, yet none of the other deprecations for 7.2 get mentioned?
Could it be generalised to advise that any PHP-level deprecated functions should be avoided (PHPCS warning or error)?
| namespace WordPressVIPMinimum\Sniffs\Hooks; | ||
|
|
||
| use WordPress\AbstractFunctionParameterSniff; | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -14,7 +14,7 @@ | |||
| * | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ruleset_test.inc
Outdated
| @file_get_contents( $foo ); // Error + Warning. | ||
|
|
||
| // WordPressVIPMinimum.VIP.RegexpCompare + WordPress.VIP.SlowDBQuery | ||
| // WordPressVIPMinimum.Performance.RegexpCompare + WordPress.VIP.SlowDBQuery |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…r messsage, rename ConstantRestrictions and fix references of WordPress.VIP
|
Audit has now been done. |
2 participants
Fixes #241. This PR moves a bunch of sniffs into new categories like so:
VIP —> Compatibility
RobotstxtPlugin —> Compatibility
ZoninatorVIP —> Functions
RestrictedFunctionsActions —> Hooks
PreGetPostsCache —> Performance
BatcacheWhitelistedParametersCacheValueOverrideLowExpiryCacheTimeVIP —> Performance
FetchingRemoteDataNoPagingOrderByRandRegexpCompareRemoteRequestTimeoutTaxonomyMetaInOptionsWPQueryParamsVIP —> Security
EscapingVoidReturnFunctionsExitAfterRedirectPHPFilterFunctionsProperEscapingFunctionStaticStrreplaceTemplatingEngine —> Security
MustacheTwigUnderscorejsVuejsVIP —> UserExperience
AdminBarRemovalVIP —> Variables
RestrictedVariablesOther things included in this PR:
ConstantRestrictionstoRestrictedConstantsAlwaysReturntoAlwaysReturnInFilterCreateFunctionsintoRestrictedFunctionsMustache,Twig,UnderscorejsandVuejsby removing theUnescapedOutputprefix