wp_mkdir_p() phpdocs do not explain the "p" in the function name in full

[hakre]

While I stumbled over "wp_mkdir_p()" I wondered what this function name is a code for. WP is for Wordpress mkdir most likely for the mkdir command in unix but P? for what? The "full path" what is a "full path".

Would be great to have some PHPDOC description explaining this cryptic function name.

[hakre]

File is: /wp-includes/functions.php}

[nacin]

I always thought it was because it also attempts to set proper permissions.

[hakre]

Replying to nacin:

I always thought it was because it also attempts to set proper permissions.

Like P = Permission (not Path)?

---

The @return value does not make any sense. On the one hand it says it's true if the path was successfully created, on the other hand it says it's true when it existed. That's contradictory.

[Denis-de-Bernardy]

To me, it's always been p as in mkdir -p... It should return true if the directory exists or was (recursively) created successfully with the proper permissions. My $.02.

[filosofo]

I think Denis's interpretation is correct. The return value behavior matches that of GNU's mkdir with the p flag, which according to the manfile should produce "no error if existing, make parent directories as needed."

[nacin]

Good call Denis. So name and return values are proper. Invalid?

[Denis-de-Bernardy]

Replying to nacin:

Good call Denis. So name and return values are proper. Invalid?

I guess so, if it's supposed to behave like the shell function. As a plugin dev, I'd want that one to return an error code (false in our case, non-zero in the shell) if I can't write to the folder I entered.

I wouldn't close without patching the phpdoc, though.

[hakre]

Thanks for the clarifications so far. That -p switch makes sense.

-p, --parents

for reference: ​http://unixhelp.ed.ac.uk/CGI/man-cgi?mkdir

I suggest to return true when the directory exists afterwards and false if not.

So this needs clarification in the PHPdocs.

Additionally the directory creation / user rights can be checked I smell something but need to find an older ticket first.

[Denis-de-Bernardy]

Replying to hakre:

I suggest to return true when the directory exists afterwards and false if not.

Additionally the directory creation / user rights can be checked I smell something but need to find an older ticket first.

It needs to return true of the folder exists and is writable at the end of the call. On the top of my head that is exactly how it's implemented at the moment.

[hakre]

Updated PHPDOCs, Inline Docs and reviewed the code.

[hakre]

I now provided a patch. It updates the PHPDOC to make the functionality more clear. The "p" in "wp_mkdir_p()" stands for parent directory (not permission).

While checking the functionality of the function to make the docblock more clear I reviewed the code as well. I re-factored the function on the way. Here is my report:

Umask Implications (Informative)

I checked it against implications discussed in #10170 / [11667] (default directory permissions mode). Umask is handeled because chmod() is used after directory creation.

0 Permission Edge-Cases (Improvement)

The function could not properly handle permissions for newly created directories with no parent directory and/or when the acquisition of the parents status failed. This did result in chmod $target to 0 for those edge cases.

I fixed this by having a fall-back to 0755 / FS_CHMOD_DIR (if defined) as we do in class WP_Filesystem_Direct.

Recursion Order (Improvement)

Next to that, the recursion of the function could be improved to take care on the parent prior to create the concrete directory. Currently per each directory name in the full path it is trying to create it first before taking care of the parent. This will always fail if the parent directory does not exists.

I fixed this by reversing the order. Prior to creation of the directory, it's taken care that the parent is handled first if the target has a parent. This is a major improvement and makes the function less complex.

Unknown/Undocumented Normlization (Informative)

Then there is some php.net user contributed note referenced I could not find. I marked that part as undocumented. But I left the functionality unchanged. I do not properly understand the motivation of str_replace() operation fully. It is unable to resolve multitudes (e.g. / or ) anyway. Maybe someone can shed some light in there, I probably might just miss something here.

Missing Writeable Check (Informative)

The function did not (and does not in my patch) check whether or not the directory is writable at the end of the call (see related comment by Denis). It gets chmod'ed to the parent folder's permissions which probably implicates that (as the new folder can be created, the parent folder's permission must allow to so). But it was and is not checked whether that chmod operation is successful.

In the current code a newly created folder might have been set to 0 successfully which is not writeable in some edge-cases.

[hakre]

PHPDOCs and Inline Docs only

[hakre]

Refactoring Iteration

[hakre]

Refactoring Iteration, 2nd

[hakre]

Related: #14783

[hakre]

As one of the Edge-Cases: Probably safe-mode prevents stat() from working (unconfirmed). Related: #13659

[hakre]

Tag: MIGPHP5 - see ref ​my wordpress PHP5 List

[dd32]

attachment 14928.3.patch added

empty( $target ) && $target = '/'; // empty target is root (posix).

Can you please stick to the Coding standards, and avoid using such syntax?

The following syntax would be greatly appreciated in patches.

 if ( empty( $target ) ) // empty target is root (posix).
    $target = '/';

[dd32]

FS_CHMOD_DIR

That shouldn't be used here either, As that's specifically for use with the Filesystem API, The permissions of the folders this creates may be different to that of what the Filesystem API creates as the API operates with higher credentials (ie. FTP/SSH)

[hakre]

Default MKDIR chmod is 0777, so it's the latest and greatest. Let me know if that should replace FS_CHMOD_DIR. I doubt it. Are you okay for 0755? Or what do you mean by higher credentials.

And regarding coding standards, please point me to the documentation that says my code was wrong. There is no rule that you should write and if only if you could.

[dd32]

Default MKDIR chmod is 0777, so it's the latest and greatest. Let me know if that should replace FS_CHMOD_DIR. I doubt it. Are you okay for 0755? Or what do you mean by higher credentials.

FS_CHMOD_DIR is used by the Filesystem API, The Filesystem API is designed to work as the user who owns the files, Ie. "higher credentials than Apache will be running as". wp_mkdir_p() is directly by apache and is not run as the user who owns the files.

And regarding coding standards

I'm not sure why, But the Coding style wiki doesnt refer to 'Common Sense' directly unfortunately. Its an If statement, Use the right tool for the job.