WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#34450 closed defect (bug) (fixed)

get_id_from_blogname returns false if the main domain starts with www.

Reported by: igmoweb Owned by: flixos90
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.3.1
Component: Networks and Sites Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

I have a local Multisite installation with the following domain:

www.wordpress.dev

I create a new site called: demo1.wordpress.dev, which is fine and works as expected (the www. is trimmed as expected too).

When I try the following code:

$result = get_id_from_blogname( 'demo1' );

$result is false and not the Blog ID.

I think that the issue is located inside the get_id_from_blogname function:

$domain = $slug . '.' . $current_site->domain;
should be
$domain = $slug . '.' . preg_replace( '|^www\.|', '', $current_site->domain );

But I'm not sure if this could affect other areas.

Attachments (3)

34450.patch (487 bytes) - added by igmoweb 4 years ago.
patch to get_id_from_blogname
34450.diff (5.3 KB) - added by flixos90 3 years ago.
34450.2.diff (5.6 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (14)

#1 @swissspidy
4 years ago

  • Keywords needs-patch needs-unit-tests added

Even if that would be expected behaviour (which I don't think it is), unit tests for get_id_from_blogname() would be great.

@igmoweb
4 years ago

patch to get_id_from_blogname

#2 @igmoweb
4 years ago

I've added the change in 34450.patch though I'm not able to make unit tests under the following conditions:

I've grabbed the regexp from wp-admin/site-new.php where www. is stripped out.

  1. Subdomains install
  2. Prefix www. to the test domain without changing WP_TESTS_DOMAIN const in wp-tests-config.php

It would be awesome if someone is capable of that or jsut give me some directions to set the environment with PHPUnit tests bootstrap and I'll make it.

#3 @jeremyfelt
3 years ago

Thanks for your work on this, @igmoweb. I think there's a good chance that WP_Site_Query via #35791 can be used in get_id_from_blogname(). Testing/feedback is welcome!

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


3 years ago

#5 @flixos90
3 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90
3 years ago

#6 @flixos90
3 years ago

  • Keywords has-unit-tests 2nd-opinion added; needs-unit-tests needs-refresh removed

34450.diff is an updated patch that makes use of WP_Site_Query (via get_sites()) in the function. This also allows us to get rid of the get_id_from_blogname_* cache key, since site query results are cached anyway. An additional benefit we get is that the function now actually returns an integer, as stated in the docs (before the return value would actually be a string, due to wpdb::get_var()).

The patch also adds unit tests, one for a www-prefixed network, another one without www.

One thing that we should probably improve (but I didn't yet because I wasn't sure): What do we do if the function fails to find an ID? Previously it would have returned null (see wpdb::get_var()), although this is undocumented. My proposal would be to check if the array returned by get_sites() is empty, and if so return null. And of course we would then document it as well.

Last edited 3 years ago by flixos90 (previous) (diff)

#7 @igmoweb
3 years ago

@flixos90

I would keep the same return types than before. So yes, I would check if empty( get_sites() ) return null; to keep backward compatibility and improve documentation for the function.

@flixos90
3 years ago

#8 @flixos90
3 years ago

  • Keywords 2nd-opinion removed

34450.2.diff is an updated patch that adjusts the return type of the function for backward compatibility and improves the documentation accordingly.

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


3 years ago

#10 @jeremyfelt
3 years ago

After writing up the commit message for this, I realized that it made sense to handle the switch to get_sites() as part of a separate commit and therefore a separate ticket. See #38175.

#11 @jeremyfelt
3 years ago

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

In 38658:

Multisite: Handle get_id_from_blogname() lookups when the network domain has www..

Previously, if a network's domain started with www. in a subdomain configuration, a slug lookup with get_id_from_blogname() would not match an existing site. A similar lookup in a subdirectory configuration would work fine.

This strips www. from the network's domain in a subdomain configuration during the lookup and returns the site as expected.

Adds tests which would previously fail in a subdomain configuration, but now pass in both configurations.

Props igmoweb, flixos90.
Fixes #34450.

Note: See TracTickets for help on using tickets.