Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#42073 closed defect (bug) (fixed)

Add limit 1 to get_blog_id_from_url

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

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 9 years ago.
Adds number parameter to limit by 1
42073.2.diff​ (2.8 KB ) - added by jeremyfelt 9 years ago.

Download all attachments as: .zip

Change History (9)

@danieltj
9 years ago

Adds number parameter to limit by 1

#1 @danieltj
9 years ago

  • Keywords has-patch added; needs-patch removed

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

#2 @flixos90
9 years 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.


9 years ago

@jeremyfelt
9 years ago

#4 @jeremyfelt
9 years ago

  • Owner set to jeremyfelt
  • Status new → 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
9 years ago

  • Keywords needs-unit-tests removed
  • Milestone Awaiting Review → 4.9

#6 @jeremyfelt
9 years 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
9 years ago

  • Resolution → fixed
  • Status reviewing → 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.