WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#41619 closed defect (bug) (fixed)

Fix documentation issues for `domain_exists()`

Reported by: flixos90 Owned by: stevenlinx
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: good-first-bug has-patch commit
Focuses: docs, multisite Cc:

Description

There are several issues with the documentation for domain_exists() and its filter:

  • It's actually the domain-path combination that is checked. So the term "blogname" should probably be replaced with something like "domain-path combination".
  • In the filter documentation the term "site ID" should be used instead of "blog_id".
  • There's no description for the return type, and it's incorrectly highlighted as int where it should be int|null.
  • The $wpdb reference in the docblock can be removed as it's actually not used in the function.

Attachments (2)

41619.diff (1.6 KB) - added by stevenlinx 3 months ago.
41619.2.diff (1.8 KB) - added by stevenlinx 3 months ago.

Download all attachments as: .zip

Change History (11)

@stevenlinx
3 months ago

#1 @stevenlinx
3 months ago

  • Keywords has-patch added; needs-patch removed

#2 @flixos90
3 months ago

  • Owner set to stevenlinx
  • Status changed from new to assigned

Thanks for the patch @stevenlinx, looks good!

@DrewAPicture @jeremyfelt Any objections for the term "domain-path combination"? Is there a better one we should use?

#3 @DrewAPicture
3 months ago

Hmm. I think introducing the new terminology actually confuses it a bit more. You're correct that domain_exists() literally checks the domain path combination, though "domain-path combination" doesn't really convey what's being checked the way blogname sort-of does currently.

Rather than introduce new vernacular, I wonder if simply providing context for what "blogname" represents in the DocBlock description wouldn't be more consistent with the rest of core. For instance, in the newblogname hook doc, the explanation of what the [blog] name represents is pretty clear:

 * Filters the new site name during registration.
 *
 * The name is the site's subdomain or the site's subdirectory
 * path depending on the network settings.
 *
Last edited 3 months ago by DrewAPicture (previous) (diff)

#4 @flixos90
3 months ago

@DrewAPicture That makes sense. Let's go with "site name" then (replacing the old terminology "blogname") and provide an explanation like the one you shared.

@stevenlinx
3 months ago

#5 @flixos90
3 months ago

Changes look good @stevenlinx, thanks for continuing to work on it!

@DrewAPicture You're good with this?

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


3 months ago

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


2 months ago

#8 @DrewAPicture
2 months ago

  • Keywords commit added

@flixos90 Looks good. All yours :-)

#9 @flixos90
2 months ago

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

In 41596:

Multisite: Fix documentation issues for domain_exists().

Props stevenlinx.
Fixes #41619.

Note: See TracTickets for help on using tickets.