Make WordPress Core

Opened 6 years ago

Last modified 3 months ago

#44083 reviewing enhancement

Add action to wp_mkdir_p() when directory is created successfully

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 2.0.1
Component: Filesystem API Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description (last modified by johnjamesjacoby)

It would be nice if the wp_mkdir_p() function contained a hook for plugins to interact with when a directory gets created.

The use-case I have currently is that Easy Digital Downloads (for example) creates .htaccess files and empty index.php files in its own uploads/edd directory, and each directory inside it, as one way of protecting those empty directories from being publicly browsable.

Because there is no action hook here, EDD uses a daily transient, which leaves new directories potentially open until the transient expires.

An action hook on directory creation would allow for EDD to create those files immediately.

Attachments (6)

44083.1.diff (573 bytes) - added by nx2017 6 years ago.
Before and after actions for direction creation has been implemented.
44083.2.diff (930 bytes) - added by nx2017 6 years ago.
Before and after actions for directory creation has been implemented. Target attribute is set with documentation.
44083.3.diff (871 bytes) - added by seusmaniqbal 6 years ago.
44083.diff (1.0 KB) - added by kkarpieszuk 6 years ago.
Changes suggested by @desrosj
44083.4.diff (2.7 KB) - added by donmhico 5 years ago.
Added unit tests.
44083.5.diff (3.0 KB) - added by donmhico 5 years ago.
Correct @since to adhere to WPCS.

Download all attachments as: .zip

Change History (35)

#1 @johnjamesjacoby
6 years ago

  • Description modified (diff)

@nx2017
6 years ago

Before and after actions for direction creation has been implemented.

@nx2017
6 years ago

Before and after actions for directory creation has been implemented. Target attribute is set with documentation.

@seusmaniqbal
6 years ago

#2 @seusmaniqbal
6 years ago

  • Keywords has-patch added; needs-patch removed

Following concerns about the above patch:

  • Remove extra line from the inline documentation.
  • As for the hook, I think it should be more specific with its name like before_directory_creation.

Thanks

This ticket was mentioned in Slack in #core by abdullahramzan. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by seusmaniqbal. View the logs.


6 years ago

#5 @desrosj
6 years ago

  • Keywords needs-refresh added

@seusmaniqbal this is looking good. Handful of small things.

  • Can we tweak the action names a bit more. How about before_create_directory / after_create_directory.
  • The @since docblock tags don't need a description when it indicates the function is being introduced. Only when changes are made do these need notes.
  • Can we change the doclbock descriptions a bit? Thinking "Fires before the directory is created." and "Fires after the directory is created and the permissions are set." sound a bit better.
  • For the after action, the $target parameter should not say attempt because it only fires if the directory is created. How about "Full path to the created directory."?
  • Please add spaces between the opening and closing parentheses and the contents of the actions to adhere to the coding standards.
  • I think we could include the $dir_permissions as a second optional filter parameter. Might be useful for plugins to be able to match permissions or perform actions only when the directory has a certain permission level.
  • Can we move the after action below the if() statement? This way, if the permissions need to be reset because of the presence of a umask, the action can reasonably expect the permissions on the directory to be correct.
Last edited 6 years ago by desrosj (previous) (diff)

#6 @seusmaniqbal
6 years ago

@desrosj Thank you for feedback. I will update new patch soon.

@kkarpieszuk
6 years ago

Changes suggested by @desrosj

#7 @kkarpieszuk
6 years ago

Hello

I am trying to contribute to the WordPress first time and decided to help here a bit :) I have attached diff with all suggestions from @desrosj applied, I hope I helped

Additionally, I updated @since to 5.0 (the same which I found in wp-includes/version.php)

Last edited 6 years ago by kkarpieszuk (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


5 years ago

#9 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

#11 @SergeyBiryukov
5 years ago

  • Milestone changed from 5.2 to 5.3

Missed the 5.2 Beta 1 deadline, moving to 5.3.

@donmhico
5 years ago

Added unit tests.

#12 @donmhico
5 years ago

  • Keywords needs-refresh removed

@donmhico
5 years ago

Correct @since to adhere to WPCS.

#13 @donmhico
5 years ago

  • Keywords has-unit-tests added

#14 @davidbaumwald
5 years ago

@SergeyBiryukov Can this be realistically included by tomorrow for 5.3 Beta 1? If not, we can punt to 5.4.

#15 @davidbaumwald
5 years ago

  • Milestone changed from 5.3 to Future Release

With 5.3 Beta 1 shipping today, I’m punting this Future Release. If this can be committed to for 5.4, feel free to move to that milestone.

This ticket was mentioned in PR #4927 on WordPress/wordpress-develop by @benniledl.


10 months ago
#16

Add 'before_create_directory' and 'after_create_directory' hooks + automated coding standard fix

Trac ticket: https://core.trac.wordpress.org/ticket/44083

#17 @benniledl
10 months ago

  • Keywords dev-feedback needs-unit-tests added; has-unit-tests removed

Updated the patch for 6.4

#18 @costdev
10 months ago

  • Keywords has-unit-tests commit added; dev-feedback needs-unit-tests removed

Both myself and @mukesh27 have reviewed PR 4927 (@benniledl's first code contribution! 🎉) which now also includes PHPUnit Tests covering the changes.

This one seems ready for commit consideration. @SergeyBiryukov do you have time to review and possibly commit the PR? I'll leave you as ticket owner to decide whether this ticket should be milestoned for 6.4. 🙂

@benniledl commented on PR #4927:


10 months ago
#19

thanks for the adjustments @costdev !

This ticket was mentioned in Slack in #core by costdev. View the logs.


10 months ago

#21 @Clorith
10 months ago

Having had a look at PR 4927, it's looking pretty good, but I am wondering if perhaps another enhancement should be included before this is committed, to cover failure scenarios as well.

Currently, there is a proposed before and after hook, but the after-hook only fires if the directory is created. Should there also be a hook for when the creation fails?

#22 @costdev
10 months ago

That would make sense, something like create_directory_failed and pass $target and $dir_perms like the others?

#23 @costdev
9 months ago

  • Keywords commit removed

@Clorith PR 4927 has been updated to add a create_directory_failed action that passes $target and $dir_perms, with an accompanying test. When you get a chance, could you give it another review? 🙂


Removing commit until we have approval for the additional action.

This ticket was mentioned in PR #5766 on WordPress/wordpress-develop by @benniledl.


5 months ago
#24

Added before_create_directory and after_create_directory hooks.

Trac ticket: [](https://core.trac.wordpress.org/ticket/44083)

@costdev commented on PR #5766:


4 months ago
#25

@Clorith Would you be free to take a quick look at this PR and see if you're happy that the new hooks cover everything?

This ticket was mentioned in Slack in #core by benniledl. View the logs.


4 months ago

#27 @hellofromTonya
3 months ago

@costdev pinged me for review.

The original proposal was to add one action hook to wp_mkdir_p() to fire "when directory is created successfully". This is accomplished with 'after_create_directory'.

I understand the need for adding another for when it fails: 'create_directory_failed'.

But why is before_create_directory needed? What benefits does it provide?

Performance:
There will be a performance hit with firing 3 new hooks and processing all of the hooked callbacks. Are there any performance concerns? I assume no and that the performance hit is acceptable given this function's recursive tasks and the frequency a new directory is created (likely infrequently). Noting performance here as a consideration, i.e. just in case there may be a concern I'm not aware of.

#28 @costdev
3 months ago

But why is before_create_directory needed? What benefits does it provide?

Good question @hellofromTonya!

I think the before_create_directory hook was likely added for consistency in wrapping the logic in before_* and after_* actions, similar to pre_something and something hooks used in Core.

@desrosj may have some thoughts on the reasoning, maybe?

@hellofromTonya commented on PR #5766:


3 months ago
#29

The patch includes all of the feedback raised by @desrosj (see here) and @Clorith (see here) ✅

LGTM _except_ for the open question I raised about why the 'before_create_directory' action is needed.

Note: See TracTickets for help on using tickets.