Stop allowing filters to disable caching

I think this would be awsome, I am currently trying to make a general box embed module, that would let one in embed a box in a content field.

The main issue we run into is as there is no connection from the node to the box, it the box change there is no way to say clear the cache on the node. It would be very nice if we could add the box after the content is cached. We had the same issue in d6 with the node_embed module.

Running uncacheable filters last implies we ignore filter order for uncacheable filters, which could break things? And if we allow filters to do things dynamically via #post_render_cache, then why do we need this at all?

I think that's what you're getting at in #2 when you say "less and less reason for this": when #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags lands, we can just *remove* the ability for filter plugins to indicate they're not cacheable.

Please confirm or clarify :)

My humble try... please see the patch.

#5: thanks for the patch! I independently worked on this too (as part of #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags), here's my version. Let's help each other get this one to RTBC :)

Rerolled/merged. Let's see if testbot likes.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterBase.php
@@ -45,13 +45,6 @@
-  /**

You lost this from #7.

Other than that, it looks good I think :)

Thanks, updated.

+++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItemBase.php
@@ -74,7 +74,7 @@ public function getCacheData() {
-    if (!$text_processing || filter_format_allowcache($this->get('format')->getValue())) {
+    if (!$text_processing) {

The original code says: "if text processing is disabled, or if text processing is enabled but none of the filters in this text format disables filtering, then generate a cached value".

We've changed that to "if text processing is disabled, then generate a cached value".

This is wrong.

This entire if-statement can go away! :)

Updated the patch.

There are a few more leftovers to be done before I can RTBC this:

1. The "cache" key should be removed from all filter.format.*.yml files.
2. filter.schema.yml should be updated also.
3. FilterFormat still has a $cache property.
4. FilterInterface still lists cache as an annotation property
5. FilterDefaultConfigTest is still testing $this->assertEqual($format->get('cache'), 1);, which is passing today because of point 1.

(Discovered while working on #2217877-22: Text filters should be able to add #attached, #post_render_cache, and cache tags.)

Thanks for the list. All removed.

testFilterAdminClearsFilterCache() is probably not needed anymore as well.

No, #19 is wrong: we are not removing the filter cache here, so you cannot remove the test coverage either.

Since #16 failed, that means this patch introduces a regression somewhere, as demonstrated by the failure in testFilterAdminClearsFilterCache().

Once you've fixed that failure, I can RTBC this :)

Oh, right.

The reason for the fail was that logic in computeFilterCacheId() still accounted for the $format->cache.

bumping this up

#2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags went in, which includes what this patch does and more. I'm very sorry, but this is no longer necessary.