Bug #12017
closedCalls to wp_upload_dir() cause empty directory to be created
0%
Description
Previously: #5964
We continue to have a large number of empty directories in blogs.dir. They are of the form 5 from #5964 - /blogs.dir/{blogid}/files/{year}/{month}.
I just set up a logger to collect stacktraces, and I found that the main culprit appears to be wp_upload_dir(). By default, the second parameter $create_dir is true. So every time a plugin uses wp_upload_dir() to fetch the proper base for an upload path, without specifying that $create_dir should be false, it triggers the creation of the month/day subdirectory on the site in question.
There are a couple of primary culprits:
1. ass_loader() in the network-activated buddypress-group-email-subscription https://github.com/cuny-academic-commons/cac/blob/eaaa6af50fa399026fa8c9cc12c7a2a96ec937e6/wp-content/plugins/buddypress-group-email-subscription/bp-activity-subscription.php#L29
2. Mixin_Mvc_View_Instance_Methods::get_template_override_dir() in nextgen-gallery https://github.com/cuny-academic-commons/cac/blob/eaaa6af50fa399026fa8c9cc12c7a2a96ec937e6/wp-content/plugins/nextgen-gallery/products/photocrati_nextgen/modules/mvc/package.module.mvc.php#L527
3. M_Static_Assets::get_static_override_dir(), also in nextgen-gallery https://github.com/cuny-academic-commons/cac/blob/eaaa6af50fa399026fa8c9cc12c7a2a96ec937e6/wp-content/plugins/nextgen-gallery/products/photocrati_nextgen/modules/static_assets/module.static_assets.php#L102
4. bp_upload_dir() in BuddyPress, which is triggered along a couple different paths. This is mitigated somewhat by the fact that it's usually (always?) run with respect to BP_ROOT_BLOG.
5. Anthologize::__construct() in anthologize https://github.com/cuny-academic-commons/cac/blob/1.15.x/wp-content/plugins/anthologize/anthologize.php#L91
We should clean up existing empty directories and try to prevent future problems of this sort. I'll follow up in a comment with specific thoughts.
Updated by Boone Gorges over 4 years ago
To see empty directories: find wp-content/blogs.dir/ -type d -empty -print. To delete, find wp-content/blogs.dir/ -type d -empty -delete. So this is pretty easy. Ray, do you see any potential problems with doing this? (I don't, but I want to get a second opinion.)
Updated by Boone Gorges over 4 years ago
- Assignee set to Boone Gorges
Regarding 1-5 above.
- 1 and 5 are in plugins that I/we control. It's easy to specify $create_dir=false when calling wp_upload_dir(), and I'm glad to make those changes in the relevant places. But then we need to be sure that there's a check that the upload directory exists before writing to it. In BPGES this is only when writing to the debug log. In Anthologize I think it's a bit more complicated. If it takes a bit longer to solve Anthologize, it's probably OK since it's only activated on a relatively small number of sites.
- 2 and 3 are in nextgen-gallery, and the stacktraces are very long. I guess I'd suggest we live with these for the time being, since they affect only those sites running nextgen-gallery.
- I'm not sure about 4. Feels like it could be a good upstream fix for BP, but then we have to add dir-exists checks in all the places where BP attempts to write. I think this is mainly avatar/cover uploads?
Ray, can you think a little bit about the above and let me know if you see errors in my reasoning? Also, any thoughts you have about 4 would be welcome.
Updated by Raymond Hoh over 4 years ago
Logic looks good, Boone.
About wp_upload_dir() creating the directory, it looks like WordPress thought of the same thing as they introduced a function in WP 4.5 called wp_get_upload_dir() that has $create_dir set to false already. Maybe we can use that function instead?
I tested uploads in BuddyPress by replacing wp_upload_dir() with wp_get_upload_dir() and both avatars and cover images worked flawlessly. When deleting, the avatar routine deletes their own parent directory, which is neat. Cover images do not though, so there's a tiny enhancement we can make in BP there.
One plugin that is missing from your checklist, Boone, is BuddyPress Docs. BP Docs has quite a few calls to wp_upload_dir(), so it might need some more care and attention.
Updated by Boone Gorges over 4 years ago
Good call on wp_get_upload_dir() - thanks for that! I'll also check BuddyPress Docs.
I'll start pushing some fixes to plugins and will post links here.
As for BuddyPress, I think it's worth an enhancement ticket to look at swapping out for wp_get_upload_dir(), but I don't think it's an important culprit here (because it always happens in switch_to_blog()) so I don't think it's urgent for our purposes.
Updated by Boone Gorges over 4 years ago
BPGES fixed in https://github.com/boonebgorges/buddypress-group-email-subscription/pull/185/commits/36a64f31b8cbe5ccf675ba48a0766791cdea6eaf and ported to the Commons in https://github.com/cuny-academic-commons/cac/commit/0974ad521de0da5179da4f4f1211c286650a4290. Deployed as a hotfix. (Didn't use wp_get_upload_dir() here because of WP version compatibility.)
Updated by Boone Gorges over 4 years ago
Fixed in Anthologize in https://github.com/chnm/anthologize/pull/114/commits/9942e4dde032fabfb1fb1582a1a612bd1ab8a3c8. Ported to the Commons in https://github.com/cuny-academic-commons/cac/commit/118ac9630e286a9c9d7b19d2736383a03d88a9ce and deployed as a hotfix.
Updated by Boone Gorges over 4 years ago
Fixed in buddypress-docs in https://github.com/boonebgorges/buddypress-docs/pull/662/commits/59283320e3145b5280e03c615f92b9b819777425. Merged to the Commons in https://github.com/cuny-academic-commons/cac/commit/80dfb601cd504d4a3fa18c1da72cab6de4969721 and deployed to production as a hotfix.
Updated by Boone Gorges over 4 years ago
I've deleted empty directories in uploads and blogs.dir. There were many tens of thousands of them.
Directories are still being generated. In addition to NextGen (which is not strictly a wp_upload_dir() call, but a manual call to wp_mkdir_p()) here are some additional culprits:
- rotatingtweets_enqueue_style()
- wp_privacy_delete_old_export_files() - I guess I'd call this a core bug
- Vc_Base::frontCss()
- events-manager.php has a wp_upload_dir() call
- of_options() in the aware theme
- a call in leaflet-maps-marker/leaflet-maps-marker.php
- a call in download-manager/download-manager.php
- GFFormsModel::get_upload_root()
- GFPDF\Model\Model_Install->create_folder_structures<()/code>
- <code>add_hypothesis()</code>
- <code>academica_get_attachment_id_from_url()</code>
- <code>ReduxFramework::set_redux_content</code>
- wysija-newsletters/core/constants.php
- <code>wptouch_setup_base_content_dir()</code>
- <code>PUM_AssetCache::get_cache_dir</code>
The call in <code>M_Static_Assets::get_static_override_dir</code> (nextgen-gallery) accounts for at least 90% of the calls to wp_mkdir_p() for empty dirs. I guess if we're going to sink resources into trying to mitigate the issue, we might want to look there first.
I'm going to take a bit more time to mull this over before deciding on next steps.
Updated by Boone Gorges over 4 years ago
Looking more closely at the nextgen-gallery things, they're trying to create not a site-specific directory, but one or two subdirectories in the wp-content/ngg/ directory. I don't know the purpose of these directories, but it's harmless to have them, so I've created them manually. This dramatically decreases the amount of noise in my wp_mkdir_p() log :)
Updated by Boone Gorges over 4 years ago
The list of instances is too long to handle on a per-case basis, and I don't see any possible way of using a filter to prevent this behavior (even using something awful like a debug_backtrace() sniffer). So I think the best we can do is probably to have a periodic task that cleans these directories up.
Updated by Boone Gorges over 4 years ago
- Status changed from New to Resolved
I've been in touch with Lihua about a cron job to do a periodic directory cleanup. So I think we are good to go here.