WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 10 days ago

#54233 new defect (bug)

Hardcoded wp-content instead of using WP_CONTENT_DIR

Reported by: malthert Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: trunk
Component: General Keywords: has-patch
Focuses: Cc:

Description

Lots of places have incorrectly hardcoded "wp-content" instead of using WP_CONTENT_DIR or

<?php
str_replace( ABSPATH, '', WP_CONTENT_DIR )

e.g. wp-admin/includes/class-wp-debug-data.php uses it correctly like in the above example (the str replace)

This causes updates for installations that use WP_CONTENT_DIR with something else than wp-content to not work correctly, which requires major manual work (to clean up those removed files) and is a major bug in my opinion.
Some files it only affects an error message, but in some files it has a bigger impact.

Files where wp-content is hardcoded, but the str replace should be used instead: (search for wp-content in those files, should be obvious then)
/wp-admin/theme-install.php
/wp-admin/includes/class-wp-site-health.php
/wp-admin/includes/class-wp-site-health-auto-updates.php
/wp-admin/includes/class-core-upgrader.php
/wp-admin/includes/update-core.php (this one is tricky, since the $from new version is wp-includes, but the $to is not, so this must be done dynamically. Also "wp-includes" is hardcoded there where WPINC should be used for $to "wp-includes")
/wp-includes/media.php (this one needs a bit special handling I can provide as snippet if requested)
/wp-includes/ms-functions.php (wp-content is in an array, however the wp-content should be the above str replace)

If there is a way to submit git pull requests, I'd be happy to. I can also just attach the whole, fixed files here if that helps (I just don't know have any idea about SVN)

Change History (9)

#1 @Clorith
2 months ago

Hiya,

You can definitely use GitHub if you prefer, have a look at https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/.

If you wish to use this, you should also consider linking your GitHub profile from your WordPress.org profile, this will help us properly credit any pull requests or patches.

This ticket was mentioned in PR #1744 on WordPress/wordpress-develop by kkmuffme.


8 weeks ago

  • Keywords has-patch added

#3 @malthert
8 weeks ago

Added a pull request.

This will also fix the same issue for hardcoded wp-includes/WPINC

The failed test of phpcs is unrelated to my code I guess, since I didn't change the ordering of any condition nor any -.
I can fix the issues however, if you tell me which files/lines they are (doesn't say that in the failed test of the pull request)

#4 @manfcarlo
7 weeks ago

Thanks for working on this, I'd expect wp-content to be hard-coded in some plugins but I'm shocked that it's still hard-coded in core.

With wp-includes, I thought changing it wasn't supported, and it looks like none of the define statements for WPINC actually check that it isn't already defined, so the hard-coding of that one is maybe less of an issue.

Upon a quick check I see that /wp-admin/load-styles.php also defines WP_CONTENT_DIR without checking if it's already defined. Maybe this is also a defect?

#5 @malthert
7 weeks ago

Upon a quick check I see that /wp-admin/load-styles.php also defines WP_CONTENT_DIR without checking if it's already defined. Maybe this is also a defect?

Yes, this is a bug too. However, this is a major issue that would go beyond the scope of this ticket and should be handled on a separate ticket. (I can create and provide pull request)
The simplest solution for this would be to actually completely remove the ABSPATH and WP_CONTENT_DIR here and require_once( wp-config.php ), so it's in line with WP standards (= user only needs to make changes in wp-config.php to set WP_CONTENT_DIR)

With wp-includes, I thought changing it wasn't supported, and it looks like none of the define statements for WPINC actually check that it isn't already defined, so the hard-coding of that one is maybe less of an issue.

This will actually be my next ticket/pull request once this is merged (bc I don't want to waste time on rebasing). Since it only requires a few lines of code changed in a few files, I think we should give advanced users the possibility to define their own WPINC (e.g. via PHP auto_prepend_file)

#6 @malthert
3 weeks ago

Can someone please take a look at this and merge the PR? This should ship with WP 5.9 please

#7 @malthert
10 days ago

@manfcarlo implemented your suggestion now

Also:

  • fix use of wp-includes instead of WPINC in all files
  • implement phpcs suggestions/make sure all checks pass

#8 follow-up: @SergeyBiryukov
10 days ago

Hi there, thanks for the ticket and the PR!

This has come up a few times before, most recently in #35151 and #47100, which were both closed.

See the explanations here:

To summarize:

  • The primary use of the WP_CONTENT_DIR and WP_CONTENT_URL constants is not to rename the folder, it's to support subdirectory installations with WordPress as an external.
  • The WPINC is not designed to be overridden or set in configurations, and definitely should not be set for the purposes of having a custom includes directory, as that is fundamentally not supported by WordPress.

Related: #23249, #24368, #28264.

Last edited 10 days ago by SergeyBiryukov (previous) (diff)

#9 in reply to: ↑ 8 @malthert
10 days ago

Replying to SergeyBiryukov:

  • The primary use of the WP_CONTENT_DIR and WP_CONTENT_URL constants is not to rename the folder, it's to support subdirectory installations with WordPress as an external.

Exactly, that's why WP_CONTENT_DIR needs to be checked if it already is defined. Since the method of not checking it may not work (e.g. if we have subdirectory/symlinked subdirectory, since DIR will not be the correct path)

  • The WPINC is not designed to be overridden or set in configurations, and definitely should not be set for the purposes of having a custom includes directory, as that is fundamentally not supported by WordPress.

Clear. I will revert the changes where I added a check !defined( 'WPINC' in this pull request.
However still: all places where "wp-includes" is used, WPINC should be used. As right now, it's a wild mix-mash of both (as can be seen in the pull request) This part of the pull request can be merged?

Note: See TracTickets for help on using tickets.