Posted by alex_b on July 26, 2009 at 9:46pm
| Project: | Chaos tool suite (ctools) |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | sdboyer |
| Status: | closed (fixed) |
Issue Summary
I have a situation where once loaded objects are being deleted and potentially recreated during a page load with the same key. In this scenario ctools_export_load_object() is returning wrong results because they're static cached ($cache, $cached_database).
I would like to propose to add a $reset flag to the function's parameter list.
Patch coming.
Comments
#1
#2
Definitely useful, but it needs to be more granular. Resetting the entire cache, including tables that you're not concerned with, is probably wasteful, and potentially even damaging to other modules using the object cache. Would you mind rerolling with a more complicated $reset parameter that allows either a) a full reset of the cache or b) selective resets? Off the top of my head, it seems like using a simple array with the names of the caches to be reset ought to be sufficient.
#3
It's much more fine-grained now.
#4
Still not quite what I'm looking for. Really doing this properly may well entail a separate function where the static caches are contained that has more get/set style parameters. That way we can separate the granular cache clearing from the loader itself.
Meh, I may just write this myself real quick.
#5
/*** Handles the static cache for the exports.
*
* @param $op
* 'set' or 'get' - Together with $table.
* 'set': Marked this table fully cached
* 'get': Returns TRUE if the table is fully cached
* @param $table
* Name of the database table to set a table ready or to adjust the scope of cache retting.
* @return
* The reference of the current cache data.
*/
function &ctools_export_static_cache($op = NULL, $table = NULL, $reset = FALSE) {
Is this function signature something that you'd like to see?
In ctools_export_load_object(), i'd eliminate the two static variables and encapsulates the functionality in one function.
If you like this direction, i can post a patch, otherwise, if you give me more detailed instructions, i happily implement that direction as well.
#6
/me smiles
Yup, that's exactly the kind of thing I had in mind. I just ran out of time. Roll me a patch with that sort of function signature, and I'll put it right in.
#7
Here is the patch.
#8
The approach in #7 is very specific to ctools_export_load_object(). Further, there are some problems (return FALSE in a function that returns by reference, not sure whether the correct static cache (there are 2) is always returned.
I refactored with http://api.drupal.org/api/function/drupal_static/7 in mind. ctools_static() and ctools_static_reset() are backports of Drupal 7's drupal_static() and drupal_static_reset(). ctools_export_load_object() uses these functions.
While this definitely helps the problem I initially described in this issue, I am not 100 % happy with moving resetting from a flag in the function signature of ctools_export_load_object() to an external static cache because it means that implementing modules will have assumptions about ctools_export_load_object()'s guts.
Using ctools_static() AND a reset flag in ctools_export_load_object() would be my preferred way: ctools_static() makes sense because it opens the possibility to do a global reset with ctools_static_reset(NULL) while the internal static caches in ctools_export_load_object() stay encapsulated.
#9
I'll have a look at this and respond in detail tomorrow. Thanks!
#10
Thought more about this.
Instead of a $reset flag, we should use a separate function to reset internal static caches.
This patch...
- adds ctools_static() and ctools_static_reset() modeled after drupal_static() as in #8
- makes ctools_export_load_object() use ctools_static() as in #8
- adds ctools_export_load_object_reset() to reset internal caches in ctools_export_load_object()
#11
Um, yeah: patch for #10
#12
Is this modelled after the static system that's in D7? (I ask because I'm not familiar enough with it to just look at the code and see).
#13
#12: it is. See links in #8. #8 starts a different approach from the patches above.
#14
I'm happy. Almost completely happy.
Only problem is that we're still just globally clearing the export cache. Give me some changes to
ctools_export_load_object_reset(), or a separate function for granular cache resets, and I'll commit.#15
Great. Function signature is now
ctools_export_load_object_reset($table = NULL);#16
Same patch for DRUPAL 7.
#17
I like this! Committed to both branches.
#18
Awesome!
#19
Automatically closed -- issue fixed for 2 weeks with no activity.