I was tracing bugs in the interaction between your module and the shorturl service, when I discovered something bigger.

Let me cut to the 'something bigger' first.

hook_shorten_service() does two things at the same time and (ideally) it shouldn't. This is confusing and 'conceptually wrong'. It does
* announcing to the caller, what services there are (without the caller having specified that the service is going to be used)
* shortening URLs (i.e. using the service).

The second thing is done for all services where 'custom'=TRUE. When a program asks if a service exists, those services are 'forced' to shorten a URL and return it, even though it probably will not be used. And the caller is forced to wait for the time those services take.
(Also, a nitpick: For services where 'custom'=TRUE, the 'url' key means "a shortened URL". For other services, the 'url' key means "a template string used to ask for the shortened URL later". These are two different concepts, and that's confusing to a new user who's trying to make out what the hook_shorten_service() example on the project page means.)

When looking at it, I get the idea that the whole '$original' argument to hook_shorten_service() should be removed, because nothing should be doing the actual conversion at that point.

But I don't really know what you mean with "services requiring $_POST or JSON" in the example on the project page, and I'm not a j.mp user, so I won't make assumptions about that one.

However...

---

getting back to the shorturl service bugs:

* When going to admin/settings/shorten, hook_shorten_service('') is called**, which calls shorturl_shorten(''). This function inserts a shortened URL with the target '' in its table, and the first time that you're actually using the shorturl service, your target URL is NOT remembered (because it reuses the 'source-shortened-url' that points to ''). So the short URL can't be used. Bug.

* Even when you don't want to use the shorturl service (because you haven't configured it in admin/settings/shorten)... if it is enabled, your table still fills up with URLs (that are also handled by other configured services).

This can be fixed:

Don't shorten URLs on a hook_shorten_service() call. Rather, return a function that can be called to shorten URLs later, by _shorten_fetch_url().

See attached patch.
I extended the use of 'custom'; you can give a function name.
Any string that is not "xml", will be made into a function call if possible.
(I don't have an opinion if that should be done in 'custom' or in another value...)

By the way,
- I do not fill $api['args'] anywhere; you can delete it if you don't like it -- I just thought that might be a good 'general thing' for a specification... (It's the second-last argument to the custom function; the first is the original URL. $api['args'] must be an array, otherwise it's ignored.)
- I reshuffled some code so that the hook isn't called twice unnecessarily. (Especially nice when the j.mp fetch is still in there.)

** and $original is undefined in shorten_admin(), by the way.

CommentFileSizeAuthor
shorten-hookservices.patch3.65 KBroderik

Comments

icecreamyou’s picture

icecreamyou commented
Status: Active » Needs work

First of all, you got your patch from #628086: Don't use url() to create 'shorturl' URLs mixed up in there.

Anyway, I acknowledge the problem and appreciate your efforts to solve it, but I'm not sure I agree with the approach. I'll review this patch this weekend and see what I can do.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Thank you. The point was mainly that I wanted to separate the 'url shortening work' from hook_shorten_service(), and solve some issues at the same time. I did that by returning a function name and calling that function later (and I'm using this code on my own site now), but there may well be a better approach.

(Or at least a better way of using keys in $api[], than using 'custom' like I did.)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I think we should make it so the service arrays either contain a 'shorten_callback' element or an URL that is the actual function for shortening URLs. Or maybe something like hook_menu() where we would have a callback function and callback arguments. The default callback function could be shorten_fetch().

icecreamyou’s picture

icecreamyou commented

So, sorry, obviously I didn't get to this this weekend. I was busy getting the 2.0 release of Facebook-style Statuses out the door. I will try to get to this as soon as I can though.

@Dave, I'm not sure of the route I want to take here, but I do know that I want to make it as simple as possible. Every time I use hook_menu() I have to go look up the keys it takes, and I like that the current simplicity of the Shorten URLs API just requires passing a name and a URL as a string. Custom URLs are a different story, but I want it to stay as simple as possible for the basic use case. For example, in hook_menu(), if my desired URL is 'admin/settings/mymodulename', I'm probably gonna want this:

$items['admin/settings/mymodulename'] = array(
  'title' => 'MyModuleName',
  'description' => 'Settings for MyModuleName',
  'page callback' => 'drupal_get_form',
  'page arguments' => array(),
  'access callback' => 'user_access',
  'access arguments' => 'administer site configuration',
  'type' => MENU_NORMAL_ITEM,
  'file' => 'mymodulename.admin.inc',
);

(Of course, of those, page arguments, access callback, and type are already assumed, and file is probably not safe to assume.)

icecreamyou’s picture

icecreamyou commented

Long story short, I have concluded that I like the essentials of the original proposed solution, and I'm hoping to implement it by the end of this weekend if not sooner.

icecreamyou’s picture

icecreamyou commented
Status: Needs work » Fixed

I've just committed a solution similar to roderik's patch. I'm rolling a 6.x-1.5 release which should be out with the changes soon.

Status: Fixed » Closed (fixed)

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