Add hook_media_rendered_flush() [#1032186]

This patch adds a helper function, media_invalidate_cache() which replaces all the spots where media_filter_invalidate_caches() was previously called (that function is now called by the helper).

It also invokes a new hook, hook_media_invalidate_cache() - this will help #986084: Add cache for media_gallery_block_view().

I vacillated a little bit whether to make media_filter_invalidate_cache() a hook implementation or not (i.e. media_media_invalidate_cache()), or whether to rename it to singule (media_filter_invalidate_cache()) - but the first doesn't hold any benefit compared to calling it directly, and the latter is namespaced exactly as if there was a media_filter.module implementing the hook. These both seemed worse than leaving it as is, but I could be persuaded.

Comment	File	Size	Author
#5	1032186_hook_media_rendered_flush.patch	14.91 KB	catch
#4	hook-media-rendered-flush-1032186-4.patch	15.58 KB	David_Rothstein
#3	hook_media_rendered_flush.patch	14.88 KB	catch
#3	interdiff.txt	2.32 KB	catch
#2	hook_media_rendered_flush.patch	14.86 KB	effulgentsia
#1	hook_media_invalidate_cache.patch	4.37 KB	catch
	hook_media_invalidate_cache.patch	3.73 KB	catch

Comments

Comment #1

catch

he/him

English

commented 19 January 2011 at 06:33

Status	File	Size
new	hook_media_invalidate_cache.patch	4.37 KB

Missed a spot in media.fields.inc...

Comment #2

effulgentsia commented 21 January 2011 at 02:48

Title:

Add hook_media_invalidate_caches()

» Add hook_media_rendered_flush()

Status	File	Size
new	hook_media_rendered_flush.patch	14.86 KB

How about calling the function and hook media_rendered_flush(), for consistency with image_style_flush(), and inlining the filter cache clearing?

This patch also contains some code organization and docs cleanup. Can you review to see if I understood and explained sufficiently accurately and clearly?

Comment #3

catch

he/him

English

commented 21 January 2011 at 03:23

Status	File	Size
new	interdiff.txt	2.32 KB
new	hook_media_rendered_flush.patch	14.88 KB

The function and hook name changes seem fine to me.

I tweaked some of the comments and found some minor whitespace issues. Very small changes, attaching updated patch and interdiff.

Comment #4

David_Rothstein commented 11 February 2011 at 22:41

Status	File	Size
new	hook-media-rendered-flush-1032186-4.patch	15.58 KB

The patch no longer applied, so here is a reroll.

I also reviewed it and it seemed fine to me. Took a little while to unravel what was going on (with the code moving around), but it makes sense to me. However, I thought the @todo for media_filter_usage_prune() needed some work. I didn't understand it at first, and while I understood the original comment that it was replacing a bit better, that didn't seem complete either :) So I rewrote it a bit in the attached patch. You might want to review that part; otherwise this should be the same patch as before.

Comment #5

catch

he/him

English

commented 14 February 2011 at 02:35

Status	File	Size
new	1032186_hook_media_rendered_flush.patch	14.91 KB

The new @todo looks fine.

I noticed we had a WHERE fid > 0 in media_filter_usage_has_records(), shouldn't be any need for that so took it out, otherwise the patch is exactly the same as #4.