VIP-Coding-Standards icon indicating copy to clipboard operation
VIP-Coding-Standards copied to clipboard

Add warning for list_files function and related

Open ovidiul opened this issue 3 years ago • 6 comments

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:

  • list_files - WP
  • scandir - PHP
  • opendir - PHP

What code should be reported as a violation?

$files = list_files( $folder, 2 );

What code should not be reported as a violation?

Additional context

ovidiul avatar Dec 15 '21 14:12 ovidiul

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

GaryJones avatar Jan 18 '23 13:01 GaryJones

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.

GaryJones avatar Mar 05 '23 11:03 GaryJones

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!

yolih avatar Mar 06 '23 16:03 yolih

@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 ?

jrfnl avatar Mar 07 '23 00:03 jrfnl

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

GaryJones avatar Mar 07 '23 07:03 GaryJones

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.

raamdev avatar Mar 09 '23 00:03 raamdev