WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 8 years ago

#14928 closed defect (bug) (worksforme)

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

Reported by: hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Inline Docs Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (4)

14928.patch (3.2 KB) - added by hakre 10 years ago.
Updated PHPDOCs, Inline Docs and reviewed the code.
14928.docs-only.patch (1.5 KB) - added by hakre 10 years ago.
PHPDOCs and Inline Docs only
14928.2.patch (3.1 KB) - added by hakre 10 years ago.
Refactoring Iteration
14928.3.patch (3.1 KB) - added by hakre 10 years ago.
Refactoring Iteration, 2nd

Download all attachments as: .zip

Change History (25)

#1 @hakre
10 years ago

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

#2 follow-up: @nacin
10 years ago

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

#3 in reply to: ↑ 2 @hakre
10 years ago

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.

#4 @Denis-de-Bernardy
10 years ago

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.

#5 @filosofo
10 years ago

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."

#6 follow-up: @nacin
10 years ago

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

#7 in reply to: ↑ 6 @Denis-de-Bernardy
10 years ago

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.

#8 follow-up: @hakre
10 years ago

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.

#9 in reply to: ↑ 8 ; follow-up: @Denis-de-Bernardy
10 years ago

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.

#10 @hakre
10 years ago

  • Component changed from General to Inline Docs
  • Keywords needs-patch added
  • Version set to 3.0

@hakre
10 years ago

Updated PHPDOCs, Inline Docs and reviewed the code.

#11 in reply to: ↑ 9 @hakre
10 years ago

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
10 years ago

PHPDOCs and Inline Docs only

#12 @hakre
10 years ago

  • Keywords has-patch added; needs-patch removed

@hakre
10 years ago

Refactoring Iteration

@hakre
10 years ago

Refactoring Iteration, 2nd

#13 @hakre
10 years ago

Related: #14783

#14 @hakre
10 years ago

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

#15 @hakre
10 years ago

Tag: MIGPHP5 - see ref my wordpress PHP5 List

#16 @dd32
10 years ago

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 = '/';

#17 @dd32
10 years ago

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)

#18 @hakre
10 years ago

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.

#19 @dd32
10 years ago

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.

#20 @markjaquith
10 years ago

  • Milestone changed from Awaiting Review to Future Release

#21 @nacin
8 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.