The module now respects cache really well and Geonames data is well-suited for long-term caching, but Drupal (especially during development) often clears the whole cache. Are there hidden costs to creating a new cache table? This can remain configurable, but I think it's very easy to add a cache_geonames table to the existing schema.
$schema['cache_geonames'] = drupal_get_schema_unprocessed('system', 'cache');
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | geonames-default-cache.patch | 3.36 KB | Anonymous (not verified) |
| #2 | geonames-default-cache.patch | 2.99 KB | Anonymous (not verified) |
Comments
Comment #1
2xe commentedthis is definitely a good idea.
Comment #2
Anonymous (not verified) commentedHere's a patch. For users who have already set up and changed their cache table from the default, that cache table will still be used. For everyone else, the new cache_geonames table is used by default.
Users need to re-install the module for this cache table to be created, unless you think I should make a hook_update_N that does this.
Comment #3
Anonymous (not verified) commentedComment #4
lyricnz commentedYes, I think the module should be able to create the table on the fly, too.
Comment #5
Anonymous (not verified) commentedWhat do you mean by on the fly? The patch creates the cache table when the module is installed. The system settings form no longer exposes the cache table variable to the user. If the user wants to use another cache table, he's going to have to create the cache table himself like he always had to.
Comment #6
lyricnz commentedI mean during module upgrade (update.php - hook_update_N etc). Requiring the user to uninstall and reinstall is unnecessary, for the couple of lines the update would take.
Comment #7
Anonymous (not verified) commentedThanks for clarifying. I'll reroll the patch today with a hook_update_N in it.
Comment #8
Anonymous (not verified) commentedNew patch! only 12 days late.
Comment #9
lyricnz commentedNooooo...
Please just create the new table explicitly, without uninstalling and reinstalling the schema (which will loose all the data loaded at upgrade time). AFAIK, you can-not/should-not call these methods from a hook_update.
http://api.drupal.org/api/function/hook_update_N/6
Comment #10
Anonymous (not verified) commentedThe schema is already different due to the patch #334726: Format of countryInfo.txt has changed, breaking geonames cache which contained no hook_update_N for those schema changes.
All the tables in the schema for Geonames are essentially cache tables. They're regularly rebuilt. The advice not to use hook_schema (and by association drupal_install_schema) in hook_update_N comes from the assumption that the schema's tables contain unique data you need to keep for your site to function. That assumption doesn't apply to Geonames.module.
I can write a hook_update_N to bring the tables up to date, but it's a lot of tedious work to preserve data that gets replaced on a monthly basis anyway. Are you sure it's justified in this case?
Comment #11
lyricnz commented1) #334726 should not have been committed without hook_update_n(). That's my fault, and I should have caught it at the time. I'll write that when I get a chance.
2) I don't think we should be taking liberties with deleting users' data (be it cache or otherwise), and it's kindof a bad precedent to be setting. Consider the performance impact on production site with tens thousands of cached entries.. In this particular case (changing cache table) we bust the cache anyway, but I'm talking about generally.
Uuugh, CVS is a mess right now - two diverging branches, and no real releases in quite some time.
Eg: #338771: Add support for loading Geonames Datafiles into local DB - not just a cache.
Comment #12
lyricnz commentedAlready done in #576112: Create and use a separate cache table by default (though I forgot about this ticket, and reinvented the code)