WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 2 weeks ago

#42072 closed enhancement (fixed)

Add limit 1 to domain_exists

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

Description

domain_exists 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 (4)

42072.diff (399 bytes) - added by danieltj 2 weeks ago.
Adds number parameter to limit by 1
42072.2.diff (658 bytes) - added by jeremyfelt 2 weeks ago.
42072.3.diff (710 bytes) - added by jeremyfelt 2 weeks ago.
42072.4.diff (2.3 KB) - added by jeremyfelt 2 weeks ago.

Download all attachments as: .zip

Change History (16)

#1 @spacedmonkey
2 weeks ago

  • Keywords needs-patch good-first-bug needs-unit-tests added

@danieltj
2 weeks ago

Adds number parameter to limit by 1

#2 @danieltj
2 weeks ago

  • Keywords has-patch added; needs-patch removed

Added a parameter to fetch only one results back by adding number to arguments in 42072.diff.

#3 @flixos90
2 weeks 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.
  • 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.
Last edited 2 weeks ago by flixos90 (previous) (diff)

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


2 weeks ago

@jeremyfelt
2 weeks ago

#5 @jeremyfelt
2 weeks ago

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

I think we're okay to use get_site_by() here. While a full object is returned, domain_exists() is not widely used and because only one result is being returned, the performance difference is negligible. See 42072.2.diff. Tests exist for domain_exists() and those continue to pass.

@jeremyfelt
2 weeks ago

#6 @jeremyfelt
2 weeks ago

42072.3.diff ensures we're passing an ID to the filter as expected.

@jeremyfelt
2 weeks ago

#7 @jeremyfelt
2 weeks ago

In 42072.4.diff:

  • Ensure the URL passed to get_site_by() is formatted with expected slashes.
  • Remove the www.wordpress.org site from tests. Site creation was failing because domain_exists() would return true after get_site_by( 'url', 'www.wordpress.org/' ); found wordpress.org. We don't support the co-existence of www and non-www domains, so it should be okay to apply this restriction to domain_exists() now.

#8 @jeremyfelt
2 weeks ago

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

#9 @jeremyfelt
2 weeks ago

I'm not feeling confident enough in domain_exists() changing behavior and treating www/non-www the same. For 4.9, let's fix the bug by just adding the count of 1.

#10 @jeremyfelt
2 weeks ago

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

In 41736:

Multisite: Only query for one site in domain_exists().

get_sites() queries for a maximum of 100 records by default. In domain_exists(), we only use one.

Props danieltj, spacedmonkey.
Fixes #42072.

#11 @jeremyfelt
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[41736] is the commit and fix for #42073, get_blog_id_from_url(), not domain_exists().

#12 @jeremyfelt
2 weeks ago

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

In 41738:

Multisite: Only query for one site in domain_exists().

get_sites() queries for a maximum of 100 records by default. In domain_exists(), we only use one.

A previous commit, [41736], has this same commit message but applies to get_blog_id_from_url() and #42073 instead.

Props danieltj, spacedmonkey.
Fixes #42072.

Note: See TracTickets for help on using tickets.