WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#35404 closed defect (bug) (fixed)

Incorrect @var docs for WP_Network property types

Reported by: jdgrimes Owned by: jeremyfelt
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Networks and Sites Keywords:
Focuses: docs, multisite Cc:

Description

Several of the properties in the WP_Network class are documented as @var int when they are actually strings.

The $cookie_domain is obviously not an integer, since even its default value is a string:

public $cookie_domain = '';

The $id and $blog_id properties are also documented as int although both of them are usually populated directly from the database values, and therefore will usually be integers represented as strings. To further complicate things, the $blog_id property is declared with a default value that is an integer:

public $blog_id = 0;

Related to this is #33929.

The quandary is whether we should change the docs from int to string to maintain back-compat, or whether we should cast the values to integers to meet the docs and dev expectations (I discovered this because I was doing a strict comparison while expecting the network id to be an integer).

The WP_Network class was just introduced in 4.4.0, so at first glance it might seem like we don't need to worry about backward compatibility for code that is built expecting these values to be strings despite their docs. However, the $current_site global has been around for quite some time, so we should be careful to maintain back-compat with the types devs are expecting for its properties. I'm assuming that prior to [34097] the values of $current_site->id and $current_site->blog_id were both strings. Of course, it is possible that people have been overriding the $current_site global with their own implementations, which might be offering these values as integers instead.

Change History (2)

#1 @jeremyfelt
5 years ago

  • Milestone changed from Awaiting Review to 4.5

Good catches. $cookie_domain is the easy one, we can change that to string with no issue.

I think we're left with changing the docs for $blog_id and $id to string. As mentioned, this object has existed in this format as $current_site for a long time.

I'm not sure that strict comparisons for these properties are a good idea in general, as custom implementations may have been doing all sorts of things in the past. It would be interesting to look into what breakage might occur if we did start casting things to integers, especially as we're getting better at clarifying some of this.

In the WP_Post docs, we note A numeric string, for compatibility reasons. for $post_author and $comment_count. We should probably do the same here. We're able to provide an int $ID property there because it was sanitized as such long before WP_Post was introduced. We do still assign a default value of 0 for these strings. I don't see any harm in that at the moment, as

It also looks like we have the same doc mismatches in WP_Comment for $comment_ID and $comment_post_ID.

#2 @jeremyfelt
5 years ago

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

In 36340:

Docs: Fix type documentation for WP_Network properties.

  • $cookie_domain was incorrectly documented as an int.
  • $id and $blog_id, though numeric, are provided as strings and should be documented as such.

Fixes #35404.

Note: See TracTickets for help on using tickets.