WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 5 weeks 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)

39419.diff (758 bytes) - added by csloisel 6 months ago.
May need phpdoc descriptions
39419.2.diff (920 bytes) - added by jeremyfelt 7 weeks ago.
39419.3.diff (1.2 KB) - added by jeremyfelt 6 weeks ago.

Download all attachments as: .zip

Change History (22)

#1 @flixos90
11 months ago

  • Keywords good-first-bug added

#2 @johnjamesjacoby
11 months ago

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.

#3 @jeremyfelt
11 months ago

Yeah, I'm on board with explicit.

  • $blog_id is declared in wp-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 in validate_another_blog_signup(), probably by other plugins, etc..
  • $domain is used in wpmu_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 in wp-settings.php, so should be defined there as well.

@csloisel
6 months ago

May need phpdoc descriptions

#4 @csloisel
6 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

#5 @DrewAPicture
5 months 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.


4 months ago

#7 @stevenkword
4 months 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?

#8 @earnjam
4 months ago

Created #41285 to have further discussions about removal of $site_id and $public

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


2 months ago

#10 @jeremyfelt
2 months ago

  • Milestone changed from Future Release to 4.9
  • Owner changed from csloisel to jeremyfelt
  • Status changed from assigned to reviewing

#11 @jeremyfelt
2 months 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 in wp-settings.php instead of ms-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.


2 months ago

@jeremyfelt
7 weeks ago

#13 @jeremyfelt
7 weeks 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.


6 weeks ago

#15 @johnjamesjacoby
6 weeks 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.

Last edited 6 weeks ago by johnjamesjacoby (previous) (diff)

#16 @jeremyfelt
6 weeks ago

  • Owner changed from csloisel to jeremyfelt

@jeremyfelt
6 weeks ago

#17 @jeremyfelt
6 weeks ago

39419.3.diff adds usage suggestions for "deprecated" globals.

#18 @jeremyfelt
5 weeks ago

  • Keywords has-patch added; needs-docs needs-patch removed

$table_prefix is already explicitly globalized in wp-settings.php, see [18993]. I'm going to go with 39419.3.diff for 4.9.

#19 @jeremyfelt
5 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41875:

Multisite: Explicitly globalize global variables in ms-settings.php.

Explicitly globalize and document $domain, $path, $site_id, and $public in `ms-settings.php.

Props csloisel, danielbachhuber.
Fixes #39419.

Note: See TracTickets for help on using tickets.