WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#42073 closed defect (bug) (fixed)

Add limit 1 to get_blog_id_from_url

Reported by: spacedmonkey Owned by: jeremyfelt
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.6
Component: Networks and Sites Keywords: has-patch good-first-bug
Focuses: multisite Cc:

Description

get_blog_id_from_url uses to get_sites to looks if site exists. But it only uses the first result. The param limit should be added to get_sites, limiting the query.

Attachments (2)

42073.diff (394 bytes) - added by danieltj 3 months ago.
Adds number parameter to limit by 1
42073.2.diff (2.8 KB) - added by jeremyfelt 3 months ago.

Download all attachments as: .zip

Change History (9)

@danieltj
3 months ago

Adds number parameter to limit by 1

#1 @danieltj
3 months ago

  • Keywords has-patch added; needs-patch removed

In 42073.diff, limits the query to get the first result from get_sites().

#2 @flixos90
3 months ago

We should consider simplifying the function further, by internally using the new get_site_by(). See https://core.trac.wordpress.org/ticket/40180#comment:24

  • The advantage would be less code to maintain, and we could theoretically even deprecate this function at some point (not a necessity though).
  • The disadvantage would be that get_site_by() returns a full object, while we only need an ID here. Not sure that's a real issue though, since it only queries for one result anyway, so it shouldn't have a significant impact on performance.

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


3 months ago

@jeremyfelt
3 months ago

#4 @jeremyfelt
3 months ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

42073.2.diff uses get_site_by() and removes tests for blog-id-cache. If we go this route, we'll be able to remove blog-id-cache from a handful of other places as well.

#5 @jeremyfelt
3 months ago

  • Keywords needs-unit-tests removed
  • Milestone changed from Awaiting Review to 4.9

#6 @jeremyfelt
2 months ago

As with #42072, I'm a little wary of the change in behavior here. Less so for get_blog_id_from_url(), but because we'll be removing blog-id-cache, I'd rather make the transition to get_site_by() earlier in a future release (5.0?).

Let's go with just the limit change for 4.9.

#7 @jeremyfelt
2 months ago

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

:facepalm:

[41736] has a commit message about domain_exists(), but that change was really for get_blog_id_from_url().

This ticket is fixed in [41736].

Note: See TracTickets for help on using tickets.