Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#20966 closed defect (bug) (fixed)

Inconsistent Variable Usage

Reported by: ericmann's profile ericmann Owned by: ericmann's profile ericmann
Milestone: 3.4.1 Priority: normal
Severity: minor Version: 3.1
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

Someone in the forums alerted me to a potential issue with the code in wp_version_check(). It seems we're using two different functions to get a user count depending on whether we're in Multisite or a standalone installation.

The Multisite function (get_user_count()) returns a number. The standalone function (count_users()) returns an array. We then assume the array when we use the variable later to build the version check query.

In some situations, this can result in an "illegal string offset" error.

In other situations, it will generate inaccurate data. Assume the following situation:

  • get_user_count() returns "100" because there are 100 users.
  • $user_count['total_users'] will return "1"

When you reference a string as an array with an offset that doesn't exist, it only returns the first digit. So our count, in this case, is only accurate for Multisite installations with 1-9 users.

Not a huge end-user issue ... but confusing for WP.org stats in the long run if it remains unpatched.

I have worked out a patch that will fix the stat reporting issue for us and silence any "illegal string offset" issues other developers might be experiencing.

Attachments (1)

20966.diff (787 bytes) - added by ericmann 12 years ago.

Download all attachments as: .zip

Change History (12)

@ericmann
12 years ago

#2 @SergeyBiryukov
12 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.4.1
  • Version changed from 3.4 to 3.1

Introduced in [16604]. I don't see the "Illegal string offset" warning on PHP 5.2.14, but can reproduce the inaccurate data (after calling wp_update_network_counts() to make sure the count is updated).

#3 @cogmios
12 years ago

I have php 5.4.4 see the warning.
I assume it has something todo with wp_cache_get where site-options does not contain the user-count yet since it reads then the value from the meta_value. Maybe the php doc from get_user_count could also be updated since the return value should NOT be an integer if i follow correctly.

#4 follow-up: @nacin
12 years ago

I am interested to see how "illegal string offset" can occur. Also, is it a notice, or a warning? Big difference as to whether we plan to include a fix for this in 3.4.1. Otherwise, we don't currently even aggregate the user stats, so I have no qualms about waiting until 3.5 for this.

#5 @cogmios
12 years ago

it's a warning

It happens on http://(url)/wp-admin/network/update-core.php where the version check takes place or every site admin page if the check hasnt runt.

Root cause is that the cache is not set for user_count in other words it does not appear in the site_options cache (but it is in the db).

Im looking into why it does not set the wp object cache

Version 10, edited 12 years ago by cogmios (previous) (next) (diff)

#6 in reply to: ↑ 4 @ericmann
12 years ago

Replying to nacin:

Big difference as to whether we plan to include a fix for this in 3.4.1. Otherwise, we don't currently even aggregate the user stats, so I have no qualms about waiting until 3.5 for this.

Since the code's been in there since 3.1 and the issue is just now coming up (and only for a handful of users) I don't see this as a critical issue. Particularly if you can work around it by lowering your PHP warning settings. I'm leaning towards waiting until 3.5.

#7 @cogmios
12 years ago

I found out... that if I run in a PHP version < 5.4

$expected_array_got_string = 'somestring';
echo $expected_array_got_string['some_key'];

that it returns "s"

However starting from PHP version 5.4. it will throw: Warning: Illegal string offset 'some_key'

:)

I think that's the reason for the low amount of users seeing it...

(see also (related) example 2 here: http://php.net/manual/en/function.empty.php )

So... to see the error... simply install php 5.4 or later and hit the check updates page a few times to get it NOT from the cache)

Last edited 12 years ago by cogmios (previous) (diff)

#8 @ericmann
12 years ago

I still definitely see this as an important fix moving forward, but since it only seems to affect those running PHP >= 5.4, I wanted to do a quick sanity check to see just how many users this bug effects.

According to http://wordpress.org/about/stats/, only 0.2% of self-hosted WordPress installs are running PHP 5.4. About 98% of installs are running either PHP 5.3 or 5.2, which explains why we haven't noticed this issue before.

But, moving forward, I do think this will become more of an issue.

#9 @nacin
12 years ago

In [21105]:

Fix string offset PHP 5.4 error by normalizing $user_count to always be an integer. props ericmann. see #20966 for trunk.

#10 @nacin
12 years ago

  • Keywords dev-feedback removed

#11 @nacin
12 years ago

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

In [21121]:

Fix string offset PHP 5.4 error by normalizing to always be an integer. props ericmann. fixes #20966 for 3.4

Note: See TracTickets for help on using tickets.