Page MenuHomePhabricator

SQLStore3 notice when running SMW_refreshData.php
Closed, ResolvedPublic

Description

http://pastebin.com/nHqWzL8Z

Got this for cbdb751c4f678a521fdc70fbe5b86a1e1df7d7ae (although the issue was not introduced in that revision)


Version: unspecified
Severity: major

Details

Reference
bz42321

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 12:50 AM
bzimport set Reference to bz42321.

Please do not use pastebin for reporting bugs. Content vanishes after some time there and the bug report will be meaningless.

The error as such is not clear to me. Maybe it has to do with the contents of some of the arrays that are diffed there. Is there a way to reproduce this for further tests? What we could use is a dump of the arguments passed to array_diff_assoc() here.

Ok, confirmed. The reason is the stupid implementation of PHP's array_diff_assoc. We use it for comparing two arrays that have entries of the form "key => value" where value is another array and key is a hash that uniquely identifies this array. We use array_diff_assoc since it compares keys and we don't want PHP to compare the rest (we already ensured that this matches if the keys match). PHP, however, compares both the keys and the values, but does this by casting values to string first and then comparing the strings. This leads to a notice in PHP >5.4.0. The result is correct for us, but we should still use another method for computing the diff here.

(In reply to comment #3)

Ok, confirmed. The reason is the stupid implementation of PHP's

Markus, I thought you knew about this. I had used these functions too initially (during GSoC) but removed them later on.

array_diff, array_diff_assoc should never be used ;)

Error log

Attached the error log from Pastebin

Attached:

array_diff, array_diff_assoc should never be used ;)

Why not? I'd say you should use them when you want to diff an array rather then doing it yourself.

Markus, not sure why you set this to minor. This is a bug that happens all the time for a lot of people. Turning off the notice in production environments is a good idea but does not fix it.

Doing a dump of the relevant values when a warning is triggered:

array (size=1)

'daffc259e178764afcf104cc040ca62f' => 
  array (size=3)
    's_id' => string '51' (length=2)
    'o_serialized' => string '1/2013/1/7/13/32/4' (length=18)
    'o_sortkey' => float 2456300.0639352

array (size=1)

'ea9c995574acd1c284d1f28b8e509b09' => 
  array (size=3)
    's_id' => string '51' (length=2)
    'o_serialized' => string '1/2013/1/7/13/29/56' (length=19)
    'o_sortkey' => string '2456300.0624537' (length=15)

Looks like the code is expecting only the inner arrays, and is thus currently not functioning properly. I am afraid that this might lead to things that need updating not getting updated, inc when this is called from the refresh job.

That dump is of $newData[$tableName] and $oldTableData

(In reply to comment #6)

array_diff, array_diff_assoc should never be used ;)

Why not? I'd say you should use them when you want to diff an array rather
then
doing it yourself.

because PHP's implementation of these methods is a bit tricky- refer to the comments on the PHP manual.
Enough of off-topic rant now :)

You need to be aware of what it does and does not do. In my experience, most usecases fall well within what it handles nicely.

This seems to fix the issue: https://gerrit.wikimedia.org/r/#/c/42547/

Ran into a pile of other issues though. To bad I can't attach a grumpy-cat image to the commit :)

Making as fixed as I'm not seeing this anymore. Please reopen if you still run into this.

This issue is still present on 1.8.x. It however is not serious. Just turn notices off.

At the top of localsettings put:

error_reporting(0);
ini_set("display_errors", 0);