Provide page to show photos for flickr_default_userid
samo - February 6, 2007 - 18:06
| Project: | Flickr |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Provide a page at /flickr to show the photos for the flickr_default_userid. I can imagine some people who could just use this module to display a set of photos from Flickr without allowing the per-user flickr display.
1) refactor function flickr_photos_get to not lookup user information. flickr_photos_get should just take a user id. move the user-based functions into flickr_photos()
2) modify flickr_photos to handle the default_userid
3) add a new menu setting for just /flickr
4) tell the user why the flickr_default_userid textbox in admin settings is disabled when no valid api key is set.
| Attachment | Size |
|---|---|
| flickr-default-user.patch | 5.22 KB |
#1
Awesome, I was planning on doing this sooner or later but now I won't have to. :)
1. I like this change a lot.
2. Good.
3. Good.
4. Great.
I'm not sure if you are aware of this but one of your other patches is include in this one as well. Could you provide a patch with just the changes for this issue? Awesome stuff though, I'm going to test this ASAP and commit after drewish/wnorrix tests as well.
#2
I didn't have time for a deep review but looking at the patch the one thing that jumps out at me is that I'd use $uid=NULL rather than 0 as a default parameter.
#3
1) I removed the information from the other patch I submitted.
2) I changed uid === NULL on drewish's suggestion.
New patch attached.
--Sam
#4
Here is Samo's patch against latest head (1.29).
by the way Samo something was up with your patch I couldn't apply it to 1.27, had to manually do it.
#5
So this patch worked fine for me. I really like the idea of getting the 'drupal user' code out of flickr_get_photos. The only thing I added to the attached patch was a page title.
I do think we could move the default user id logic into hook_menu, but I will bring that up in another issue.
If we could get one more +1 (hopefully from drewish or wnorrix?) I will commit this.
#6
re-rolled for current head
#7
great, i've committed this (with a few small changes). it moves us in the right direction.
#8