{twitter}.twitter_id incompletely stored (final digits are zeroes) due to json_decode limitation in PHP<5.3

I can confirm that all my entries have between 2-7 trailing zeros (mostly 2-4). All numbers are 16 digits.

mysql/MyISAM

Could this be something to do with the function json_decode, which is used in Line# 261 in twitter.lib.php.

I got thinking that this may not be anything to do with the database or how the insert into the database works. It may be something to do with the way the twitter status id is returned/obtained from the twitter api itself.

Hence, I started following the twitter_fetch_user_timeline function in the twitter_cron() hook. I found that it finally obtains the twitter timeline and then uses json_decode to parse the results.

I looked up the PHP manual for json_decode ( http://php.net/manual/en/function.json-decode.php ) and found that there is one example (Example# 5 in the PHP Manual) which talks about how long integers will be handled by this function.

Maybe using

return json_decode($response, TRUE, 512, JSON_BIGINT_AS_STRING);

instead of

return json_decode($response, TRUE);

will solve the problem?

But, there may be downstream impact since the Database has twitter_id as a Float and not a string.

Your comments?

Regards,
KH

I was probably right. It appears to me that the JSON format is what is causing the issue with the last few digits of the twitter_id being corrupted.

However, the earlier approach of replacing the json_decode line will not work since json_decode will support JSON_BIGINT_AS_STRING only from PHP version 5.3 onwards. Most of us are still on PHP 5.2.x

I knew that this problem started manifesting itself only after I upgraded to version 6.x-3.0-beta2. Hence, I compared the code in version 6.x-3.0-beta3 with that in version 2.6 and found that version 3.0 uses the JSON format whereas version 2.6 used XML.

Since the twitter api still supports the XML format, I borrowed the code from version 2.6 and added it to the version 3.0-beta3 code files.

The resultant file is attached. I can confirm that using this updated code solved this problem for me. However, I am not too sure if there are other problems potentially waiting in the wings, which I haven't encountered yet.

You could take a look if you choose.

Regards,
KH

Here is the file

I applied your patch from comment #10.
Thank you very much it seems to work. There are now errors any longer in the log after cron runs.

Thanks a lot!

I had the exact same issue and the patch at #10 fixed it for me.

Storing big id's in the mySQL DB no longer seems to be a problem. However:

(in twitter.inc -> function twitter_status_save)

if (db_result(db_query("SELECT * FROM {twitter} WHERE twitter_id = %d", $status->id))) {

This error check, to see if tweets already exist in the db, seems unreliable to me.

if (db_result(db_query("SELECT * FROM {twitter} WHERE twitter_id = %b", $status->id))) {

This looks better. But is it?

%d - the argument is treated as an integer, and presented as a (signed) decimal number.
%b - the argument is treated as an integer, and presented as a binary number.

Huh? what? Ok im not sure myself, lets test:

$i = 17610042068967424;
printf($i . '<br/>'); // fun stuff, this will be been turned into a float (%f) ?
printf('%d <br/>', $i); // standard integer representation
printf('%b <br/>', $i); // binary representation
printf('%s <br/>', $i); // string

//Output:
//1.7610042069E+16
//1101110000000000010000000000000 
//1845501952
//1.7610042069E+16 

My findings:
1845501952 is actually a very strange number, as max_int on 32 bit = 2147483647
Output from %b is expected, i guess, but i find it kind of strange that it works quite well when using it in queries
Result from %s is unexpected, i guess i would expect a string as output, but it seems to work aswell

Still looking for reason to choose either %b (binary) or %s (string) representation of $status->id. Or maybe a better solution?

@grobot: your patch from #16 works for me.

tested #16 with no issues. Thanks!

sorry.. I spoke too fast.. #16 didn't work for me..

I had error: json_decode() expects at most 3 parameters. 4 given.

subscribing

Please don't try to reinvent the wheel.

After json_decode() that BIGINT exists in memory as correct integer. To output it you just need to format it like this:

sprintf('Here your BIGINT: %.0f', $decoded_json['some_bigint_eg_id']);

voila

why not use defined() instead of the version check? If the constant exists, then decode must support the fourth parameter.

  if (defined('JSON_BIGINT_AS_STRING')) {
    return json_decode($response, TRUE, 512, JSON_BIGINT_AS_STRING);
  }
  else {
    // ...
  }

> Just a thought - if I'm reading the PHP docco linked correctly, the 4th param (JSON decode option) isn't contingent on PHP > 5.3.0, it's only in the development ver?

Yup. I'm on PHP 5.3 and getting an error message about that 4th param. I don't think the JSON_BIGINT_AS_STRING is something we can count on.

>sprintf('Here your BIGINT: %.0f', $decoded_json['some_bigint_eg_id']);

@yark, I'm not sure entirely what you mean by the above, but looking in the entire array of stuff returned by json, there is a string version of the id. Also, I've no idea what you're doing with sprintf does -- best way to really explain what you mean is a patch :)

So here is my patch, which fixes the problem for me.

It could do with more work, probably giving the in_reply_to_status_id key the same treatment, and the sprintf thing if that needs doing too.

Here's an updated patch that takes into account that the return from the API may be a JSON array (in my case it seems to always be one). @bumathan, please let me know if that takes care of the issue for you.

I'm not entirely sure I get what this does...

+++ b/twitter.lib.php
@@ -257,7 +257,17 @@ class Twitter {
+            $item['id'] = $item['id_str'];

I'm not sure what this is for as it's not going to change what you return...

And just some coding standards stuff...

+++ b/twitter.lib.php
@@ -257,7 +257,17 @@ class Twitter {
+          // if we're getting a single object back as JSON

Comments should start with a capital and end with a full stop.

+++ b/twitter.lib.php
@@ -257,7 +257,17 @@ class Twitter {
+        } else {

Coding standards: don't coddle elses.

Powered by Dreditor.

Also MySQL/PHP related?

I have a custom module that converts tweets to nodes. As part of this one has to read from the twitter table of course and interestingly this fails:

  $tweet = db_fetch_object(db_query('SELECT * FROM {twitter} WHERE twitter_id = %d', $twitter_id));
  # Tweet is empty here

But this works (note the treatment of the ID as a string):

  $tweet = db_fetch_object(db_query('SELECT * FROM {twitter} WHERE twitter_id = %s', $twitter_id));
  # Tweet is defined here

In both cases $twitter_id was read from the database using db_fetch_object (and passed to a function from $row->twitter_id).

Hopefully that is of some help here.

Basically - as one never really has to mathematically manipulate these IDs I would suggest just treating them as strings in the twitter module.

#28: The problem is that the integer is too big for PHP to represent, leading to corruption of the id with trailing zeros. For older tweets, there's no difference, but newer tweets have much higher id values. If you're running on a 64-bit system, you might not see the problem at all.

Sorry about the coding standards. I will work on getting that back into compliance, but it's better to have a fix than nothing at all!

#29: Yep, same underlying issue, see #966748: Duplicate key error on normal import.

@28

  foreach ($response_decoded as &$item) {
    $item['id'] = $item['id_str'];
  }

The assignment inside the loop wouldn't normally do anything, since foreach copies array elements, but in this instance the reference operator (as &$item) allows the foreach loop to operate directly on the original array's elements.

----

Here's a patch that builds upon #27, making PHP do the work if it can so we don't have to.
First it checks if running on a 64bit version on PHP, so that any special handling can be avoided.
Next, if JSON_BIGINT_AS_STRING is defined it will use that as a parameter to json_decode().
Finally, it will overwrite values of ['id'] with the string values in ['id_str']

----

As in #21, I'm also reluctant to rely on id_str if it's not in the docs. Using preg_replace also allows the opportunity to replace every large integer, not just those keyed with 'id', and at any level of depth in the JSON data structure without extra iterations or recursion.

Here's my attempt at using a general regular expression pattern to convert large integers, however there are a few issues.

Anything like a JSON string inside the JSON being parsed could make things blow up spectacularly.

While matching scalar values (e.g. "id":12345) should be pretty safe, the match for values in an array (e.g. [123,456,789]) has to be pretty lenient and could cause problems. The issue is that if the first element in an array is matched, the next element cannot use the separating comma as part if its match; trying to match both will result in only the odd-positioned elements being converted (e.g. [123,456,789,123] changes to ["123",456,"789",123]).
The pattern for the second replacement call in the patch will match all the values, but also any number (of suitable length) followed by a comma or closing bracket will match. (e.g. "stringvalue":"this is some numbers: 123, 456" will get mangled to "stringvalue":"this is some numbers: "123", 456")

----

If it's safe to ignore arrays, the second replacement could be omitted.
To be extra safe, if all numbers to be converted have identifiers ending with 'id', the first match could be made more restrictive: preg_replace('/"(\\w*id)":\\s*(\\d{' . $length . ',})\\s*([},])/','"\\1":"\\2"\\3'

Hi, @gapple.

I like your approach in #31. Somehow I'd missed that id_str wasn't documented. I feel really wary of using preg_replace on JSON, as in #32, if we can avoid it - string manipulation always seems simpler than it turns out being.

Also, @gapple, could you reroll your patch against the current dev version? That seems like it's against a slightly older version.

Just so we're fixing the right problem... when does the long integer actually break? is it broken by json_decode, or once Drupal tries to do something with the result of that?

I can't test at the moment, but I'm pretty sure it's inherently broken, because of the limitations of PHP's integer. json_decode attempts (correctly) to store the value in an integer, which doesn't work (details repeated, repeatedly, elsewhere). It's not a problem with Drupal or json_decode, but a limitation of the language.

And the reason that the preg_replace approach is a solution (albeit a sort of troublesome one) is that it makes the change to the JSON string _before_ it gets decoded into an integer.

I didn't know you had pushed your patch to the repository before working on my own.
Here is an updated patch for the integer size and JSON_BIGINT_AS_STRING checks, based on current 3.x-dev.

----

I am also wary of the preg_replace solution due to the potential for it to mangle data, just worried that handling each individual element will get out of hand (and that it's not supported by StatusNet yet). In #21 @grobot noted that "in_reply_to_status_id" may also need to be handled as a bigint.

Thanks! I've come around to the preg_replace approach since my first post on it - you're right that it's the way to go. I think we should probably get this patch into the module!

Here's a patch with the preg_replace, and the checks for 64bit and JSON_BIGINT_AS_STRING.

I've used "((?:\w+_)?id)"\s*:\s*(\d{' . $length . ',})\s*(,|}) as the matching expression, as I think it's the simplest, safest match we can start with. It will match elements with the key "id" or ending in "_id" that has an integer value long enough to potentially not fit within the integer value limit.

It will not match any integer values within an array, but I'm not sure if these are an issue or priority.

Here is an improved patch for converting all large integers using a regular expression.

This patch uses a look-ahead subexpression in the array regex to check that the proper delimiters are both before and after the integer, fixing some issues I ran into in #32.

The only remaining problem is JSON-like string values. The most likely is a list of large numbers separated by commas (e.g. "string": "1234,5678,9101,2345" will convert to "string": "1234,"5678","9101",2345", "string": "list: 1234, 5678, 9101" to "string": "list: "1234", "5678", 9101").

With some more experimenting, I think the issue of matching within a string can be avoided by adding the following look-ahead match to the end of each expression:
(?=(?:[^"]*(?<!\\\)"[^"]*(?<!\\\)")*[^"]*$)
This will check that the remaining string to be matched only contains pairs of unescaped quote characters, so that numbers inside quotes will not match.

All this regex stuff really does make me nervous. I think I'm going to check with Twitter to see 1) why they're providing id_str and 2) if they can get it into the documentation so we can rely on it. Until then, I would rather just use id_str - replacing all long integers with strings seems like it might be a solution in search of a problem, as the only thing we've actually got a problem with is the id.

To allow javascript and JSON parsers to read the IDs we need to include a
string version of any ID when responding in the JSON format. What this means
is Status, User, Direct Message and Saved Search IDs in the Twitter API will
now be returned as an integer and a string in JSON responses. This will
apply to the main Twitter API, the Streaming API and the Search API.

That seems pretty well-documented to me.

So it's a bug :) Maybe someone can contact twitter and ask them to update their documentation?

Hi, folks. I assumed that this was the reason they were providing id_str, because they were aware their ids were getting too big to reliably represent as integers - that's what I wanted to ask them about, but the explanation @joachim points to makes that unnecessary. I believe they'll update the documentation if we bring it to their attention.

I think we can use id_str if it's there, use id if it's not, and be pretty confident that we're not going to break another implementation of Twitter's API.

I've created a ticket with Twitter to get that added. We'll see where it goes, but I feel pretty comfortable taking a chance on this. Working with Twitter's API always involves a leap of faith. :)

Here's the response from Twitter:

Hi Michael,
Thank you for pointing this out. We'll make sure the documentation is updated with correct response information soon. Let me know if you spot anything else.

Brian
Twitter API Policy

We should be good to go on using id_str.

I've just checked the code, and that's exactly what we're doing.

Let's not forget the patch back in #31 that doesn't have any funky regex stuff, and just tries to use PHP's capabilities if available first (64bit or JSON_BIGINT_AS_STRING). I think it should still apply to the latest 3.x-dev, but let me know if you would like a re-roll.

I think it was also noted that there are other id values that may need to be handled as well, and if id values are nested at any deeper levels some extra handling will be required.

I'm ok with the approach from #31, but I don't see the downside of just using the string Twitter provides. If other services get into tweet ids that are high enough to cause problems, they'll need to implement something similar.

I'd say let's worry about any other id values as we come to them. If Twitter is concerned enough about this id to provide it as a string, they would probably do so anywhere else an integer might break.

Because the string value is being used to overwrite the (mangled) integer value, there is overhead to looping through each item and performing the assignment.
This overhead should be unnecessary if the system uses 64bit integers, or supports JSON_BIGINT_AS_STRING; both of which should become more common in the future.

Hi folks.

I've gone through the entire thread and the analysis made is very reasonable. I applied the patch (manually, just to make sure) from #56 while going through each line to understand what it does. Unfortunately I still get the 'Duplicate key' error on cron run (though the tweets are rightly posted to twitter).

Funnily enough, I'm running PHP 5.3.2 (as shown below from version request in terminal) but a PHP request for the defined constant JSON_BIGINT_AS_STRING returns nothing.

PHP 5.3.2-1ubuntu4.9 with Suhosin-Patch (cli) (built: May  3 2011 00:43:34) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies

With the explanations given above everything should be working, but for some reason it doesn't on my system.

Can you help me out @grobot.

Thanks in advance.

@dela.deyoungster
the $options parameter to json_decode (where JSON_BIGINT_AS_STRING is used) is only available in PHP 5.4+ (it was previously available in some development versions of 5.3)

What is the value of your PHP_INT_SIZE constant? (4 means 32bit, 8 means 64bit)

How could I reproduce this bug? Can anyone provide steps or scenarios? I have not faced it while fixing bugs over the last three weeks.

Cheers

Looks like the same issue occurs with the fields in_reply_to_status_id as well.

We will need to include a line of code

$response_decoded['in_reply_to_status_id'] = $response_decoded['in_reply_to_status_id_str'];

to handle this error.

More details of the change I have made is available in the attachments in #1520008: Extract actual URLs from t.co wrappers using Tweet Entities

Regards,
KH

Is this still an issue? If so, it needs updating for 7.x-5.x.

 $ git apply twitter-985544-62-reroll.patch 
error: patch failed: twitter.lib.php:229
error: twitter.lib.php: patch does not apply