WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 12 months ago

#29688 new enhancement

Stop editing line in default config not explicit enough

Reported by: kirrus Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 0.71
Component: Bootstrap/Load Keywords: has-patch
Focuses: Cc:
PR Number:

Description

In my experience, new system administrators tend to append to config files, unless said config files have explicit "Don't edit below this line" instruction lines.

The language of the "/* That's all, stop editing! Happy blogging. */" comment doesn't indicate that the functions defined below it are order-critical, in my experience they tend to assume a config file won't have any functional logic in -- most software packages do not put critical logic in config files.

WordPress actually runs the main codebase setup from within the config file, so if defines are put below the wp-settings.php require, they are not used by WordPress at all, effectively discarded. This tends to cause a few hours of head scratching, as to why /wp-admin/ is redirecting to the old domain, when SITEURL define is appended in the config, for example.

Would it be possible to put another comment below the stop editing line, along the lines of
/* Nothing should be defined below this line */

Or anything in plainer language, that makes explicit that Bad Things Will Happen if you put things below the settings require?

Attachments (4)

29688.diff (2.5 KB) - added by diddledan 3 years ago.
needs HEAVY testing. moves the require from wp-config-sample.php into wp-load.php!
29688.1.diff (2.6 KB) - added by diddledan 3 years ago.
re-roll of previous patch with a check to determine whether wp-settings.php has been loaded by wp-config.php
29688.2.diff (3.2 KB) - added by diddledan 3 years ago.
per slack discussion - docs change in sample config file and two additional _doing_it_wrong() calls
29688.3.diff (2.9 KB) - added by diddledan 3 years ago.
cleanup patch with proper indents on new code. Fix previous _doing_it_wrong checks were wrong

Download all attachments as: .zip

Change History (16)

#1 @kirrus
5 years ago

Others have been caught by this before, see ticket https://core.trac.wordpress.org/ticket/17789

Version 0, edited 5 years ago by kirrus (next)

#2 @Bonesnap
5 years ago

I definitely agree. I am a developer and didn't know about this for a long time. Luckily I never had a scenario where I defined something at the bottom of the wp-config.php file, so I never fell prey to it. I only discovered it because I randomly stumbled upon it when searching for something else.

#3 @SergeyBiryukov
5 years ago

  • Component changed from General to Bootstrap/Load

#4 @boonebgorges
5 years ago

  • Version changed from trunk to 0.71

@diddledan
3 years ago

needs HEAVY testing. moves the require from wp-config-sample.php into wp-load.php!

#5 @diddledan
3 years ago

  • Keywords has-patch added

This is an attempt at fixing the problem by a more invasive manner. The wp-config.php file of any currently installed system will require changing to support the different method I've created. This might be possible to change via the automatic updating system, but this will require extensive testing if the patch is accepted. I anticipate the patch to be thrown-out, but wanted to suggest it anyway. :-)

@diddledan
3 years ago

re-roll of previous patch with a check to determine whether wp-settings.php has been loaded by wp-config.php

#6 @diddledan
3 years ago

29688.1.diff allows older, unmodified, wp-config.php files to remain unmodified. I've wrapped the require of wp-settings.php in wp-load.php with a check for the presence of a constant that is only set in wp-settings.php so that if wp-config.php does the require as is normal currently then we do not attempt to do it automatically in wp-load.php. If the require hasn't been executed by wp-config.php then the constant will be undefined and we can continue with the new method.

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


3 years ago

#8 @dd32
3 years ago

I anticipate the patch to be thrown-out, but wanted to suggest it anyway. :-)

:)

The wp-config.php file of any currently installed system will require changing to support the different method I've created.

Unfortunately that's not going to fly, as you know :)

I thought we had a backup require_once() in wp-load.php already incase it was removed from wp-config.php but seems not, I guess the main reason that isn't there is due to the obvious BC issues:
The major back-compat issue here is mostly that bad plugins / other stand alone scripts currently load WordPress by including wp-config.php directly rather than wp-load.php (which is a fairly "recent" thing, in the history of WordPress).

Maybe we could just be more explicit along the lines of:

/*
 * That's all, stop editing! Happy blogging.
 *
 * NOTHING PLACED BELOW THIS LINE WILL HAVE AN EFFECT ON YOUR SITE.
 *
 */

@diddledan
3 years ago

per slack discussion - docs change in sample config file and two additional _doing_it_wrong() calls

#9 @diddledan
3 years ago

try that one on-for-size. I've reverted the original attempt and replaced it with a new attempt which is ONLY docs change and two _doing_it_wrong() calls to coerce plugin devs to stop using wp-load.php and wp-config.php directly via include/require.

@diddledan
3 years ago

cleanup patch with proper indents on new code. Fix previous _doing_it_wrong checks were wrong

#10 @diddledan
3 years ago

  • Keywords has-patch removed

wrong bug.

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

#11 @diddledan
3 years ago

  • Keywords has-patch added
Note: See TracTickets for help on using tickets.