WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 3 days ago

#40180 assigned enhancement

Introduce `get_site_by()` function for multisite

Reported by: flixos90 Owned by: flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch dev-feedback has-unit-tests ms-roadmap
Focuses: multisite Cc:

Description

As @stephdau mentioned on #40064, the new get_site() function from 4.6 is not a direct replacement for get_blog_details() which we are planning to deprecate.

Therefore I think it makes sense to introduce a function get_site_by() that is basically a wrapper for a WP_Site_Query (with LIMIT 1), and then runs array_shift() to only return the first result. This function would then become the direct replacement that people who previously relied on get_blog_details() with something other than an ID could use.

Parameter-wise, we should probably adjust it a little bit to match things like get_term_by(), which also makes the way that function works more explicit.

Attachments (3)

40180.diff (9.8 KB) - added by flixos90 6 months ago.
40180.2.diff (3.5 KB) - added by flixos90 6 weeks ago.
40180.3.diff (8.6 KB) - added by flixos90 6 weeks ago.

Download all attachments as: .zip

Change History (24)

#1 @stephdau
6 months ago

I'm liking the get_site_by() idea, from a processing standpoint.

I'm still not super fond of the idea of deprecating get_blog_details(), primarily because of the amount of person-hours that 3rd-parties will end up spending on moving to the new function, and users upgrading plugins/etc because of it (EG: I have 559 instances of the use of get_blog_details() outside of Core in the codebase I work on). But at least having a direct replacement means devs have more options (regex replacements, etc).

Keeping the full functionality around is ultimately what matters, so it's still a more viable option than the previous plan.

Thanks, @flixos90.

Last edited 6 months ago by stephdau (previous) (diff)

#2 @stephdau
6 months ago

There is one thing we'll need to contend with, for consistency, with other get_*_by() instances: get_user_by(), get_term_by(), etc all assume that the 1st parameter is a single $field, such as login (user) or slug (term).

In the case of get_blog_details(), that same equivalent parameter can accept an array of $fields, such as array( 'domain' => 'blah.com', 'path' => '/' );

#3 @stephdau
6 months ago

This will also have to be considered:

foreach ( get_site( 52723464 ) as $key => $value ) { echo "$key, "; }
blog_id, domain, path, site_id, registered, last_updated, public, archived, mature, spam, deleted, lang_id, ds_blog, ds_stats, 

foreach ( get_blog_details( 52723464 ) as $key => $value ) { echo "$key, "; }
blog_id, domain, path, site_id, registered, last_updated, public, archived, mature, spam, deleted, lang_id, ds_blog, ds_stats, blogname, siteurl, post_count, home, 

#4 @flixos90
6 months ago

@stephdau We can likely handle the parameters in the same way as a function like add_query_arg() works: You can either pass a key and a value as two separate parameters, or an array of key => value pairs (in that case the second parameter would be ignored). This makes the function flexible enough for these use-cases.

Thanks for bringing up the object iteration as well.

#5 @flixos90
6 months ago

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

#6 @flixos90
6 months ago

In 40317:

Multisite: Add further unit tests for get_blog_details().

These tests verify that the returned site object is iterable and contains the expected properties.

See #40228, #40180.

@flixos90
6 months ago

#7 @flixos90
6 months ago

  • Keywords has-patch dev-feedback has-unit-tests added; 2nd-opinion removed

40180.diff is a possible implementation for get_site_by(), including unit tests. It accepts either a $field and its $value as parameters or a single array of fields and their values.

I decided to not make the function a one-to-one replacement for get_blog_details() as we're probably not going to deprecate that function. However, get_site_by() supports everything that get_blog_details() needs and does that in a much more efficient way, so I suggest we should use the new get_site_by() in get_blog_details() - all the latter needs to do then is tweak how the parameters are passed to it and modify the return value appropriately.

#8 @flixos90
6 months ago

I just added a patch on #40228 to demonstrate how this function could be used to improve get_blog_details() as well. My comment https://core.trac.wordpress.org/ticket/40228#comment:8 also goes a bit more into detail why I think that modifying get_blog_details() is not sufficient here and why it's worth introducing get_site_by().

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


5 months ago

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


5 months ago

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


5 months ago

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


5 months ago

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


5 months ago

#14 @flixos90
5 months ago

  • Milestone changed from 4.8 to Future Release

This needs more discussion and review. If we introduce such a function, it makes sense to bring it in alongside enhancements like #40364.

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


2 months ago

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


6 weeks ago

#17 @flixos90
6 weeks ago

  • Keywords ms-roadmap added
  • Milestone changed from Future Release to 4.9

Per today's office hours, it was decided to introduce the new function and use it to improve get_blog_details().

Let's think about possibly finding a more suitable name. The function is not entirely equivalent to something like get_term_by(), since it also supports an array, so a different kind of name might make sense.

@flixos90
6 weeks ago

#18 @flixos90
6 weeks ago

40180.2.diff makes a few improvements:

  • It adds support for getting a site by only its path, if the current setup is a subdirectory install. Example: get_site_by( 'path', '/foo/' )
  • It introduces a new idea of simply passing a URL (domain-path combination), which is a lot quicker than passing an array of domain and path. Example: get_site_by( 'url', 'mysite.com/foo/' )
  • An additional tweak in the beginning of the functions ensures that get_site() is called as an easy shortcut when only an ID is passed.
  • Further unit tests have been added/adjusted.

I'm starting to like the url argument. Actually, that argument would allow us to completely get rid of the requirement of supporting an array, which would make the function much simpler. The only thing that people might wanna pass in addition is the network ID, but since that only applies to multi-network installs, it could as well be an extra third parameter, as it is in several other core functions. I will create another patch with that proposal.

@flixos90
6 weeks ago

#19 @flixos90
6 weeks ago

40180.3.diff implements a more straightforward version of get_site_by(), where it only accepts a single parameter (as mentioned in https://core.trac.wordpress.org/ticket/40180#comment:18). With this kind of functionality, it actually works similar like get_term_by(), thus the name now makes sense.

The idea is that the new url field is a combination of domain and path and thus covers that use-case in a simple way. Unit tests for the function are included.

I like this approach far better, and it clears up the naming issues we discussed on Tuesday. Furthermore get_site_by( 'url', 'mysite.com/foo/' ) is simpler than get_site_by( array( 'domain' => 'mysite.com', 'path' => '/foo/' ) ).

#20 @spacedmonkey
6 weeks ago

I really like 40180.3.diff. My problem with this function as been passing an array to it. It makes this function stick out compared to get_term_by. By the param url is a better work around for this.

Do we have a use for it core other than get_blog_details ? We replaced nearly all calls to the blogs table directly a while ago..

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


3 days ago

Note: See TracTickets for help on using tickets.