View Issue Details

IDProjectCategoryView StatusLast Update
0027200mantisbtbugtrackerpublic2020-09-05 15:57
ReporterBasCostBudde Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Product Version2.24.1 
Summary0027200: bugnote_update fires EVENT_BUGNOTE_EDIT but does not clear the note cache
Description

When a plugin I wrote tried to do something with the updated note, I kept getting the old values.

Suggested fix in bugnote_update.php, right after line 80

bugnote_set_time_tracking( $f_bugnote_id, $f_time_tracking );
+ unset($GLOBALS['g_cache_bugnotes_by_id'][$f_bugnote_id]);
TagsNo tags attached.

Relationships

related to 0021876 closedvboctor Email notifications for notes shouldn't include full issue information 
related to 0021884 closedcproensa Use meaningful names for bugnote cache global variables 
related to 0027217 closeddregad bugnote_clear_cache() does not work properly 

Activities

dregad

dregad

2020-09-02 07:17

developer   ~0064338

Accessing the global variables directly is not the right way of doing this; you should be calling bugnote_clear_cache() instead, like so:

diff --git a/bugnote_update.php b/bugnote_update.php
index 414a0fe62..df204d7d4 100644
--- a/bugnote_update.php
+++ b/bugnote_update.php
@@ -78,6 +78,7 @@ $f_bugnote_text = trim( $f_bugnote_text ) . "\n";

 bugnote_set_text( $f_bugnote_id, $f_bugnote_text );
 bugnote_set_time_tracking( $f_bugnote_id, $f_time_tracking );
+bugnote_clear_cache( $f_bugnote_id );

 # Plugin integration
 event_signal( 'EVENT_BUGNOTE_EDIT', array( $t_bug_id, $f_bugnote_id ) );

That being said, to ensure consistent behavior I'm wondering if it would not be cleaner to clear the cache in the bugnote API functions performing updates, i.e. bugnote_set_text(), bugnote_set_time_tracking(), bugnote_set_view_state() and probably bugnote_delete() as well.

BasCostBudde

BasCostBudde

2020-09-02 10:24

reporter   ~0064339

The bugnote_clear_cache call, while obviously the way to go instead of meddling with globals, does not remove the entry from the bugnoted_by_id cache structure :( should that be updated in the call as well then?

dregad

dregad

2020-09-02 11:02

developer   ~0064340

The bugnote_clear_cache call [..] does not remove the entry from the bugnoted_by_id cache structure

I didn't actually look at the code, but it sounds like a bug in bugnote_clear_cache().

dregad

dregad

2020-09-02 12:25

developer   ~0064341

Here's what happens with bugnote cache functions:

  • bugnote_clear_cache() calls bugnote_clear_bug_cache() -- That makes sense because if an individual bugnote's cache becomes invalid, then we'll also want to make sure it is reloaded from DB the next time bugnote_get_all_bugnotes() is called.
  • bugnote_clear_bug_cache() loops over all the bugnotes cached for a given bug id ($g_cache_bugnotes_by_bug_id) and removes the corresponding individual bugnote cache items ($g_cache_bugnotes_by_id) -- I'm not sure it is strictly necessary or useful to do this, but other than a potential performance degradation, it will not hurt.

So the logic only works as long as the bugnote has been stored in $g_cache_bugnotes_by_bug_id, but fails in the scenario where the bug-level cache has not been populated.

This can easily be reproduced and confirms the bug :

include 'core.php';
$id = 1234;
$note = bugnote_get( $id );
bugnote_clear_cache( $id );
isset( $g_cache_bugnotes_by_id[$id] );   # true -> cache has not been cleared

bugnote_get_all_bugnotes( $note->bug_id );
bugnote_clear_cache( $id );
isset( $g_cache_bugnotes_by_id[$id] );   # false -> now we're ok
dregad

dregad

2020-09-02 12:33

developer   ~0064342

I opened 0027217 to track the bug in bugnote_clear_cache().

dregad

dregad

2020-09-02 13:04

developer   ~0064344

@atrol @cproensa @vboctor your thoughts on where to put the clear cache calls (see 0027200:0064338) would be appreciated

atrol

atrol

2020-09-05 15:57

developer   ~0064367

I'm wondering if it would not be cleaner to clear the cache in the bugnote API functions performing update

+1 for that