WP.AlternativeFunctions: Add more functions to filesystem functions list

[JDGrimes]

While looking at VIP.FileSystemWritesDisallowed I saw that it listed a lot more functions than were here. I think most of them rightly belong on this list as well. The only ones I'm not sure about are fputcsv(), symlink() (WP_FileSystem doesn't support symlinks), and tempnam(). I've gone ahead and added them, but everyone is welcome to weigh in if they think they don't belong here.

Incidentally, the group name is filesystem_read, even though filesystem write functions were included as well. I assumed that was for back-compat, so I've left it.

For easier review, the first commit reorders the existing list alphabetically, the second commit adds the new functions.

[jrfnl]

Just wondering: Do all those PHP functions actually have sister-methods within the WP FileSystem ? If not, we'd just be confusing people.
If they do, it may be nicer if we'd do a direct translation, i.e. for function A and B, you can use the WP FileSystem method C. This would mean splitting the list into a multitude of groups to point to the correct functions.

Other than that, I'd want to think a bit more about legitimate use-cases not to use the WP FileSystem (other than the overhead) for any of these functions. If there are legitimate use-cases, we should probably be a bit wary of adding these functions.

[JDGrimes]

Here is a quick comparison of the functions and methods that provide direct analogs:

PHP Function	WP_FileSystem Method
chgrp	chgrp
chmod	chmod
chown	chown
delete	delete
fclose
file_get_contents	get_contents
file_put_contents	put_contents
flock
fopen
fputcsv
fputs
fread
fsockopen
ftruncate
fwrite
is_writable	is_writable
is_writeable	is_writable
lchgrp
lchown
link
mkdir	mkdir
pfsockopen
readfile
rename	move
rmdir	rmdir
symlink
tempnam
touch	touch
unlink

I think that for many of the remaining ones, a similar effect can still be achieved, but not necessarily in exactly the same way (e.g., fwrite() et al. vs put_contents()). Note that some of these (like fwrite()) were already present in the list.

Other than that, I'd want to think a bit more about legitimate use-cases not to use the WP FileSystem (other than the overhead) for any of these functions.

I'm honestly not exactly sure when WP_FileSystem needs to be used; even core is inconsistent in this AFAIK.

[aristath]

I'm honestly not exactly sure when WP_FileSystem needs to be used; even core is inconsistent in this AFAIK.

Agreed, info on that is very inconsistent - which is why we end up using it for almost all things. Just because WPCS is constantly nagging. However, I never understood what the problem is exactly with file_get_contents.
The WP_FileSystem method is basically nothing but an alias for file_get_contents. It makes perfect sense to use $wp_filesystem for everything else, but not $wp_filesystem-> get_contents()... It should not be forced.

[GaryJones]

While file_get_contents() may work in most environments, a plugin that is distributed and used in a Windows environment, or Litespeed server, or some weird file ownership / permissions, or some other exotic setup, may not work. WP_Filesystem supposedly helps cater for these edge cases.

Saying that, for a custom plugin that isn't distributed, where the filesystem is known and managed correctly, using the native PHP would seem more beneficial. Perhaps these checks could be added in, but with a custom property that could turn them off en masse (instead of whitelist flag lots of lines)?

[aristath]

@GaryJones thanks for taking the time to add a detailed explanation 👍

But as far as I know file_get_contents works fine on both Windows and Litespeed... In fact I think (not 1000% sure though) it works everywhere - provided of course we're trying to get a local file and not a URL (windows and litespeed do have issues sometimes with that - but so do "normal" servers if allow_url_fopen is disabled). And $wp_filesystem-> get_contents() is only ever used to get local files so no issues there. So perhaps just checking if file_get_contents is used with a URL instead of path would be preferable (I think I've seen another rule about this somewhere so it should already be implemented if I'm not mistaken).
The only scenario I can think of where file_get_contents won't work is if the user doesn't have read permissions - in which case it should be (and is) respected.

---

Edit: From what I see there were lots of issues in PHP versions 4.x but that's no longer the case in PHP 5.2+

[GaryJones]

Although I've used the WP_Filesystem API, I've not really looked into the internals of too much - but https://www.sitepoint.com/introduction-to-the-wordpress-filesystem-api/ has an explanation of why, as does https://codex.wordpress.org/Filesystem_API - it seems it's more about file ownership from a security perspective, rather than just making the file-writing work.

[aristath]

For writing I agree 100%!!
WP_Filesystem should be enforced.
My objections are for reading...

[jrfnl]

Perhaps these checks could be added in, but with a custom property that could turn them off en masse (instead of whitelist flag lots of lines)?

@GaryJones The current implementation allows for that already through the exclude property. Using that, you can exclude a complete function group from the sniff, in this case the file_system_read function group.
See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#excluding-a-group-of-checks

[JDGrimes]

#42986 brought wp_delete_file() to my attention as well. It was introduced in #17864, and I'm not sure if it is intended for general use or not, but I thought I'd mention it here.

[dd32]

Just a quick note here - Using file_get_contents() and various other methods should be considered OK and using WP_Filesystem should not be a requirement for most things.
The abstraction methods offered are purely designed as wrappers for both "direct" io and FTP io operations during upgrades and the such.

WP_Filesystem should only be required when interactive filesystem operations are occurring where it's desired to automatically switch to using FTP (or SSH) for writes after a UI prompt for credentials.

• For reading files, WP_Filesystem should nearly never be used - exception where files are both being read and written interactively, for example, in Plugin/Theme/Core updates (But even then, there's a good reason to directly read files sometimes).
• For writing files, WP_Filesystem should be utilised where it's a file which the site owner should be able to modify/access/delete through FTP (or other host-access methods). It's intended to automatically switch to using FTP credentials where the filesystem is read-only to the PHP process, as a result of that, it's not viable to use during say a caching plugin, random REST API calls (Unless specifically for updates, or something-else UI related), or when uploading files, etc.

[jrfnl]

Ouch, this PR has been open way too long.

Looking at it now, I believe the following actions are needed:

• Check the current state of WP_File_System and remove any functions from the list which don't have a WP_File_System counterpart.
  If there is no replacement function, the sniff would throw a non-actionable error/warning, which effectively is just noise.
• For functions which do have a WP counter-functions (outside WP_File_System), set those up in separate groups with an informative warning/error message.
• Remove file_get_contents() from the list as it is already checked in another group in the same sniff (with logic to prevent triggering on opening of local files) and we don't want to throw duplicate notices.

As @JDGrimes isn't active anymore in the WPCS sphere, the above action list is up for grabs for anyone who wants to work on it.

[sandeshjangam]

@jrfnl I will take a look and create a PR

[sandeshjangam]

@jrfnl I have updated the latest develop branch and created a PR with required changes - #2108

I am not sure about these 3 functions so kept commented for now. I will update it after your feedback.
'fputcsv', 'fputs', 'ftruncate',