Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#41936 closed defect (bug) (fixed)

get_main_site_id ignores custom values of $current_site->blog_id

Reported by: spacedmonkey's profile spacedmonkey Owned by: flixos90's profile flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Networks and Sites Keywords: has-unit-tests has-patch commit
Focuses: multisite, performance Cc:

Description

The function get_main_site_id was added in #29684 . This new function adds a way to get the main site id of the current network. This works on site id of the site with the same domain / path of the network. However, before the point of truth main site id was property on $current_site. Some domain mapping plugins build the $current_site and $current_blog manually. Build these globally manually means that the $current_site->blog_id will be set. Currently get_main_site_id ignores these values.

Attachments (8)

41936-test.diff (561 bytes) - added by spacedmonkey 7 years ago.
41936.diff (5.8 KB) - added by spacedmonkey 7 years ago.
41936.2.diff (6.0 KB) - added by spacedmonkey 6 years ago.
41936.3.diff (6.0 KB) - added by spacedmonkey 6 years ago.
41936.4.diff (6.0 KB) - added by spacedmonkey 6 years ago.
41936.5.diff (5.8 KB) - added by flixos90 6 years ago.
41936.6.diff (5.8 KB) - added by flixos90 6 years ago.
41936.7.diff (6.2 KB) - added by jeremyfelt 6 years ago.

Download all attachments as: .zip

Change History (21)

@spacedmonkey
7 years ago

#1 @spacedmonkey
7 years ago

41936-test.diff
This is a test if blog_id is set from another source, such as a sunrise file. Test is currently failing.

41936.diff
This is my solution, the moves the logic of getting blog_id into the WP_Network. It also makes the get_main_site_id method public, so it can be called in the bootstrap process.

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


7 years ago

#3 @flixos90
7 years ago

  • Milestone changed from Awaiting Review to 4.9

I agree this can become a bug in some cases, thus should be fixed in 4.9.

41936.diff fixes it, but I still don't like the logic being part of the WP_Network method, as this is not a common practice. It may very well be the only solution to accomplish this, but I wanna try to think further about another way. This can go in post-beta, so we don't need to rush.

#4 @flixos90
7 years ago

  • Owner set to flixos90
  • Status changed from new to assigned

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


6 years ago

@spacedmonkey
6 years ago

@spacedmonkey
6 years ago

@spacedmonkey
6 years ago

#6 @spacedmonkey
6 years ago

41936.4.diff

Changes

  • Magic method returns int
  • blog_id is now cast to string
  • get_main_site_id now returns get_current_blog_id
  • Don't return early, on pre_get_main_site_id filter, check if not empty, to make sure it value is not a null value.

@flixos90
6 years ago

#7 @flixos90
6 years ago

41936.5.diff is a minor update:

  • Ensure that the $blog_id property is always a string and that the get_main_site_id() method always returns an int.
  • Handle the filter separately, i.e. have it not set the $blog_id property.
  • Use an internal variable for the cache key instead of "hard-coding" it twice.
  • Keep the return 1 if not multisite in the get_main_site_id() function for now, as this should be a separate commit.
  • Add two extra unit tests, one to ensure the property value is honored correctly, and another to ensure the filter still takes precedence.

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


6 years ago

@flixos90
6 years ago

#9 @flixos90
6 years ago

41936.6.diff adjusts the following:

  • Fixes access of not existing $network variable by using $this instead.
  • Only returns value of pre-filter or property if it is positive.
  • Adjusts documentation of the pre-filter accordingly.

@jeremyfelt
6 years ago

#10 @jeremyfelt
6 years ago

  • Keywords commit added

41936.6.diff looks good! I made one small change to the tests in 41936.7.diff and removed the pre filter each time it was used to avoid possible cross-test pollution.

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


6 years ago

#12 @flixos90
6 years ago

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

In 41861:

Multisite: Take WP_Network::$blog_id into account in get_main_site_id().

When the WP_Network::$blog_id property is set manually, for example in the multisite bootstrap process, get_main_site_id() should use that value instead of running its own logic. The main logic for the function was therefore moved into the internal WP_Network::get_main_site_id() method, which is now being accessed by the function through the magic property handling for WP_Network::$blog_id (and its equivalent WP_Network::$site_id).

Props spacedmonkey, jeremyfelt.
Fixes #41936.

#13 @flixos90
6 years ago

In 41862:

Multisite: Return get_current_blog_id() value instead of hard-coded 1 in get_main_site_id() for non-multisite environments.

See #41936.

Note: See TracTickets for help on using tickets.