WordPress.org

Make WordPress Core

#37061 closed enhancement (fixed)

Use `get_sites()` in `get_blogs_of_user()`

Reported by: flixos90 Owned by: jeremyfelt
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

Now that we have get_sites() merged (see #35791), we should use this function in get_blogs_of_user(). By adjusting this...

  • the function would only need to find the site IDs of that user and then make a query using get_sites() and the site__in argument (plus additional arguments for non-archived/non-spam/non-deleted if $all is false)
  • the function would then return an array of proper WP_Site objects instead of an array of stdClass

We should probably wait for #36717 and #36935 to be completed and merged first before we start playing around with a patch here. By that...

  • we'd no longer need to use get_blog_details()
  • we could add userblog_id as a magic property in WP_Site that returns $blog_id as an integer

This should keep it backwards-compatible and make the function a lot less cluttered.

Attachments (3)

37061.diff (3.6 KB) - added by flixos90 17 months ago.
37061.2.diff (6.1 KB) - added by flixos90 16 months ago.
37061.3.diff (6.1 KB) - added by flixos90 16 months ago.

Download all attachments as: .zip

Change History (16)

@flixos90
17 months ago

#1 follow-up: @flixos90
17 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7

37061.diff uses get_sites() in get_blogs_of_user() and gets rid of the juggling with raw objects. Since the objects use a property called $userblog_id (?), this property is now added as a magic property in WP_Site to remain compatible. get_blog_details() is no longer necessary either since these properties are lazy loaded automatically now (related: #37102).

I also checked in Core where and how this function is used, and in no case is there any weird array transformation of objects that would cause problems with lazy-loaded properties. :)

A minor issue is that on a non-multisite, the array will still contain a raw object since WP_Site is not available there, so one has to be aware of that. However, I still think we should do this as it's an improvement for Multisite setups.

#2 in reply to: ↑ 1 ; follow-up: @jorbin
17 months ago

Replying to flixos90:

A minor issue is that on a non-multisite, the array will still contain a raw object since WP_Site is not available there, so one has to be aware of that. However, I still think we should do this as it's an improvement for Multisite setups.

I don't think having something different returned under MS from single site is a good decision. Seems like something that could cause plugins to have bugs only under one or the other. Or for code originally written for MS to not work and cause confusion when run on single site installs.

#3 in reply to: ↑ 2 ; follow-up: @flixos90
17 months ago

Replying to jorbin:

I don't think having something different returned under MS from single site is a good decision. Seems like something that could cause plugins to have bugs only under one or the other. Or for code originally written for MS to not work and cause confusion when run on single site installs.

I think the real problem is that we are loading the WP_Site class on Multisite only. I think we should made it compatible with non-multisite installs and load it on every setup which would allow us to fix this issue consistently. After all, every setup has a site - only in a Multisite there is a WP_Network, but I think the WP_Site class can be useful on a regular setup as well. Let's think about this and maybe open a ticket.

#4 in reply to: ↑ 3 @DrewAPicture
17 months ago

Replying to flixos90:

Replying to jorbin:

I don't think having something different returned under MS from single site is a good decision. Seems like something that could cause plugins to have bugs only under one or the other. Or for code originally written for MS to not work and cause confusion when run on single site installs.

I think the real problem is that we are loading the WP_Site class on Multisite only. I think we should made it compatible with non-multisite installs and load it on every setup which would allow us to fix this issue consistently. After all, every setup has a site - only in a Multisite there is a WP_Network, but I think the WP_Site class can be useful on a regular setup as well. Let's think about this and maybe open a ticket.

+1 for exploring global use of WP_Site. Care to open a ticket?

#5 @flixos90
17 months ago

Ticket opened: #37948

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


16 months ago

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


16 months ago

#8 @jeremyfelt
16 months ago

  • Keywords needs-refresh added
  • Owner set to flixos90
  • Status changed from new to assigned

We had a good conversation around this today during multisite office hours. Here's a quick final summary:

Let's avoid adding userblog_id as a magic (deprecated) property to WP_Site and instead stick with returning an array of standard objects populated via WP_Site objects in a similar way to they are now from the DB. This keeps our options open for the future and keeps the changeset clean for both multisite and single site.

For reference, userblog_id has existed since MU:63.

@flixos90
16 months ago

#9 @flixos90
16 months ago

  • Keywords needs-refresh removed
  • Owner changed from flixos90 to jeremyfelt
  • Status changed from assigned to reviewing

37061.2.diff is an updated patch that preserves the original return types (raw objects including the property userblog_id). The function still uses get_sites() as proposed in this ticket.

While I was at it, I adjusted some docs and variable names to properly reflect our current multisite naming conventions. Doing that, I noticed that the docs of the pre_get_blogs_of_user filter state that the returned value should be an array of WP_Site objects which is incorrect, so I changed it to simply "site objects" as well.

#10 follow-up: @jeremyfelt
16 months ago

@flixos90, looking good, thanks!

I would normally want to avoid the churn of blog -> site naming in a change like this, but since we're modifying such a large part of the function and introducing a new loop through $site_ids and $_sites I think we're okay. Just noting. :)

A couple of things:

  • There should be an additional @since 4.7.0 doc rather than using @internal.
  • Two tests are failing for me in current trunk - Tests_AdminBar::test_admin_bar_contains_correct_about_link_for_users_with_no_role_in_multisite and Tests_AdminBar::test_admin_bar_contains_correct_links_for_users_with_no_role_on_network. I have not poked around further than that to see why.

@flixos90
16 months ago

#11 in reply to: ↑ 10 @flixos90
16 months ago

Replying to jeremyfelt:

I would normally want to avoid the churn of blog -> site naming in a change like this, but since we're modifying such a large part of the function and introducing a new loop through $site_ids and $_sites I think we're okay. Just noting. :)

I was hoping you would say that for this case. :) Generally I'm aware of it.

  • There should be an additional @since 4.7.0 doc rather than using @internal.
  • Two tests are failing for me in current trunk - Tests_AdminBar::test_admin_bar_contains_correct_about_link_for_users_with_no_role_in_multisite and Tests_AdminBar::test_admin_bar_contains_correct_links_for_users_with_no_role_on_network. I have not poked around further than that to see why.

37061.3.diff has fixes for these; the latter were caused by missing a ! empty( $site_ids ) check which led to get_sites() returning all sites, which is something I still need to get used to, see #33341 :)

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


16 months ago

#13 @jeremyfelt
16 months ago

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

In 38682:

Multisite: Use get_sites() in get_blogs_of_user().

Previously, we looped through all of the site IDs found via user meta and ran get_blog_details() for each site. Now, we can pass all of the site IDs at once to get_sites() and receive full WP_Site objects.

To avoid possible issues with return type, sites are still processed into a standard object that also has a userblog_id property attached. Nothing is changed for non-multisite configurations.

Props flixos90.
Fixes #37061.

Note: See TracTickets for help on using tickets.