Various issues past and present have touched on the need to communicate and store Twitter IDs ({twitter}.id, {twitter}.in_reply_to_status_id, {twitter}. in_reply_to_user_id, {twitter_account}.twitter_uid). Issues arise both in communication (the presence of JSON_BIGINT_AS_STRING in PHP5.3/PHP5.4) and in storage (available size for DB storage of large integers).

Issues known -

  • On 32-bit architecture and older PHP, large integers may wrap or be limited to the architecture max int value
  • Twitter API via JSON provides both id as a number, and id_str as a string to work around architecture issues
  • I believe an issue with Drupal DB layer and large ints existed but is now resolved (#333788: Cannot insert/update unsigned integers or numbers bigger than an integer)
  • Storing the data has been problematic, and approaches to store the data as integer, string, numeric (decimal) have been made.

There are lots of issues on this, which is why I figured that one more won't hurt and might help co-ordinate progress.

CommentFileSizeAuthor
#28 twitter-n2205277-28.patch1.18 KBdamienmckenna
#19 twitter.2205277-19.patch1.36 KBadamps
#14 twitter.2205277-14.patch1.27 KBadamps
#7 twitter-2205277-7.patch1.27 KBadamps

Comments

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Related issues:
xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

At this point, I think we need a co-ordinated solution, and the straightforward one seems to be moving everything back to integers, which I suspect would make this module dependent on JSON_BIGINT_AS_STRING and hosting on 64-bit architecture.

That is the approach put forward by #2055951: Interesting MySQL issue appears to cause randomish "twitter_views_handler_field_formatted_tweet->render()" errors, but that issue only addresses one field and there are several. I think that moving to storing as proper integers is the right step here, as the workarounds from related issues have not been sustainable.

It is likely that versions of PHP without JSON_BIGINT_AS_STRING and storage on 32-bit architecture may require community input / contribution for continued support.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Related issues:
xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Issue summary: View changes
tr33m4n’s picture

tr33m4n commented

Hmm, wish I had more skills to fix this for 32 bit arch. I'm just going to have to live with this problem for now then

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

@tr33m4n, TBH the odds are it'll be less work to migrate to a better hosting environment than for anyone to fix this. Unless there is either (1) a combined effort by many people who want to avoid an hour each migrating hosting platforms, or (2) a single contributor who has a massive investment in 32-bit hosting (?!?!). No-one seems to be putting their hand up for either.

Note that there are multiple discrete issues (PHP's json_encode and MySQL's storage) at play here.

adamps’s picture

adamps commented
StatusFileSize
new twitter-2205277-7.patch1.27 KB

OK, I'm not an expert here, but in the absence of a working solution so far, I've given it a go.

As far as I can see

  • underlying storage is "decimal(20,0) unsigned" and that works fine
  • in the PHP layer need to avoid forcing values to int

The code as stands seems to work fine for twitter_id and in_reply_to_status_id, which have been way too big for an int for a long time. More recent twitter accounts now have user IDs too big for an int, so the same situation now applies to in_reply_to_user_id.

The main bug that I hit on 32-bit is #2142055: in_reply_to_user_id failing for IDs greater than 32 bit - causes cron to fail, which comes when casting in_reply_to_user_id to signed int making it negative.

So I think the fix is to copy the working code for the other two fields to in_reply_to_user_id. I have attached a patch that attempts to do this. So far it has survived limited testing on my dev server. I would welcome comments from anyone else, or feel free to test on your own dev server.

FWIW I would guess that the patch in #2142055 might well work from 2^31 up until 2^32 but will then fail at it's missing the PHP_INT_MAX magic.

adamps’s picture

adamps commented
Status: Needs work » Needs review
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: » #2402307: Plan for Twitter v7.x-5.9 release
rclemings’s picture

rclemings commented

The patch in #7 works for me on 32-bit CentOS6.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Needs work
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: twitter-2205277-7.patch, failed testing.

adamps’s picture

adamps commented
StatusFileSize
new twitter.2205277-14.patch1.27 KB

Sorry my patch was against the stable code rather than dev - updated.

adamps’s picture

adamps commented
Status: Needs work » Needs review

AdamPS queued 7: twitter-2205277-7.patch for re-testing.

The last submitted patch, 7: twitter-2205277-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: twitter.2205277-14.patch, failed testing.

adamps’s picture

adamps commented
Status: Needs work » Needs review
StatusFileSize
new twitter.2205277-19.patch1.36 KB

Try again (sorry tweaked patch by hand and missed a bit)

AdamPS queued 14: twitter.2205277-14.patch for re-testing.

The last submitted patch, 14: twitter.2205277-14.patch, failed testing.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

FYI the tests should now be passing, so if it's reporting a problem then there might be an actual problem ;)

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community

Fixed it. This patch has been confirmed by @rclemings in #10 so I hope I'm not out of order to marked it as RTBC on that basis, and request commit - thanks.

  • DamienMcKenna committed 4643e96 on 7.x-5.x authored by AdamPS 
    Issue #2205277 by AdamPS: Do not convert in_reply_to_user_id field to...
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks AdamPS!

  • DamienMcKenna committed c469baa on 7.x-6.x authored by AdamPS 
    Issue #2205277 by AdamPS: Do not convert in_reply_to_user_id field to...
damienmckenna’s picture

damienmckenna
Location TN, USA
commented

FYI I ported the patch to 7.x-6.x.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Fixed » Needs review
StatusFileSize
new twitter-n2205277-28.patch1.18 KB

Backported to 6.x-5.x.

Status: Needs review » Needs work

The last submitted patch, 28: twitter-n2205277-28.patch, failed testing.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Version: 7.x-5.x-dev » 6.x-5.x-dev
Status: Needs work » Needs review

Lets try that again, this time on the correct branch.

DamienMcKenna queued 28: twitter-n2205277-28.patch for re-testing.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.