WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#20589 closed defect (bug) (fixed)

domain_exists should add trailing slash to path

Reported by: ejdanderson Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: minor Version: 3.0
Component: Networks and Sites Keywords: has-patch needs-testing
Focuses: multisite Cc:
PR Number:

Description

insert_blog runs trailingslashit on the path, however, domain_exists uses whatever is passed in. If WP is enforcing the trailing slash on blog insertion, it really should when checking existence. It should probably also cast the site_id as an int per insert_blog.

Attachments (3)

ms-functions.diff (520 bytes) - added by ejdanderson 8 years ago.
20589.diff (557 bytes) - added by jeremyfelt 6 years ago.
20589.2.diff (1.4 KB) - added by ericmann 6 years ago.
Adds a test to verify trailingslashit behavior.

Download all attachments as: .zip

Change History (16)

#1 @scribu
8 years ago

  • Cc scribu added

#2 @jamescollins
7 years ago

When working on #21142, I encountered $path being empty when creating a site via wp-signup.php, so I agree that we should be running trailingslashit on the path.

The $site_id parameter does not need to be cast to an integer though, because the $wpdb->prepare() takes care of that.

@jeremyfelt
6 years ago

#3 @jeremyfelt
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

In both places that insert_blog() is used in core, domain_exists() is used immediately before. If we continue to enforce / on sites as they are inserted, we should use the same checks in both functions.

20589.diff is a refresh of the original patch that applies to trunk. Unit tests would need to be refreshed as we don't expect a trailing slash at this point.

Another answer may be to deprecate domain_exists() in favor of something more comprehensive. site_id is currently checked along with domain and path on every request. Using this with another site_id would not identify possible conflicts across multiple networks. This may be a bigger workflow to figure out in another ticket.

Moving to 3.9 for discussion. This could likely apply to an overall domain/path strategy as well.

@ericmann
6 years ago

Adds a test to verify trailingslashit behavior.

#4 @ericmann
6 years ago

  • Keywords needs-unit-tests removed

Contributed patch during WordCamp Phoenix!!!

This ticket was mentioned in IRC in #wordpress-dev by ericmann. View the logs.


6 years ago

#6 @jeremyfelt
6 years ago

  • Keywords needs-testing added

#7 @jeremyfelt
6 years ago

  • Component changed from Multisite to Networks and Sites
  • Focuses multisite added

#8 @jeremyfelt
6 years ago

We might want to hold on any changes to domain_exists() until get_site_by_path() is sorted via #27003. There's a chance that we could deprecate or change the internals so that the trailing slash is not necessary.

#9 follow-up: @jeremyfelt
6 years ago

We can't use get_site_by_path() as a direct replacement in current trunk as it will always search for / as a path in addition to the passed $path argument.

The current patch does add tests and trailing slashes to domain_exists() so that current behavior gets a bit better. It would be nice to drop the site ID check at some point in the future, but this can probably go in to solve the immediate issue.

#10 in reply to: ↑ 9 @nacin
6 years ago

Replying to jeremyfelt:

We can't use get_site_by_path() as a direct replacement in current trunk as it will always search for / as a path in addition to the passed $path argument.

I wonder if that is an indication that get_site_by_path() is not quite flexible enough and/or isn't ideally named. Let's discuss that further.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


6 years ago

#12 @nacin
6 years ago

Fixed by [27717], accidentally posted to #18209. Tests were accidentally omitted, adding those now.

#13 @nacin
6 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27718:

Ensure the $path is trailing-slashed in domain_exists().

Adds tests for [27717].

props ejdanderson, ericmann.
fixes #20589.

Note: See TracTickets for help on using tickets.