{filter_format}.cache is not saved

HEAD is broken.

reroll

Looks good, but testbot bailed out again for any reason.

+++ modules/filter/filter.module	5 Sep 2009 10:29:45 -0000
@@ -172,6 +172,7 @@ function filter_format_save($format) {
+  $format->cache = filter_cacheable(array_keys(array_filter($format->filters)));
@@ -459,16 +460,34 @@ function _filter_list_cmp($a, $b) {
+ * Helper function to check if a text format that contains the given filters
+ * can be cached. 
...
+function filter_cacheable(array $filters) {

PHP summary should be on one line.

I think the function name should be filter_format_is_cacheable() for extra clarity.

It would make sense to pass a $format object instead and let the helper function iterate over the contained (enabled) filters.

Also note the trailing white-space. (and elsewhere)

+++ modules/filter/filter.module	5 Sep 2009 10:29:45 -0000
@@ -459,16 +460,34 @@ function _filter_list_cmp($a, $b) {
+ * @param $filters
+ *   An array of filter names. 

It's not entirely clear to me what "an array of filter names" means. Can we clarify that? :)

+++ modules/filter/filter.module	5 Sep 2009 10:29:45 -0000
@@ -459,16 +460,34 @@ function _filter_list_cmp($a, $b) {
+    // The default is 'cache' => TRUE.
+    if (isset($filter_info[$name]['cache']) && !$filter_info[$name]['cache']) {

This comment could be a bit more clear; f.e. By default, 'cache' is TRUE for all filters unless specified otherwise.

Lastly, it looks like this functionality should be covered in a test, so we don't break it again. :)

I'm on crack. Are you, too?

Introducing a new tag for feature freeze: API clean-up.

Looks good, and I agree with sun's comment.

In addition, we should make the difference between filter_format_is_cachable() and filter_format_allowcache() more clear. We might have to rename filter_format_allowcache().

+++ modules/filter/filter.module	5 Sep 2009 10:29:45 -0000
@@ -172,6 +172,7 @@ function filter_format_save($format) {
+  $format->cache = filter_cacheable(array_keys(array_filter($format->filters)));
@@ -459,16 +460,34 @@ function _filter_list_cmp($a, $b) {
 function filter_format_allowcache($format) {
...
+function filter_cacheable(array $filters) {

Not sure whether we need another helper function for this.

We should additionally clarify when to use which function (in this approach), because if they happen to be invoked in a unintended order, then we'd end up in an endless loop (filter_format_load() invoked during filter_format_save()...)

Additionally, filter_format_save() already iterates over all filters, so we might just have to change the order of storage (which sounds unorthodox, but shouldn't affect anything).

I'm on crack. Are you, too?

Also, seems like we should tackle + commit #562908: {filter}.module is not saved first.

Thanks!

However, I'd like to defer that test clean-up to #573852: Rename {filter_format}.name to .title and replace .format with machine-readable .name -- the entire filter.test file gets "filters" and "formats" completely wrong. Which is, btw, quite interesting: Not only users don't get it, but also developers/test writers obviously had no clear idea of what exactly they were testing...

Tagging absolutely critical clean-ups for D7. Do not touch this tag.

I just realized that this patch contains an API change... :( This it absolutely has to be done before 10/15.

Can we quickly re-roll #13 without any test clean-ups?

Re-rolled the essentials - without tests. Not sure how to test this yet.

heh... nice if bugs are totally obvious by reviewing the patch. ;)

Passing the tests.

Let's get this in.

+++ modules/filter/filter.module	28 Oct 2009 00:06:29 -0000
@@ -494,16 +495,51 @@ function _filter_list_cmp($a, $b) {
 function filter_format_allowcache($format_id) {

I think the difference between filter_format_allowcache() and _filter_format_allowcache() is confusing. Can be find better names for those, or can we merge them somehow?

+++ modules/filter/filter.module	28 Oct 2009 00:06:29 -0000
@@ -494,16 +495,51 @@ function _filter_list_cmp($a, $b) {
+ * Helper function to determine whether a given text format can be cached.

I guess this means the "text output by a given text format" rather than the text format itself?

I can see how this could be confusing. But well, these two functions are really completely different functions.

The private function is only used by filter_format_save() to determine whether a format that is about to be saved - and may not exist in the database yet - supports caching.

The public function, filter_format_allowcache(), exists for a long time already, and takes a $format_id only, so it can be used by modules in other situations. For example, text.module calls this function to determine whether it can cache the content of a text field for rendering.

I don't want to introduce an API change by renaming filter_format_allowcache() in here.

But, I also don't want to inline the code of the private function into filter_format_save(). So I hope that renaming the private function to _filter_format_is_cacheable() is sufficient to get this done.

.

This time, also including tests to make sure we don't break this again.

Alright. Can't think of a better name either. Committed to CVS HEAD -- we have to move forward.