Unify flickr_*.module API, eliminate code duplication

i don't know what you mean about flickr_tags and flickr_sets... they're not in CVS...

Those are 2 modules that I wrote and submitted patches for. They were never committed and are out of date now, but IMHO when I get them working again they should be committed so we can get some testing from the community.

so don't go committing anything you don't have time to test

sorry to waste your time, I just wanted to get it in the queue in case I had no time to get back to it.

OK so the newest patch:

• is a working version of the patch above (i tested it)
• changes flickr_error to flickr_request_has_error
• puts spaces around =>...but I didn't see where there wasn't spaces around an =
• fixes the broken photosets block by adding a theme_flickr_block_photoset

Oh...and I checked flickr_filter and didn't find any API stuff that needed changing.

I actually agreed with you on that point 2 months ago...dejavu : http://drupal.org/node/116533

Let's get that committed and I'll rework this one.

The reason it takes tags as a separate array is so you don't have to worry about the flickr tags input syntax and implode them yourself. I personally don't care one way or another so I'll change that if you want.

Since we're moving so much code out of flickr_photos_get, I think we should turn it into a regularish flickr API call wrapper (flickr_photos_search in flickr.inc).

It would be something like this:

function flickr_photos_search($nsid, $page = 1, $other_args = array()){
  $args = array (
    'user_id' => $nsid,
    'page' => $page,
  );

  //set per_page to flickr module default if it is not specified in $other_args
  if (!isset($other_args['per_page'])) {
    $args['per_page'] = variable_get('flickr_photos_per_page', 20);
  }

    return flickr_request('flickr.photos.search', array_merge($args, $other_args));
}

What do you think?

I really like how this patch is shaping up. I think it's RTBC now. Here is a list of everything this patch does:

• Renames flickr_error to flickr_request_has_error
• Adds flickr_is_nsid and flickr_get_nsid
• Takes most of the stuff out of flickr_photos_get and make it flick_photos_search in flickr.inc
• Create a tag handling function to turn an array into request args. (Not currently used but will be in flickr_tags)
• Deletes module specific theme_photoset and adds theme_flickr_photoset_preview which is used by both block and filter
• Change flickr_block and flickr.module to use our shiny improved API (thereby fixing just about everything that was broken)

reading the code i realized that flickr_request_has_error() should be flickr_response_has_error() because we're checking the result not the request. how about simplifying it to:

I changed the name and simplified the function. Please make sure to check its logic because unless I'm more tired than I think I am, the logic in your snippet is incorrect.

maybe it'd make sense to just have them pass the call along to theme_flickr_photoset_preview and theme_flickr_photo so there's a default implementation.

Agreed, it's in the new patch.

also, what are your thoughts on my comment on #136542 in terms of checking for errors and returning the relevant array member in our API wrapper functions?

Yes. But lets get this patch committed first since it is getting big.

This function really doesn't like you, I think you meant this:

function flickr_response_has_error($response) {
  return !(array_key_exists('stat', $response) && $response['stat'] == 'ok');
}

Can you re-roll and then I will test and commit?

committed! i'll open another issue for the wrapper function changes we're making.