Opened 8 years ago
Last modified 6 days ago
#40736 new defect (bug)
Ensure that `get_blog_count()` and `get_user_count()` return an integer
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Networks and Sites | Keywords: | 2nd-opinion has-patch has-unit-tests |
Focuses: | multisite | Cc: |
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
@
8 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
@
8 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()
.
@
8 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.
8 years ago
#5
@
8 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.8
and4.8.0
should be replaced with4.9.0
, as this change won't make it into 4.8. MU
should not be used in the@since
annotation for new functions.- The function
get_blog_count()
should be moved to thems-deprecated.php
file.
@
8 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function.
@
8 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function. All changes done.
@
8 years ago
int typecating for get_user_count(). Deprecated get_blog_count() function.Developed get_site_count() function.Final patch
#7
@
8 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).
Alternatively, we may even aim for get_network_site_count()
(would also need a get_network_user_count()
) in order to have parity with the other functions that deal with these settings (such as wp_update_network_site_counts()
). This is only an idea though.
#8
@
8 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.
8 years ago
This ticket was mentioned in Slack in #core-multisite by realloc. View the logs.
2 weeks ago
This ticket was mentioned in Slack in #core-multisite by soean. View the logs.
12 days ago
This ticket was mentioned in PR #8904 on WordPress/wordpress-develop by @Soean.
12 days 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
@
12 days 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
@
12 days 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
@
11 days 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
@
6 days ago
Replying to johnbillion:
Under what condition can
get_blog_count()
return a non-integer?
Theoretically:
- The
$default_value
return value ofget_network_option()
isfalse
- Using the
pre_site_option_blog_count
filter with a non-int - Using the
default_site_option_blog_count
filter with a non-int - Using the
site_option_blog_count
filter 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.