Make WordPress Core

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's profile 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)

#1 @Clorith
3 years 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.


3 years ago
#2

  • Keywords has-patch added

#3 @malthert
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 @manfcarlo
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 @malthert
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 @malthert
3 years ago

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

#7 @malthert
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: @SergeyBiryukov
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:

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 3 years ago by SergeyBiryukov (previous) (diff)

#9 in reply to: ↑ 8 @malthert
3 years 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?

#10 @audrasjb
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: @malthert
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: @SergeyBiryukov
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 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)

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.

#14 @SergeyBiryukov
2 years ago

In 53518:

General: Use WPINC as a shorter reference to wp-includes in some files.

This brings more consistency with similar require_once statements and _deprecated_file() calls in other files.

Follow-up to [601], [628], [36693], [44359], [45663], [45678], [52026],

Props malthert.
See #54233.

#15 in reply to: ↑ 13 @SergeyBiryukov
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 :)

#16 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.1

#17 @SergeyBiryukov
2 years ago

In 53519:

Coding Standards: Fix WPCS issue in [53518].

This fixes an "Concat operator must be surrounded by a single space" error.

See #54233.

#18 @SergeyBiryukov
2 years ago

In 53579:

General: Revert an earlier define of the WPINC constant in src/index.php.

This avoids a Constant WPINC already defined in src/wp-settings.php on line 16 PHP warning, which happens when running a WordPress install out of the src directory after npm run build:dev.

Add a comment to clarify the check for built assets and the direct mention of wp-includes.

Follow-up to [53518].

Props aristath.
See #54233.

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


2 years ago

#20 @audrasjb
2 years ago

  • Milestone changed from 6.1 to Future Release

As per today's bug scrub: since the last attempt of committing this was reverted, and as it didn't get more momemtum during WP 6.1 alpha, let's move it to Future Release.

New patch/ideas are welcome!

Note: See TracTickets for help on using tickets.