[Twitter Bridge] Don't send @-less replies to Twitter

@ssweeny Sorry, that wasn't obvious. Over the weekend both me (status.davidhaberthuer) and bavatar (http://diekershoff.homeunix.net/statusnet/bavatar) had troubles with twitter mirroring. maybe it's also related to this.

I'm also experiencing this issue.

Same here with my own instance.
Have no idea of which part of the code is concerned.

OK, think I've found a first problem:
line 124 from file "plugins/TwitterBridge/twitter.php" we have:
($flink->noticesync & FOREIGN_NOTICE_SEND_REPLY == FOREIGN_NOTICE_SEND_REPLY)

This may turn into problem since "==" operator has precedence on "&" operator.
That means that PHP will first evaluate "FOREIGN_NOTICE_SEND_REPLY == FOREIGN_NOTICE_SEND_REPLY" which is obviously TRUE and then evaluate "$flink->noticesync & TRUE", which I guess always returns TRUE.

So, solution could be to add parenthesis like:
( ($flink->noticesync & FOREIGN_NOTICE_SEND_REPLY) == FOREIGN_NOTICE_SEND_REPLY)

But, performaing some other tests, I found that is_twitter_notice($notice->reply_to) was sometime wrongly evaluated as TRUE.

On concerned notice was indeed a reply, involving myself, @evan (who I'm subscribed to) and @march (who I wasn't subscribed to). So I think we may have another problem in "is_twitter_notice" function. But stating this function is almost very simple, problem could be in "Notice_to_status::staticGet('notice_id', $id);".

Don't know exactly yet, I'll investigate that later in the day.

Why didn't TwitterBridge plugin worked as expected
**************************************************
I recently ask for merge request 179 to partly fix this bug. Here are some detailled explanation about what was actually wrong.

Before merge-request 179
========================
https://gitorious.org/statusnet/mainline/merge_requests/179

3 checks to know if a notice should be sent or not:
1. !preg_match('/^@[a-zA-Z0-9_]{1,15}\b/u', $notice->content)
2. ($flink->noticesync & FOREIGN_NOTICE_SEND_REPLY == FOREIGN_NOTICE_SEND_REPLY)
3. is_twitter_notice($notice->reply_to)

As stated in code comments, the idea was to sent to Twitter:
- Notice NOT beginning with at-mention
- All notice if the user WANTS to send replies
- Twitter replies

First problem: assumption that notice beginning with at-mention are NOT twitter replies is quite... dangerous. By the way it worked because of last check.
BUT, with Statusnet version 1.x coming, things changed: with new web UI, replies now do not begin with at-mention anymore. As a consequence, !preg_match was ALWAYS evaluated as TRUE and... local replies are sent :(

Second problem was, old-style desktop or mobile client still uses at-mentions beginning notices for replies. Since !preg_match was evaluated as FALSE, second check was performed.
We touch the heart of the bug: PHP operator's precedence (http://www.php.net/manual/en/language.operators.precedence.php). In other words, "==" operator is to be evaluated BEFORE "&".
Therefore, second check can be changed as: ($flink->noticesync & TRUE); This, basically, is ALWAYS evaluated as TRUE and... local replies are sent :(

After merge request 179
=======================
The only change I made was adding parenthesis to restore correct precedence for second check.

This way, I JUST RESTORE v0.9.x BEHAVIOUR. Not more.

But we still have a problem:
Since (($flink->noticesync & FOREIGN_NOTICE_SEND_REPLY) == FOREIGN_NOTICE_SEND_REPLY) is now evaluated the right way, no more problem with old-style clients, but still one with WebUI (see above).

And now ?
=========
Now, I want to get rid, as much as possible, of this !preg_match check. Here are the use cases I've identified:
[pseudo-code]
Do we allow twitter export ?
yes:
Do we allow local replies to be sent ?
yes: send it.
no: Is the notice a Twitter reply or repeat ?
yes: send it.
no: Is the notice a 'new' one (ie not a reply)
yes: send it.
no: does the notice NOT begin with at-mention ?
yes: send it.
no: DON't send it.
no: DON't send it.
[/pseudo-code]

Here's the corresponding code I'm currently testing on my instance:

function is_twitter_bound($notice, $flink) {
if ($notice->object_type == ActivityObject::ACTIVITY) {
return false;
}
$allowedVerbs = array(ActivityVerb::POST, ActivityVerb::SHARE);
if (!in_array($notice->verb, $allowedVerbs)) {
return false;
}
if (!empty($flink) && (($flink->noticesync & FOREIGN_NOTICE_SEND) == FOREIGN_NOTICE_SEND)) {
if ( ( ($flink->noticesync & FOREIGN_NOTICE_SEND_REPLY) == FOREIGN_NOTICE_SEND_REPLY ) ||
( is_twitter_notice($notice->reply_to) || is_twitter_notice($notice->repeat_of) ) ||
( empty($notice->reply_to) && !preg_match('/^@[a-zA-Z0-9_]{1,15}\b/u', $notice->content) )
) {
return true;
}
}
return false;
}

This way, I'm now able to catch replies sent from web UI. Those ARE replies, but not beginning with at-mention. Here, they won't be sent to Twitter.

"But, you wanted to get rid of preg_match and you still use it ?"
- Yes, that's right, and here's why: I sometimes need to be able to sent a notice to specific people on statusnet. For that, I begin my notice with at-mention. But I usually don't want this notice to be send to Twitter. That's why I keep preg_match in the code. And, yes, I could have use same tips to be able to sent "direct mention" to Twitter and to StatusNet but... Since I use StatusNet, I just prefer to give it an higher priority :)

Hmmm... right, but that's IMHO another topic.
The function I fixed there just check whether or not it should sent notice to twitter. It's called by another function "broadcast_twitter" in plugin/TwitterBridge/twitter.php

Since this function also deals with repeat (ie build retweet), adding @-nickname should occurs there.

The problem is that:
- we'll have to deal with both new web UI, which does not add @-nickname, and "old style" clients, which do.
- adding @-nickname could make notice length more than 140 caracters. Then Twitter will through an error with which we'll have to deal as well (and besides that, this will be confusing for users)

Another solution could be to add @-nickname in web UI, but this has also drawbacks: this will make UI javascript code a bit more complex.

Just to second Jean, I've had his patch running on my instance as well, for nearly as long, without issue. Works great. :)

A similar bug:

[Twitter Bridge] local '@' replies beginning with @ disrespect the user preference to not cross-post
http://status.net/open-source/issues/3703