Opened 3 years ago
Last modified 2 years ago
#54233 new defect (bug)
Hardcoded wp-content instead of using WP_CONTENT_DIR
Reported by: | malthert | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | major | Version: | |
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 (20)
This ticket was mentioned in PR #1744 on WordPress/wordpress-develop by kkmuffme.
3 years ago
#2
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/54233
#3
@
3 years 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
@
3 years 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
@
3 years 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
@
3 years ago
Can someone please take a look at this and merge the PR? This should ship with WP 5.9 please
#7
@
3 years 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:
↓ 9
@
3 years 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:
- comment:3:ticket:23249
- comment:3:ticket:24368
- comment:3:ticket:35151
- comment:7:ticket:35151
- comment:1:ticket:47100
To summarize:
- The primary use of the
WP_CONTENT_DIR
andWP_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.
#9
in reply to:
↑ 8
@
3 years ago
Replying to SergeyBiryukov:
- The primary use of the
WP_CONTENT_DIR
andWP_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?
#10
@
3 years ago
- Version trunk deleted
Since most of the change were not introduced in WP 5.9 development cycle, I'm removing the trunk
version from this ticket :)
#11
follow-up:
↓ 13
@
3 years ago
@SergeyBiryukov could I get a quick reply on my last comment, so I can update the PR and we get this merged for 6.0?
Basically I just need to remove the !defined check for WPINC and then this should be good to merge.
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
2 years ago
#13
in reply to:
↑ 11
;
follow-up:
↓ 15
@
2 years ago
Replying to malthert:
Basically I just need to remove the !defined check for WPINC and then this should be good to merge.
I would say let's keep this ticket for WP_CONTENT_DIR
only, that way it should be easier to review.
Replying to SergeyBiryukov:
The primary use of the
WP_CONTENT_DIR
andWP_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)
Looking at the PR, it seems that most of these instances should still work with symlinks, as long as the wp-content
part of the path is not renamed. __DIR__
is only used to define ABSPATH
, and WP_CONTENT_DIR
can then be used to replace ABSPATH
with something else. So I'm a bit confused about the necessity of these changes, is it to also account for the wp-content
part being renamed? I'm not really against that, just want to be clear about the use case.
#15
in reply to:
↑ 13
@
2 years ago
Replying to SergeyBiryukov:
I would say let's keep this ticket for
WP_CONTENT_DIR
only, that way it should be easier to review.
I believe [53518] addresses most of the WPINC
changes, so that we could focus on WP_CONTENT_DIR
:)
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.