WP_Filesystem_Base::getchmod() reporting permissions incorrectly

[mnelson4]

While doing some unit testing of WP_Filesytem_MockFS, I noticed WP_Filesystem_Base::gethchmod() is reporting permissions on files incorrectly. It uses an undefined method getchmod() to get the file permissions. Most of the children implement it (except WP_Filesystem_MockFS see #29544), and most return a string that represents the octal representation of the file's permissions. However, the results of $this->getchmod($file) need to be an octal number in order to do bitwise comparison, like WP_FIlesystem_Base::gethchmod() tries to do on it.
For that reason I suggest the following patch: use invtval( $this->getchmod($file), 8 ) to make the results of $this->getchmod($file) become an octal number, and define WP_Filesystem_Base::getchmod($file) to clarify its the expected return type.

[mnelson4]

git patch to apply to master

[dd32]

In 32923:

WP Filesystem: Correctly parse the permissions field in the directory listings, and actually set it in FTP environments
Props mnelson4 for initial patch.
Fixes #29548

[dd32]

In 32924:

WP Filesystem: Correctly set the @since for WP_Filesystem_Base::getchmod() added in r32923.
This is 2.5.0 as that's when the function become available in all subclasses.
See #29548

[DrewAPicture]

@dd32: In the scenario where a subclass method is introduced that "extends" a parent class method, I think we'd do better to use an @since for where the subclass method was introduced rather than inherit it. When you think about it in terms of a "changelog" entry, it makes a lot more sense to be able to see when a subclass stopped relying on the parent method to handle that logic.

Edit: grammar

[DrewAPicture]

The question then becomes: Do we "inherit" the @since version for the subclass method from the parent and add a changelog entry to the class DocBlock for the current version mentioning that the subclass method was added, or simply use a current version in the subclass method DocBlock itself?

[DrewAPicture]

What I mean by comment:7 is this:

/**
 * Base WordPress Filesystem class for which Filesystem implementations extend
 *
 * @since 2.5.0
 * @since 4.3.0 The `getchmod()` sub-class method was added to override the parent.
 */
class WP_Filesystem_Base {
...

        /**
         * Gets the permissions of the specified file or filepath in their octal format
         *
         * @since 2.5.0
         *
         * @param string $file
         * @return string the last 3 characters of the octal number
         */
        public function getchmod( $file ) {
                return '777';
        }

vs

/**
 * Base WordPress Filesystem class for which Filesystem implementations extend
 *
 * @since 2.5.0
 */
class WP_Filesystem_Base {
...

        /**
         * Gets the permissions of the specified file or filepath in their octal format
         *
         * @since 2.5.0
         * @since 4.3.0 Introduced in the sub-class to override the parent.
         *
         * @param string $file
         * @return string the last 3 characters of the octal number
         */
        public function getchmod( $file ) {
                return '777';
        }

I think I like the second one better.

[dd32]

Out of those two examples, the second makes more sense.

The main reason it's been done the way it has, is because that's the way it was done in the past for that file, specifically r25560 (Which you added the @since's for)

For changelog purposes, the fact the function now exists on the base class is mostly meaningless, the only reason it does now exist is because there are no docs in the subclasses. For that reason alone I'm mostly against adding any reference to 4.3 there, but I'll defer to you.

[dd32]

@dd32: In the scenario where a subclass method is introduced that "extends" a parent class method, I think we'd do better to use an @since for where the subclass method was introduced rather than inherit it

I agree with that, but just for records sake, the added method to the parent class exists in all the subclasses since 2.5, and no subclass should exist without that method defined ideally.

[DrewAPicture]

Let's leave it as-is for the time being I'll think about it a little bit and probably handle it in #32246.