WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#29544 new enhancement

WP_Filesystem_MockFS permissions support

Reported by: mnelson4 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

At Event Espresso, we are starting to use WP_Filesystem_MockFS to unit test some of our filesystem-related code. One of the things our code was checking for was the permissions on a file, but currently that isn't supported by WP_Filesystem_MocksFS. Actually, when WP_Filesystem_MocksFS::gethchmod() (please note the "h" between "get" and "chmod") is called, a method-not-declare fatal error is thrown, because WP_Filesystem_Basegethchmod() calls the undefined method getchmod() (note the absense of the "h" between "get" and "chmod"), which all the other children of WP_Filesystem_Base declare, except WP_Filesystem_MocksFS. I have a patch for this I will try to submit

Attachments (3)

0001-added-permissions-to-mock-filesystem-and-unit-tests-.patch (8.5 KB) - added by mnelson4 4 years ago.
git patch to apply to master
0001-added-permissions-to-mock-file-system-and-is_readabl.patch (15.8 KB) - added by mnelson4 4 years ago.
a different git patch that also adds is_readable() and is_writable() to mock filesystem
0001-3rd-patch.-Includes-some-more-bugfixes.patch (17.3 KB) - added by mnelson4 4 years ago.
3rd patch

Download all attachments as: .zip

Change History (16)

#1 @mnelson4
4 years ago

  • Keywords has-patch added

#2 @nacin
4 years ago

  • Component changed from General to Build/Test Tools
  • Milestone changed from Awaiting Review to 4.1

#3 @mnelson4
4 years ago

sorry, actually I noticed there are more methods that need to be implemented for this to be useful (eg is_readable() and is_writable()). I'll make a new patch asap

@mnelson4
4 years ago

a different git patch that also adds is_readable() and is_writable() to mock filesystem

#4 @mnelson4
4 years ago

so please ignore my first patch "0001-added-permissions-to-mock-filesystem-and-unit-tests-.patch" (I'd delete it but I don't see an option to do that), and instead use the 2nd patch "0001-added-permissions-to-mock-file-system-and-is_readabl.patch"

#5 @mnelson4
4 years ago

found a couple bugs in my 2nd patch so am uploading another

This ticket was mentioned in IRC in #wordpress-dev by jorbin. View the logs.


4 years ago

#7 @dd32
4 years ago

It makes sense to expand the coverage of the MockFS classes, originally I implemented the bare basics so that it was possible to test the feature I was looking at (which required nothing more than directory transversal).

It might be a good idea to look at alternative Virtual Filesystems which exist in the PHP land before investing too heavily here, for example:

If we can leverage one of these existing projects (and contribute back any features we need) I think we'll be much better off than just implementing a bunch of extra features ourselves.

Most of these projects seem to be using extra PHP streams functionality in PHP 5.4, I don't think that's a blocker to using them though.

Does anyone else have any thoughts here?

#8 @mnelson4
4 years ago

Yeah I think using a pre-built virtual filesystem would probably be smart, although we'll still want to have an adapter which would be your WP_Filesystem_MockFS. So we could change the implementation of WP_Filesystem_MockFS to use some other library at some future date, but for now just apply this patch.

But if anyone else finds a moment to rewrite the WP_Filesystem_MockFS to use some other mock filesystem library that would be great. For now, for my project, I just wanted the features I added in the above patch.

#9 @dd32
4 years ago

  • Milestone changed from 4.1 to Future Release

For now I'm punting this to the next release pending a better investigation of the virtual filesystems we could leverage instead.

Since WordPress core doesn't require these for it's unit testing, I'm not sure we should spend any time on these until we know the direction going forward.

If someone would like to take a look at the filesystems I mentioned, or other ones you find and report back with what you think will work best for us (or you know, a patch!), it'd be appreciated by all :)

#10 @johnbillion
4 years ago

  • Version changed from trunk to 3.7

#11 @mnelson4
3 years ago

hey @dd32 I think the idea of using a library for the filesystem is good, but I personally haven't had the time to look into it and won't for the forseeable future. Next time I work on something filesystem related I'll look into it; but for now I'd be ok with just invalidating this ticket in order to clean up trac a little (or we can keep it just for reference if you prefer).

#12 @mnelson4
3 years ago

FYI I just stumbled on this today: https://github.com/JDGrimes/wp-filesystem-mock Apparently jdgrimes tried using vfs stream but gave up: http://codesymphony.co/phpunit-and-the-wordpress-file-system-api/

Last edited 3 years ago by mnelson4 (previous) (diff)

#13 @jdgrimes
3 years ago

Thanks for alerting me to this ticket @mnelson4. I'm not sure that I even knew that WP_Filesystem_MockFS existed when I created my version of this.

The reason that I didn't attempt to use vfsStream was that I actually didn't care about ownership/permission stuff, and handling users and groups in vfsStream seemed complicated. I think vfsStream also had some limitations in regard to the number of users and groups as well.

So, currently https://github.com/JDGrimes/wp-filesystem-mock doesn't bother with ownership at all, but support could probably be added easily.

I don't know what all of the pros and cons are of using this approach as opposed to VFS, but obviously my approach will only work when $wp_filesystem is used—any direct use of functions like file_exists() that would normally work when using VFS wouldn't work (since there is no virtual filesystem, the "filesystem" is just a bunch of nodes in an array). (vfsStream actually also has the same issue with some functions.)

Note: See TracTickets for help on using tickets.