Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
237 stars 40 forks source link

Add warning for list_files function and related #704

Open ovidiul opened 2 years ago

ovidiul commented 2 years ago

What problem would the enhancement address for VIP?

I've recently been doing a code review and noticed the list_files function had no warning attached. On VIP Filesystem, the list_files, scandir, opendir will return empty result sets or false, so we should probably highlight this to customers to avoid unexpected results.

Describe the solution you'd like

A warning should be added when the following functions are coded:

What code should be reported as a violation?

$files = list_files( $folder, 2 );

What code should not be reported as a violation?

Additional context

GaryJones commented 1 year ago

These would need to go into https://github.com/Automattic/VIP-Coding-Standards/blob/develop/WordPressVIPMinimum/Sniffs/Functions/RestrictedFunctionsSniff.php

GaryJones commented 1 year ago

I've tested this on a VIP test site, and I'm getting the expected results for each of them, so I will close this out.

cc @yolih and @raamdev, who I think had worked on docs around this that may need to be reverted/updated.

yolih commented 1 year ago

The Docs currently state:

scandir(), list_files(), and opendir() will not work as expected, and will instead return either an empty array or false and trigger a PHP Warning.

So I believe we are in sync with the VIP Coding Standards rules for warnings. Thanks for cross-ping!

jrfnl commented 1 year ago

@yolih @GaryJones I have a feeling there is some miscommunication going on here.

When I read @GaryJones' message, I get the impression he got proper correct file listings, not an empty array. @GaryJones is that correct ? If so, that would mean the docs would need to be updated to remove the warning about those functions ?

GaryJones commented 1 year ago

Correct. On my VIP test site, the three functions worked as expected. I can get it verified, and if so, or docs need fixing.

raamdev commented 1 year ago

The docs are referring to file listing functions on wp-content/uploads/ paths on VIP, which utilize the VIP Filesystem.

I've tested and confirmed that the following is true when those functions are used on wp-content/uploads/:

scandir(), list_files(), and opendir() will not work as expected, and will instead return either an empty array or false and trigger a PHP Warning.

I will reopen this issue with the caveat that we should produce a warning indicating that if those functions are being used on a path that points to the wp-content/uploads/ directory, they will not work as intended.