WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#18489 reopened enhancement

Create constants in default-constants.php for the uploads folder to allow better custom uploads location

Reported by: eddiemoya Owned by: eddiemoya
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.2.1
Component: Upload Keywords: has-patch close
Focuses: Cc:

Description

There are cases in which a the uploads directory might need to be divorced WP_CONTENT_DIR, currently the only thing we can use is the UPLOADS constant, which works but is relative to ABSPATH and as such limits where the uploads directory can be moved to.

In default-constants.php we have constants for the wp-content, and plugins folder - the uploads folder is a natural addition to this. Currently there is only a poorly documented UPLOADS override in wp_uploads_dir, which can be overridden in wp-config.php. I also think there should be a similar constant for the themes folder, but I would that would be a bit more complex of a change.

I have create a new function in default-constants.php which introduce WP_UPLOADS_DIR and WP_UPLOADS_URL, which are called after wp_plugins_directory_constants() in wp-settings.php - because that function create WP_CONTENT_URL, which is needed in order to create WP_UPLOADS_URL.

It is important to note that I have not changed any of the precedent in terms of what overrides what - the uploads_path option still overrides the default location (or now, the potentially custom location) defined by the new constant, the old UPLOADS constant will still override either of them if it is set. Thats the way it worked before and that behavior has been preserved.

Additionally, I have patched /wp-includes/function.php wp_uploads_dir to make use of these new constants as well as a little clean up of some related logic.

First patch to core - go easy.

Attachments (3)

18489.patch (2.9 KB) - added by eddiemoya 3 years ago.
default-constants.php, wp-settings.php, and /wp-includes/functions.php
18489.php (1.2 KB) - added by nacin 3 years ago.
18489.2.patch (1.7 KB) - added by eddiemoya 3 years ago.
Does not define the option but checks to see if it has been defined.

Download all attachments as: .zip

Change History (26)

eddiemoya3 years ago

default-constants.php, wp-settings.php, and /wp-includes/functions.php

comment:1 eddiemoya3 years ago

  • Owner set to eddiemoya
  • Status changed from new to accepted

comment:2 toscho3 years ago

  • Cc info@… added

comment:3 dd323 years ago

  • Keywords has-patch dev-feedback added

I'm personally leaning away from this, as there already exist filters and options which can be used to direct it elsewhere/force it elsewhere. This differs from the Content and Plugins directories in that they're needed to be known before plugins can be loaded to utilise those filtes..

comment:4 follow-up: GaryJ3 years ago

A further consideration is whether a patch like this would also account for uploads in a multisite setup.

comment:5 in reply to: ↑ 4 ; follow-up: nacin3 years ago

  • Keywords close added; dev-feedback removed

Replying to GaryJ:

A further consideration is whether a patch like this would also account for uploads in a multisite setup.

That was my thought as well. In multisite the upload paths are all different. Mind you, wp_upload_dir() returns an entire array of data, not a single scalar data point. Suggesting wontfix.

comment:6 in reply to: ↑ 5 eddiemoya3 years ago

Replying to nacin:

Replying to GaryJ:

A further consideration is whether a patch like this would also account for uploads in a multisite setup.

That was my thought as well. In multisite the upload paths are all different. Mind you, wp_upload_dir() returns an entire array of data, not a single scalar data point. Suggesting wontfix.

This patch should not interfere with any of that. wp_upload_dir has logic that deals with multisite which I have not modified. All I've done is replace instances of WP_CONTENT_DIR . '/uploads' with WP_UPLOADS_DIR, which itself is a definition of WP_CONTENT_DIR_ . '/uploads'. The same applies to WP_UPLOADS_URL.

None of the behavior of wp_uploads_dir changes here, all that has happened is thats the concatenation of WP_CONTENT_DIR with /uploads happens in default-constants.php rather than being hardcoded within the function itself.

Certainly some testing should verify that this wont effect multisite.

Last edited 3 years ago by eddiemoya (previous) (diff)

comment:7 nacin3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

Thanks for the contribution, but I don't think we wish to go this route.

Closing as wontfix based on a lengthy conversation that can be found in the #wordpress-dev IRC logs.

comment:8 nacin3 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

After further IRC discussion, UPLOADS is a bit deficient in that it must be relative to ABSPATH. Re-opening for further thoughts and consideration.

nacin3 years ago

comment:9 nacin3 years ago

18489.php is something I wrote more than a year ago to address some issues I was having with wp_upload_dir(). Good thing I documented it well.

comment:11 follow-up: Otto423 years ago

The whole wp_uploads_dir is a bit of a mess, and a refactor would certainly be in order, but introducing more constants isn't the right solution. Eliminating the constants entirely would be a better solution. If we could eliminate UPLOADS that would make me much happier. Getting rid of BLOGUPLOADDIR if possible would be nice as well.

Code wanting to modify the paths and such should use the upload_dir filter, or pre_option_upload_path to modify the non-absolute path. Maybe an extra filter or extra args on the existing filter to allow for multisite trickiness is needed.

comment:12 scribu3 years ago

Agree with Otto: death to constants! (or deprecation, rather)

eddiemoya3 years ago

Does not define the option but checks to see if it has been defined.

comment:13 eddiemoya3 years ago

Per my conversation in IRC with Nacin and others...

I've modified the approach by removing the define statements from default-constants.php, and merely checking to see if it exists at all within wp_uploads_dir(). This makes it much less likely that a developer would accidentally use it incorrectly, to get the uploads directory path.

Scribu and Otto: I understand the desire to remove constants - but unless we have plans to change the way we have people move wp-content, this seems like a necessary and very useful evil. If we can move wp-content, it is only natural that we also be able to move uploads. Not to rehash what was talked about in IRC, but my primary point is one of ease-of-use. To have the official, sanctioned, USDA approved method of moving wp-content be so simple, and then to overcomplicate the moving of a directory directly effected by this change, by requiring a plugin is silly.

Im open to other ways of simplifying the wp-content problem.

comment:14 dd323 years ago

To have the official, sanctioned, USDA approved method of moving wp-content be so simple, and then to overcomplicate the moving of a directory directly effected by this change, by requiring a plugin is silly.

You can change the upload path in the Settings for the Uploads, it takes a Folder location and URL, both of which can be relative to wp-content (I believe) OR absolute. Moving the uploads directory is probably the one with the lowest barrier to entry..

comment:15 eddiemoya3 years ago

dd32: This would make perfect sense if all of the options had the same solution - but they dont. For wp-content and plugins there is wp-config, but for this one option theres a manual field to enter - which puts into play issues of access and permission where they dont exist for wp-config.

What im trying to achieve here is no specifically to create any particular constant in any particular way - what we need is a unified complete solution for managing wp-content, and that must include the ability manipulate the uploads folder - not a mixed bag of several partial solution.

After thinking about what Otto said, I am trying to think through a possible that we might solve this issue while perhaps beginning to pave a path for eventual deprecation of some of the constants. I don't care about the constants specifically.

Version 0, edited 3 years ago by eddiemoya (next)

comment:16 shawnkhall3 years ago

  • Cc shawnkhall added

Is there a reason why it can't use filters similar to this? (for ms-default-constants.php):

define( 'UPLOADBLOGSDIR', apply_filters( 'define_uploadblogsdir', 'wp-content/blogs.dir' ) );

define( 'UPLOADS', apply_filters( 'define_uploads', UPLOADBLOGSDIR . "/{$wpdb->blogid}/files/" ) );

define( 'BLOGUPLOADDIR', apply_filters( 'define_bloguploaddir', WP_CONTENT_DIR . "/blogs.dir/{$wpdb->blogid}/files/" ) );

This method is used in the twenty eleven theme.

comment:17 scribu3 years ago

Because apply_filters() is defined after wp-config.php is loaded.

comment:18 in reply to: ↑ 11 mikeschinkel3 years ago

Replying to Otto42:

Eliminating the constants entirely would be a better solution.

+1

comment:19 shawnkhall3 years ago

@scribu, I don't see why that matters? apply_filters is loaded via WPINC/plugin.php in wp-settings.php:72, the database is loaded at wp-settings.php:75, and the multisite constants are loaded in wp-settings.php:90.

I understand that means absolutely nothing as far as wp-config.php, but that file is not the only place configuration can occur.

For those of us using systems of scale, we rely heavily on database plugins (I personally use Multi-DB), and the sequence above would enable the specific filters above to be effected within WP_CONTENT_DIR/db.php

Adding these filters within the ms_upload_constants function (ms-default-constants.php) would add almost no overhead, but would allow systems of scale to address the problems within the otherwise static constants.

comment:20 SergeyBiryukov3 years ago

  • Milestone set to Awaiting Review

comment:21 smhmic13 months ago

  • Cc smhmic added

Eliminating certain constants may avoid some developer confusion when manipulating upload locations. But without the constants added in this patch, there is a substantial short-circuit in wp-config's ability to customize the install's file structure. It can move plugins, themes, and all of entire wp-content; uploads is the black sheep, and is a source of confusion that will not be remedied by removing the other constants. So, I'd say the motion to add this has a perfectly sensible reasoning.

Sensible, and useful. @dd32, it is true that the uploads directory "differs from the Content and Plugins directories in that they're needed to be known before plugins can be loaded" -- at which point filters can be used to change the upload locations as desired. But why must a developer manage the uploads location differently when it can be handled the same way as themes and plugins, neatly, in wp-config?

comment:22 creativeinfusion11 months ago

  • Cc wptrac@… added

Using a constant to do this will suffer from the same problem limitation as WP_CONTENT_URL, WP_PLUGINS_URL and WPMU_PLUGINS_URL. Namely for a subsite in a multisite subdirectory install where WordPress has its own directory - it won't be possible to reliably set WP_UPLOADS_URL in wp-config.php to http://domain.com/any-user-subsite/somewhere/custom-upload and know that any-user-subsite really is a valid subsite, i.e. if the page loaded is http://domain.com/a-page/a-child-page/ the upload url wanted would be http://domain.com/somewhere/custom-upload, but if the page loaded is http://domain.com/any-user-subsite/a-page/ the upload url wanted would be http://domain.com/any-user-subsite/somewhere/custom-upload

UPLOADS as it is doesn't cut it, and the per site options aren't appropriate for multisite users; but config constants generally are pain in the neck - they should never be directly used by developers but are. There's various functions like contents_url, plugins_url and wp_upload_dir that give a lot more flexibility as they're filtered, but anything that uses constants directly just bypasses any filters applied.

I actually use the same idea as WP_UPLOAD_xxx at present together with filters on all the above functions to inject the user subsites into the config URLs, but I'd much rather see constants replaced with something that can't be accessed directly and that allows filters to be applied to config URLs in particular.

comment:23 nacin3 months ago

  • Component changed from General to Upload
Note: See TracTickets for help on using tickets.