Opened 8 years ago
Closed 7 years ago
#39419 closed task (blessed) (fixed)
Explicitly globalize global variables in ms-settings.php
Reported by: | danielbachhuber | Owned by: | jeremyfelt |
---|---|---|---|
Milestone: | 4.9 | Priority: | normal |
Severity: | normal | Version: | 3.0 |
Component: | Bootstrap/Load | Keywords: | good-first-bug commit has-patch |
Focuses: | multisite | Cc: |
Description
ms-settings.php uses and sets a few implicitly global variables:
- $domain
- $path
- $blog_id
- $site_id
- $public
- $table_prefix
It would be more correct if these were explicitly globalized in the file header.
Discovered in https://github.com/wp-cli/wp-cli/pull/3695
Attachments (3)
Change History (22)
#3
@
8 years ago
Yeah, I'm on board with explicit.
$blog_id
is declared inwp-settings.php
as of [34961], so that should be okay already.- I noted previously that
$site_id
and$public
are no longer used anywhere by core. It'd be great if we could just null these out, but who knows what would break. Note that we kind of accidentally did this with$sites
and it only came up once. See #29415. $path
is used invalidate_another_blog_signup()
, probably by other plugins, etc..$domain
is used inwpmu_validate_blog_signup()
, and possibly elsewhere.$table_prefix
is at least used a few times (object cache drop-ins,wp_set_wpdb_vars()
). It's first used inwp-settings.php
, so should be defined there as well.
#5
@
7 years ago
- Owner set to csloisel
- Status changed from new to assigned
Assigning ownership to mark the good-first-bug
as "claimed".
This ticket was mentioned in Slack in #core-multisite by stevenkword. View the logs.
7 years ago
#7
@
7 years ago
- Keywords needs-docs added
39419.diff applies cleanly against trunk as of July 10, 2017.
PHPdoc descriptions are missing.
@csloisel -- Do you intend on updating the doc to get this one ready for committer's eyes? Do you need any help or have any questions?
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
7 years ago
#10
@
7 years ago
- Milestone changed from Future Release to 4.9
- Owner changed from csloisel to jeremyfelt
- Status changed from assigned to reviewing
#11
@
7 years ago
- Keywords needs-patch added; has-patch needs-testing removed
- Owner changed from jeremyfelt to csloisel
It looks like we can do a bit more with the patch. A couple notes:
- We'll need documentation next to each global that describes its purpose.
$table_prefix
should be documented inwp-settings.php
instead ofms-settings.php
.- We can remove
$site_id
and$public
. See #41285.
@csloisel - if you are interested, please feel free to update the ticket with a new patch. I'm going to reassign the ticket to you.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
7 years ago
#13
@
7 years ago
I started taking a stab at the docs for these in 39419.2.diff. I added Deprecated
next to $domain
, $path
, $site_id
, and $public
to try and indicate that they should not be relied on. I'm not sure if there's a better way to do that.
$table_prefix
is interesting because it's first used in wp-config.php
. We can probably still document it in wp-settings.php
though.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#15
@
7 years ago
- Keywords commit added
- Type changed from defect (bug) to task (blessed)
- Version set to 3.0
39419.2.diff looks OK to me.
I agree with @jeremyfelt's original assessment of $blog_id
and $table_prefix
.
#17
@
7 years ago
39419.3.diff adds usage suggestions for "deprecated" globals.
Agree. We should definitely do this, and it should lead us down the path of education & awareness around their existence and what their intended purposes are.
Multisite has more than a few dedicated globals, and several of them are very situational (sign-ups, site creation, etc...) so I'm sure they've probably created several hours of work for people who needed to discover them the hard way.