![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 9 years ago
Last modified 13 months ago
#40736 new defect (bug)
Ensure that `get_blog_count()` and `get_user_count()` return an integer
| Reported by: | flixos90 | Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | Future Release |
| Component: | Networks and Sites | Version: | |
| Severity: | normal | Keywords: | 2nd-opinion has-patch has-unit-tests |
| Cc: | Focuses: | multisite |
Description
The documentation for the functions get_blog_count() and get_user_count() states that the return type is an integer, however the functions only call get_network_option() without any typecast.
The functions should be adjusted to actually reflect their documentation. While this might theoretically be problematic in terms of backward-compatibility, in #40724 some initial thoughts of this being a viable change were expressed, so let's think about it on this ticket.
Attachments (11)
Change History (28)
#2
follow-up:
↓ 3
@
9 years ago
Since we're striving towards an API with site/network terminology in favor of blog/site long-term, we should consider deprecating get_blog_count() in favor of a get_site_count(). The change of the return type, whether it was specified in the docs or not, is a breaking change anyway, so I suggest to deprecate the function at the same time and replace their usage in core with calls to get_site_count().
#3
in reply to: ↑ 2
@
9 years ago
I have done all chanmges. I am not able to login with my current internetc connection. It gives me erroe like:
Your IP (103.217.85.2) has been flagged for potential security violations. Find out more...
I am trying to upload patch by proxy, but it changesname. I will try to give patch again tommorow morning again
Replying to flixos90:
Since we're striving towards an API with site/network terminology in favor of blog/site long-term, we should consider deprecating
get_blog_count()in favor of aget_site_count(). The change of the return type, whether it was specified in the docs or not, is a breaking change anyway, so I suggest to deprecate the function at the same time and replace their usage in core with calls toget_site_count().
@
9 years ago
Deprecated get_blog_count() function and developed get_site_count function. This is final.
This ticket was mentioned in Slack in #core by pmbaldha. View the logs.
9 years ago
#5
@
9 years ago
Thanks for the patch @pmbaldha!
Looks good except for a few minor issues:
- Let's cast the values to integers with
(int)instead of using the oldintval(). - For version numbers, the
4.8and4.8.0should be replaced with4.9.0, as this change won't make it into 4.8. MUshould not be used in the@sinceannotation for new functions.- The function
get_blog_count()should be moved to thems-deprecated.phpfile.
@
9 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function.
@
9 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function. All changes done.
@
9 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function.Final patch
#7
@
9 years ago
Highlighting https://core.trac.wordpress.org/ticket/40736#comment:2 again: Is it reasonable to change the name from get_blog_count() to the more appropriate get_site_count()? In my opinion it's fine since we're practically breaking back-compatibility anyway, so we might as well deprecate the function in favor of the new one (and of course also adjust its old calls in core).
#8
@
9 years ago
wp_update_network_site_counts() function is used or called by internal wordpress code. Generally, Normal Wordpress developer doesn't know about wp_update_network_site_counts() function. He/she are are familiar wth get_blog_count() function. There is no worth to change get_site_count() to get_network_site_count().
This is my personal opinion.
If community like get_network_site_count() (would also need a get_network_user_count()) function name, Please tell me. I will create new patch.
May i know what is opinion of other contributors?
This ticket was mentioned in Slack in #core-multisite by pmbaldha. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by realloc. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-multisite by soean. View the logs.
14 months ago
This ticket was mentioned in PR #8904 on WordPress/wordpress-develop by @Soean.
14 months ago
#13
- Keywords has-patch has-unit-tests added
Deprecate get_blog_count and rename it to get_site_count. Ensure that get_site_count() return an integer.
Trac ticket: https://core.trac.wordpress.org/ticket/40736
#14
@
14 months ago
get_user_count() already has a typecast, so we only updated get_blog_count() and renamed it to get_site_count() as discussed before. We also adjusted the usage of the functions and updated the tests.
props to @heikomamerow
#15
@
14 months ago
This looks good to me!
My single hesitation differs from @flixos90’s recommendation to formally deprecate get_blog_count() (using _deprecated_function()).
I’m not strongly opposed here, but I do always wonder if deprecating older functions without introducing any new functionality adds unnecessary noise – I generally prefer when deprecations are paired with meaningful improvements or alternatives, not just a 1-to-1 rename.
#16
follow-up:
↓ 17
@
14 months ago
The value for the blog_count option gets populated by wp_update_network_site_counts(), which as far as I can tell returns an integer.
Under what condition can get_blog_count() return a non-integer?
#17
in reply to: ↑ 16
@
13 months ago
Replying to johnbillion:
Under what condition can
get_blog_count()return a non-integer?
Theoretically:
- The
$default_valuereturn value ofget_network_option()isfalse - Using the
pre_site_option_blog_countfilter with a non-int - Using the
default_site_option_blog_countfilter with a non-int - Using the
site_option_blog_countfilter with a non-int
From the original ticket description:
the functions only call
get_network_option()without any typecast.
Forcing a typecast inside of get_site_count() (in the PR) mitigates any unexpected return type, at the consequence of the above conditions returning 1 for truthy values or 0 for falsey ones.
No evidence that these filters are being intentionally used to bypass this return value being an integer, so the chance of regressions or breakage is near-zero.
Hard to know if anyone is using get_blog_count() expecting anything but an int, but I sure ain't. 😅
Type cast return values as integers.