Filter system doesn't communicate with modules about altered text formats

Additional issue about Field API 'text fields' is that :
- a mass update can be expensive
- you cannot even assume SQL storage...

Roadmap suggestion:

- add a hook_filter_delete()
- implement hook_filter_delete() in node.module, comment.module, block.module and text.module (is there others?)
- possibly, make customer modules form_alter to the filter deletion confirmation form in the case deleting a filter could lead to potential disasters

Deletion is only part of the picture. Any possible content caches need to be flushed whenever a text format is altered in any way. Enabling/reconfiguring/disabling filters and deleting a text format both must trigger this hook.

Additionally:

- Page cache must be flushed
- Block cache must be flushed
- Search index must be reset (contains not only indexed content itself, but also excerpts of filtered [and strip_tags()'ed] content)

So we probably need hook_filter_update($format) and hook_filter_delete($format).

drupal_flush_all_caches() would be an alternative to hook_filter_update(), but that would flush each and everything, while we only want to flush the cache for affected stuff.

.

This is also needed for #369011: Performance: do check_markup() in text_field_load().

Attached patch adds the two hooks next to the cache_clear_all() - I didn't add documentation or attempt to clear up any of the existing mess there - which also means that the _update hook gets invoked on insert as well. Not planning to work on this beyond here but wanted to see this in the patch queue instead of active.

subscribe

Here, the 'object' being altered is a format (text format) not a filter, why the hooks are called hook_filter_update($format) and hook_filter_delete($format), instead of

hook_format_update($format)
hook_format_delete($format)

Attached patch uses hook_format_* instead of hook_filter_*, implements the hooks in block.module, comment.module and text.module, and add hooks documentation to filter.api.php

There still a TODO in text.module. Fields in core experts required ;)

Hook names need to be in the namespace of the providing module. So if you want to use *format_*, then you have to use hook_filter_format_*().

Ok, here is the patch using hook_filter_format_* and fixing the exceptions reported by the tests.

Tagging.

"DX" tag: this is not only DX, it's a condition for text Fields to work properly.

+++ modules/comment/comment.admin.inc	22 Jul 2009 21:03:17 -0000
@@ -292,3 +292,15 @@
+	$default = variable_get('filter_default_format', 1);
+  // Replace existing instances of the deleted format with the default format.

Indentation issue.

+++ modules/filter/filter.admin.inc	22 Jul 2009 21:03:21 -0000
@@ -261,10 +261,13 @@
+  	
+    // If a new filter was added, return to the main list of filters. Otherwise, stay on edit filter page to show new changes.
+++ modules/field/modules/text/text.mo

Exceeds 80 characters.

+/**
+ * Implement hook_filter_format_delete().
+ */
+function text_filter_format_delete($format) {
+  $default = variable_get('filter_default_format', 1);
+  // @TODO: Replace existing instances of the deleted format with the default format. (Fields in Core expert required)
+  
+} 

I think this should be a (critical, bug), followup issue rather than a TODO.

All the changes here look really sane and those nitpicks are very minor. I think we could do with some very basic tests like some of the other hook tests in core (set a variable if it's required, check the value before and after, that sort of thing) - or maybe do that as part of the text.module implementation. Apart from that it's ready to go.

It's not required by the use cases we currently have, but is it possible/worth adding hook_filter_format_insert() separate to _update()?

I'm on crack. Are you, too?

Thanks catch for the review.

Updated patch.
- Fixed minor issues pointed by catch.
- Added hook_filter_format_insert(), for consistency, though there is no use case in core.
- Added test module filter_test.module to test filter hooks that are not implemented in core.
- Added tests for the hooks introduced.
- Remove incomplete implementation of the hook in text.module, it's better to fix that in a separate issue: #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do

Updated patch, rerolled for #556832: Add text format API to manipulate formats.

Forgot to add hook_filter_format_delete(), and there was a test for it already!

I'll try to re-roll this.

+++ modules/filter/filter.test	26 Aug 2009 05:22:03 -0000
@@ -746,3 +746,73 @@
+      'group' => 'Filter'

Missing trailing comma.

+++ modules/filter/filter.test	26 Aug 2009 05:22:03 -0000
@@ -746,3 +746,73 @@
+    $this->drupalLogin($admin_user);  	

(and elsewhere) Trailing white-space here.

+++ modules/filter/filter.test	26 Aug 2009 05:22:03 -0000
@@ -746,3 +746,73 @@
+}
\ No newline at end of file

(and elsewhere) Please always add a newline to the end of files to avoid this diff message.

+++ modules/filter/filter.api.php	26 Aug 2009 05:21:51 -0000
@@ -117,5 +117,64 @@
+ * 
+ * @see hook_filter_format_update().
+ * @see hook_filter_format_delete().
+ *
+ * @param $format
+ *   The format being updated.
+ *
+ */

(and elsewhere) Please remove the empty blank line.

Also containing trailing white-space.

@see should come last. Only put @see right below a PHPDoc paragraph, if the paragraph refers to some really special stuff that's not tightly coupled with the function at hand.

+++ modules/filter/filter.api.php	26 Aug 2009 05:21:51 -0000
@@ -117,5 +117,64 @@
+ *  *  
+ * @param $format
+ *   The format being deleted.
+ *
+ */
+function hook_filter_format_delete($format) {

Something weird going on here. ;)

+++ modules/simpletest/tests/filter_test.info	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,8 @@
+name = "Filter test module"
+description = "Tests filter hooks and functions."

We can remove the quotes here.

Beer-o-mania starts in 5 days! Don't drink and patch.

Fixed those issues and

- Reworked filter_format_delete() to take a format object (instead of the format id) like the other CUD functions.

- Reworked filter_admin_delete() accordingly - another arg() bites the dust - yay! :)

- Reworked hook_filter_format_delete() to pass on the default format (object), to make it easier for modules to perform their updates and decrease the chance that someone retrieves a wrong default format.

RTBC if the bot comes back green.

sorry, silly typo in that last patch.

Fixing the tests and also

- Changed the menu router item path from admin/settings/formats/delete to admin/settings/formats/%text_format/delete to make the whole thing even more trivial.

RTBC as far as I can see.

This review is powered by Dreditor.

Looks great. Committed to CVS HEAD. Thanks for the steady stream of great work, sun and dropcube.