Closed (fixed)
Project:
Flickr
Version:
5.x-1.0
Component:
flickr (main module)
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
25 May 2007 at 03:45 UTC
Updated:
19 Jul 2007 at 19:53 UTC
Jump to comment: Most recent file
Comments
Comment #1
anj commentedI'd like to bump this issue, as I have tried installing both the 1.0 and 1.x iterations for the flickr module for Drupal 5, and neither seems to update properly. The lists of sets and photos have remained fixed, and have not been updated to reflect changes at flickr since the time the pages were enabled. This applies to all blocks, the site page (/flickr) and the user pages (/flickr/1 etc).
I suspect the caching system is broken, as the configuration parameter never updates. I'll just have a look...
Right, I just hacked off the caching system (sticking a '$cacheable = FALSE;' in flickr.inc/flickr_request) and everything started to update sensibly. There is some problem with the caching system, but I don't have time to finish debugging this right now, sorry.
Hope that helps.
Anj
Comment #2
anj commentedOk, well I looked into it anyway, and found that this was a 'feature'. The expiry time for the cache of flickr API calls is only advisory, and the caching system just ensures that the cache is kept for at least that long. The data will only get refreshed on the next cache wipe, which could take an awful long time if you use a reasonably aggressive caching policy.
The fix is simple. To enforce the cache time limit, instead of it being advisory, the line
in flickr.inc is made conditions, such that:
Thus, the cached data is only used if it is still 'in date'.
Hope that helps.
Anj
Comment #3
drewish commentedthe cache gets cleared when you create a node (or maybe even posting a comment). adding an explicit check isn't a bad idea.
Comment #4
andrewlevine commentedFor the most control of caching, there would be two configuration options: cache for at least x minutes and cache for no more than x minutes.
However, I get the feeling that more non-standard caching options wouldn't be used much and would create confusion. We are using the standard drupal caching mechanism. Shouldn't some other caching module have the job of tweaking this functionality?
So I'd rather keep it as it is unless there is something I am missing.
Comment #5
anj commentedI think the issue is that the internal cache system can know when it has fallen out of date, i.e. it can update the cache when a node is edited because it is all being done within Drupal.
With external content, the ideal update frequency is probably 'whenever the external source changes', but for many practical reasons the updates are configured to happen less frequently (bandwidth use etc). Looking at the code this way, the flickr module works no differently from the aggregator - drawing in content from an external feed. Therefore I suggest that the flickr module uses the same approach (and language) as the aggregator. IIRC, this means that we should configure an 'update frequency' instead of a 'caching time', although under the hood these are essentially the same thing, of course.
Anj
Comment #6
anj commentedHere's my suggestion in patch form. I hope you'll agree that following the aggregator mould is the path of least confusion, but I understand if you prefer your approach.
Thanks for your time.
Anj
Comment #7
drewish commentedi'm partial to andy j's solution but i'd like to get andrewlevine's feedback.
Comment #8
andrewlevine commentedI like andy j's solution best too. The patch looks fine although I haven't actually tested it.
Comment #9
anj commentedIt's a patch against the DRUPAL-5 branch, which is hopefully the right one. I've tested in on my own site, but that's all. More testing is wise, so I'm changing the status of this issue to 'patch (code needs review)' - I hope that's ok.
Andy J.
Comment #10
drewish commentedokay, i gave it a whirl and it looks good the only question i've got is does it require crontab? i don't think it does... so i've added one comment and removed that bit from the description. this is a simple enough feature that it's not worth dragging out any longer. i'm going to commit this. if there's something you don't like, re-open this and we can follow up.
oh, a quick note on the issue status, any time there's a patch attached and in consideration, feel free to flip it to "needs review". if someone looks at it and doesn't like it they'll change it to "needs work". if you review someone else's code, it looks good and tests out, then mark it as "ready to be committed". in core the standard for RTBC is a bit higher than in most contrib modules.
Comment #11
anj commentedAh, yes, you're completely right. Crontab is not required, as old data will be refreshed on demand. Sorry about that.
Cheers,
Andy J.
Comment #12
drewish commentedoh, i forgot to say thanks! it's very nice to have people point out your bugs and then submit a patch.
Comment #13
anj commentedNo worries - It's good to be able to help out. Thanks again.
Comment #14
(not verified) commentedComment #15
justinledwards commentedIs this going to be added to a release soon?
How do you apply patches?
I downloaded the flickr module yesterday for 5.X and I am still having this issue.