Twitter TinyURL

Another guy hint to cache tinyurl results, your solution is more fast and light and plus it must work too without Global Redirect (later ill try again), the problem is just with long domain names or sub-domain.

Anyway i can provide another choice about twitter but without tinyurl usage, integrating ur patch!

Thank you again Rob :))

I agree the tinyurl.com hit can be brutal. :o I'm tossing a patch against 6-dev takes care of that and several other things. I would have broken it out into separate issues, but I need to wrap up the site I'm working on pronto.

Anyways take anything you'd like but I think it's all pretty worthwile. (obviously :))

1.) Fixes the URL for Yahoo bookmarks which currently re-directs and can cause double URL encoding
2.) Fixes the URL for Twitter which currently re-directs and can cause double URL encoding
3.) Adds two optional admin checkbox options: a) Use Tiny URLs for all service links b) Use Tiny URLs for Twitter
4.) Fixes the way Tiny URLs are used w/ Twitter (before it was changing $url so any service links constructed after Twitter got the Tiny URLs also, (messy).
5.) Changes what is sent into Twitter to make a little more sense IMO -> "Title -- http://link"
6.) Separates the query string from the URL so it can be correctly tossed into the theme call as a 'query' option.

I just read your re-factoring post (oops :) ), but I do think some of this should fit well. Either way, I needed it cleaned up here quick...

Thanks

Hey Moonshine, great work, just i have some doubt:

#5:

putting Title + Url the url will trunk if it is too long, maybe, at least, is better to save the url than the title;

#6:

is not better split the address around ? instead to add another param (overwriting 17 function call)? Too for 2.x branch it will be more easy for write a service instead add other params.
The code below is correct?:

if ($query_string) {
  $url = $url .'&'. $query_string; // < is not "$url = $url .'?'. $query_string;" ?
}

The 3 fixes (#1, #2, #4) are committed;
The #3 is a great idea for 2.x branch, in 1.x i duplicated the item for discard tinyurl usage with twitter;
The #6 is pending :P

Thank you!

Howdy!

#5:

The original Twitter patch already had both the url & title being submitted, it was just forced to use tinyurl and put the url first. But your right, Twitter posts are very small. :) Currently 140 characters, so I suppose the original method of "url" then "title" is safer. I guess I'd like to just see a break in there of " -- " or something, but I can just do that here on my end.

The real problem for me is just the TinyURL hit on every set of service links. I'd certainly like to use a very small url, but request hit every load will be to much for the site I'm working on. :) But really that leaves me with either caching or possibly doing some url redirect method as part of the module for a path like n/$nid. If people have a reasonably short domain name that would be a nice solution. But really that only buys 3 characterss off of node/$nid anyways :)

#6:

You're correct on the ? rather then &. :) Sorry about that, fast fingers here trying to wrap this up. Heh. But really IMO that lower section should all be l() calls anyways, rather then building them by hand. The upper section (where $nodelink == true) all gets pass through l() via theme_links() where it's sent.

The idea behind the change is that both l() (and therefore theme_links() which calls l()) make use of a 'query' option to attach a query string to the base url. As it's part of the function, so it's technically more correct to use it. Although it does "work" either way. :)

Thanks..

Just tossing this out there as a thought... Going to use for short URLs here, obviously in another module here, but... :)

function gigglefuel_menu() {
  $items['n/%'] = array(
    'page callback' => 'gigglefuel_tinyurl',
    'page arguments' => array(1),
    'access arguments' => array('access content'),
    'description' => 'Tiny URLs for use in Service Links',
    'type' => MENU_CALLBACK
  );
  return $items;
}

function gigglefuel_tinyurl($nid) {
  drupal_goto(drupal_get_path_alias("node/$nid"), NULL, NULL, 301);
}

heh... yes.. it's a site for the children's industry ;)

url() looks to call drupal_get_path_alias() which in turn calls drupal_path_lookup(). I think it's safe to jump in at the drupal_get_path_alias level, as we know we're after and alias and if one isn't found it should just return the path node/$nid. Not that we're shaving ms here. :) url() is certainly less obscure, so maybe it's better to go with that for readability.

I agree, I think it should just be conditionally included based on some admin settings for Tiny URLs. It's definitely the option I'd use ,rather then nailing tinyurl.com with 25k pages :o

I'd offer up a new patch here if TheCrow is interested, but don't want to jump on any v2 plans he has..

Ok guys give me some time for reorder the ideas about all the refactoring plan, anyway the short url will be available as option in settings menu :))

@Moonshine i tought the original author didn't used l() for some reason, i changed in the 2.x and seem work well! About Twitter, the separator '--' is already available in 1.x-dev version

Thank you!!!

How 'bout creating a little detour for the links. Each service link would point to a local page that would format the appropriate path whether it's a tinyurl.com path or something else and use that. This way the call to tinyurl.com only happens when a service link is clicked.

Just a quick suggestion for a necessary fix, however the url is generated for Twitter. The code that generates the tinyurl currently overrides following service links' urls ... thus Buzz Up! LinkedIn etc. all get tinyurls too.

Here's a simple fix starting at line 381:

if (variable_get('service_links_show_twitter', 0)) {
    $turl = drupal_http_request('http://tinyurl.com/api-create.php?url='. $url);
    $url = isset($turl->data) ? $turl->data : urldecode($url) ;
    
    $links['service_links_twitter'] = theme('service_links_build_link', t('Twitter'), "http://www.twitter.com/home/?status=$url". "+". $title, t('Share on Twitter.'), 'twitter.png', $nodelink);
  }

becomes:

if (variable_get('service_links_show_twitter', 0)) {
    $turl = drupal_http_request('http://tinyurl.com/api-create.php?url='. $url);
    $turl = isset($turl->data) ? $turl->data : urldecode($url) ;
    
    $links['service_links_twitter'] = theme('service_links_build_link', t('Twitter'), "http://www.twitter.com/home/?status=$turl". "+". $title, t('Share on Twitter.'), 'twitter.png', $nodelink);
  }

See also #445962: Twitter links broken in DRUPAL-5 due to faulty backport (using the D6 API for url() in D5).

Also, it strikes me as confusing UI that you have separate checkboxes for the "display twitter link" and "display twitter with tinyurl support", but in practice, you only see one twitter service link, and if the tinyurl checkbox is selected, that's what you get (since it's overwriting the $links['service_links_twitter'] array element). Seems like you really just want a single checkbox for "Display twitter link" in the big pile of checkboxes, and then a separate set of radio buttons or a drop-down selector about what kind of twitter links you want to generate (tinyurl, node/N, full alias, etc).

Just a mistake in the second array id, but about what we really did is explained here... read all please!

@TheCrow: I did read. I'm saying that the UI is a little weird, that's all. You're never going to want 3 different (undifferentiated) twitter links showing up. You either want a twitter link or you don't. Once you have a twitter link, you need some options to control what the default "tweet" it creates for you looks like. You don't want N different twitter checkboxes for this. That doesn't scale well at all, especially once things get more complicated (like use node/N links instead of tinyurl, etc). Please don't get defensive, I'm just reporting problems as I uncover them and providing patches where appropriate.

Cheers,
-Derek

removed it from 1.x branch, this will be just 2.x feature