Convert filters to plugins

Yay, thanks for attacking this !

[edit: numbered my items]

1.
This is a typical case where having #1764380: Merge PluginSettingsInterface into ConfigurableInterface would help consistency across core subsystems.
Not a blocker, obviously - just saying :-)

2.

+ * This hook is invoked by filter_get_filters() and allows modules to register
+ * input filters they provide.

Not good in FilterManager :-)

3.
Nitpicky, but I'm really not fond of FooManager instead of FooPluginManager (I know that's what Entity does, I dislike it there as well :-). According to the coding standards in http://drupal.org/node/608152 :
"Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity"
What this is is a "plugin manager", not a "manager" (which, in itself doesn't mean anything - "manager" is a very versatile word, and it would be a shame to see confiscated by the Plugin API, we could have many other sorts of managers).
The interface & base class are PluginManagerInterface & PluginManagerBase - thus the specialized instances are SomethingPluginManager.

4.

+ *   - weight: A default weight for the filter in new text formats.

I wonder if we really have use cases for this. Weights being mostly arbitrary, I'm not sure how a default weight of 1 or 10 can make any general sense. Probably outside the scope of this "direct convertion to plugin API" patch, but that might be a good opportunity to get rid of a strange feature ?

5.

+    $this->discovery = new AlterDecorator($this->discovery, 'filter_info');
+    $this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));

Process after alter ? ;-)

6.

+  public function getDefinitions() {
+    $definitions = parent::getDefinitions();
+    uasort($definitions, function($a, $b) {
+      return strcmp($a['title'], $b['title']);
+    });
+    return $definitions;
+  }

In similar cases for other plugin types, it has been considered that a list of plugin definitions has no "natural" order that would make sense over another one, and that ordering should be done in the UI code that needs to present a list of plugins.

7.

FilterManager::processDefinition()
+    // @todo Remove this check once http://drupal.org/node/1780396 is resolved.
+    if (!module_exists($definition['module'])) {
+      $definition = NULL;
+      return;
+    }

No biggie, but I'm wondering if we shouldn't be adding that once and for all in PluginManagerBase::processDefinition() instead (and remove it once and for all when the bug is fixed).

8.

FilterBag::__construct()
+    $this->instanceIDs = drupal_map_assoc(array_keys(array_filter($manager->getDefinitions(), function ($filter) {
+      return isset($filter['module']) && module_exists($filter['module']);

Doesn't this duplicate the check in FilterManager::processDefinition() ?

9.

FilterInterface
+ public function settings(array $form, array &$form_state, FilterFormat $format);

settingsForm() rather than settings() ?

10.
All methods in FilterInterface receive a FilterFormat $format. Yet, a FilterPlugin is the instantiation of a specific filter_plugin id with some specific settings - meaning, a different format using the same filter with different (or the same...) settings will not reuse the same plugin instance, right ?
Therefore, given that a given filter object will not be reused across different formats, wouldn't it make more sense for the $format to be injected in the filters constructor rather than in each method separately ?

Actually, unless really needed, it would be a best if FilterPlugin objects stayed agnostic about what the're being used in, and didn't hardcode anything about "I know I'm being used in a filter format". Just like image effects plugins do not need to know anything about a the notion of "image styles" to do their work, and could potentially be used in other contexts.
Container knows its containees, but not the opposite (and containees might be contained in different kinds of stuff)

I might be wrong, but aside from FilterTestReplace (a test filter), I don't see $format being actually used in any of the actual plugin class methods, and I'm not sure why it should ?

3)
FilterManager still violates the code standards though :-)
It's not about reminding people that all filters are plugins, I'm not arguing for HTMLFilterPlugin instead of HTMLFilter.
It's about stating that the plugin manager is a Plugin manager.

10)
Yeah, removing the knowledge about $formats within $filters altogether would need some double-checking with @sun, and could happen in a followup.
But having the $format as a property of the Filter, injected at construct time, is a natural consequence of the migration to an OO API, and doesn't break anything. The previous magic-callback-based API, where a filter was just a collection of functions, had no other way than passing the $format in each function, but that constraint doesn't hold anymore when filters are object. And in the current design of the patch, a $filter is de facto created to be used in a single given format, so passing the format again in each method is just misleading IMO.

The hook_MODULE_config_CRUD() calls will have to go as well. Just a heads up.

Here, have a free reroll to HEAD

As an additional argument for FilterPluginManager rather than FilterManager:
just realized we have an AliasManager right now, and that #1331486: Move module_invoke_*() and friends to an Extensions class is about to add a ModuleManager. Both of them are completely non plugin-related, of course.
The common pattern is to name FooThing the classes that implement ThingInterface. Can we standardize on calling FooPluginManager the classes that implement PluginManagerInterface ? When you are a PluginManager, "Plugin" is the important word, not "Manager", coz there are lots of managers :-).

IMO #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin means
"ideally filters should not need to know $format", not
"if they *do* need $format, it should be injected in each method separately rather than in the constructor"

Thanks for working on this! :)

Filters definitely are a perfect fit for plugins. This is definitely a step in the right direction.

With this baseline, there might still be hope to get some more fundamental filter system changes into D8, which are being summarized in #1828224: Split filter process into text processing and filtering. Of course, that would require us to move on rather quickly here. So let's try :)

Initial review + early feedback on the current patch:

+++ b/core/modules/filter/filter.module
@@ -556,41 +548,13 @@ function filter_get_filters() {
   $filters = &drupal_static(__FUNCTION__, array());

This static should be obsolete, I think. Definitions should be "property-cached" on the manager already.

+++ b/core/modules/filter/filter.module
@@ -768,29 +709,26 @@ function check_markup($text, $format_id = NULL, $langcode = '', $cache = FALSE,
-      $text = $function($text, $filter, $format, $langcode, $cache, $cache_id);
...
+      $text = $filter->process($text, $filter, $langcode, $cache, $cache_id);

1) Passing $filter to Filter::process() seems unnecessary?

2) I'm not fond of removing $format from the arguments. AFAIK, there are advanced filters out there that dynamically adjust themselves depending on the existence/configuration of other filters in a format.

That's a unique use-case for filters. I can perfectly understand the "plugin purists" perspective you guys discussed earlier in here, but that doesn't really apply to the Filter API.

I think having access to the $format is pretty essential for filters.

I'd actually like to see more "smart/intelligent" filters, which adjust themselves (in safe ways), in case another filter is active, and the combination of the two wouldn't lead to results that anyone would intend to have.

If you don't want to pass $format, then what I actually care for is, I think, passing the FilterBag of all configured filters in the set (i.e., $format->filters ?) to each filter. The format itself isn't necessary. Only the other filters are. (including their weight and settings)

+++ b/core/modules/filter/filter.module
@@ -1060,10 +997,9 @@ function _filter_tips($format_id, $long = FALSE) {
+        $tip = $filter->tips($filter, $long);

We should totally split the long summary from the short summary into two separate methods.

The long tips (being output on the /filter/tips page) should actually return a render array and thus be named buildUsage().

The short tips are supposed to return either a single string, or a render array that is suitable for #type 'item_list'; thus, getSummaryItem().

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -23,7 +23,20 @@ protected function preSave(EntityInterface $entity) {
+    // Clear out any instances to ensure they are rebuilt.
+    $entity->filterPlugins->clear();

Hm. Can we clarify the code comment to explain why this is needed?

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -38,19 +51,17 @@ protected function preSave(EntityInterface $entity) {
-      if (isset($entity->filters[$name])) {
-        $entity->filters[$name] = array_merge($defaults, $entity->filters[$name]);
-      }
-      else {
-        $entity->filters[$name] = $defaults;
+      if (!isset($entity->filters[$name])) {
+        $entity->filters[$name] = array();
       }
+      $entity->filters[$name] += $defaults;

I think we should actually replace this with a NestedArray::merge() now, since filter plugins should be able to use nested array keys for their settings.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -10,6 +10,7 @@
+use Drupal\filter\Plugin\FilterBag;

Hrm. I really dislike classes in \Plugin, but ok...

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/FilterBag.php
@@ -0,0 +1,65 @@
+    $this->instanceIDs = drupal_map_assoc(array_keys($manager->getDefinitions()));

1) Since we know that this won't be empty, we can safely use array_combine() directly here.

2) That said, can't we simply assign the result of getDefinitions() to $this->instanceIDs?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterAutoP.php

+ *   title = @Translation("Convert line breaks into HTML (i.e. <code>&lt;br&gt;

and <p>)"),
+ * type = FILTER_TYPE_MARKUP_LANGUAGE

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtml.php

+ * title = @Translation("Limit allowed HTML tags"),
+ * type = FILTER_TYPE_HTML_RESTRICTOR,
+ * default_settings = {
+ * "allowed_html" = "

 <ul> <ol> <li> <dl> <dt> <dd> <h4> <h5> <h6>",
+ *     "filter_html_help" = 1,
+ *     "filter_html_nofollow" = 0
+ *   },
+ *   weight = -10

I'm not sure whether we need this data in annotations for filters. It is only needed and used in situations in which we have to instantiate a plugin either way.

We should put them into getType(), getLabel(), getDefaultSettings(), and getDefaultWeight() methods.

I'm aware that other plugin types have been converted differently, but I strongly believe those conversions are architecturally wrong, and I don't want the Filter API to be wrong. ;)

Let me know in case you want to punt on that, so I'll do it. :)

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterBase.php
@@ -0,0 +1,96 @@
+  /**
+   * Implements the magic __get() method.
+   */
+  public function __get($key) {
+    if (isset($this->configuration[$key])) {
+      return $this->configuration[$key];
+    }
+  }
+
+  /**
+   * Implements the magic __set() method.
+   */
+  public function __set($key, $value) {
+    $this->configuration[$key] = $value;
+  }
+
+  /**
+   * Implements the magic __isset() method.
+   */
+  public function __isset($key) {
+    return isset($this->configuration[$key]);
+  }

Are these meant to be temporary until this patch is done?

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.php
@@ -93,13 +93,12 @@ function verifyTextFormat($format) {
       // If this filter is not cacheable, update $cacheable accordingly, so we
       // can verify $format->cache after iterating over all filters.
-      if ($filter->status && isset($filter_info[$name]['cache']) && !$filter_info[$name]['cache']) {
+      if ($filter->status && !$filter->cache) {

Hm... I must be missing something - where does that $cache property come from? Shouldn't it be a FilterInterface::isCacheable() method?

That's all I have for now. :) Let me know when you start/stop working on this patch, and I'll try to enhance in between. Can we use branches in the CMI sandbox for doing so?

I'd suggest a filter-plugins-1868772-[username] branch, each. Mix + merge :)

I'm not sure whether we need this data in annotations for filters. It is only needed and used in situations in which we have to instantiate a plugin either way.

We should put them into getType(), getLabel(), getDefaultSettings(), and getDefaultWeight() methods

We really need to bring #1764380: Merge PluginSettingsInterface into ConfigurableInterface to a system-wide agreement about this. Having all plugin types handle their settings in their own separate way is a heartbreak.
My current position, quoted from over there :
" I'd question whether we really expect "several chunks of metadata, each collected separately with separate alter step and static + persistent caches that we need to figure out when to clean" will be more performant and manageable than "one single, all-inclusive list of metadata". Doesn't seem obvious to me, and at least is clearly more painful code wise."

Started to review the patch and the branch last night. Working on this.

Upfront, I've to say that it took me and still takes me hours to decipher the class hierarchies, code paths, and called code. Plugin managers on their own are way too abstracted already, the additional layer of FilterBag exceeds the sanity threshold.

Also, the responsibilities between FilterBag vs. FilterPluginManager are very fuzzy and do not seem to be cleanly separated. That's primarily caused by FilterBag::initializePlugin(), since that contains instantiation logic that simply doesn't belong in there, but exclusively into the plugin manager instead.

The more I study this, the more it appears to me that the concept of FilterBag shouldn't exist in the first place, and instead, we simply need PluginManagers that are smart by design.

As I'll also note below, this patch currently causes the text format creation form to take 2 seconds to load on my machine. Which is caused by this call trace:

FilterFormat::__construct()
-> new FilterBag(FilterFormat, FilterPluginManager)
FilterBag::__construct()
-> FilterPluginManager->get[ALL]Definitions()
-> Discovery::getDefinitions()

So, alas, a complete file system scan and annotation parsing flow on every instantiation of a text format.

Lastly, the current code introduces not one, not two, but three different variables/properties for the settings of a filter plugin instance:

default_settings
settings
configuration

I'm still in the process of figuring out from where each one of those comes from, and what exactly it contains at which point in time, and ultimately, what the actual resulting set of settings for a particular filter is. What's already clear is that the three data variables are needlessly merged into each other a dozen of times during the plugin instantiation process. It looks like origin, availability, intention, and purpose of each one of them wasn't clear, so the current code simply performs some "safety merges" in various spots.

We'll have to figure these points out, but I'm sure we'll get there. :)

I've changed a couple of issues in my local branch already, will push as soon as I figured out some more details.

+++ b/core/modules/filter/filter.admin.inc
@@ -239,43 +222,48 @@ function filter_admin_format_form($form, &$form_state, $format) {
+      '#title' => $filter->title,
...
-      '#title' => t('Weight for @title', array('@title' => $filter['title'])),
+      '#title' => t('Weight for @title', array('@title' => $filter->title)),

Should be ::$label, especially since this is not a user-defined "title".

+++ b/core/modules/filter/filter.module
@@ -307,24 +307,6 @@ function filter_permission_name($format) {
-/**
- * Implements hook_modules_disabled().
- */
-function filter_modules_disabled($modules) {
-  // Reset the static cache of module-provided filters, in case any of the
-  // newly disabled modules defined or altered any filters.
-  drupal_static_reset('filter_get_filters');
-}

+++ b/core/modules/filter/lib/Drupal/filter/FilterBag.php
@@ -0,0 +1,69 @@
+  protected function initializePlugin($instance_id) {
...
+      throw new PluginException(t("Invalid filter '@filter' for format '@format'.", array('@filter' => $instance_id, '@format' => $this->format->label())));

Hm.

It looks like we handled this case gracefully previously.

That was most probably wrong. At the same time, this means a significant API change.

+++ b/core/modules/filter/filter.module
@@ -664,35 +592,15 @@ function filter_list_format($format_id) {
-        uasort($filters['all'][$filter_format->format], 'Drupal\filter\Plugin\Core\Entity\FilterFormat::sortFilters');
...
-        foreach ($filters['all'][$filter_format->format] as $filter_name => $filter) {
...
+          $filters['all'][$filter_format->format][$filter_name] = $filter_format->filterPlugins->get($filter_name);
         }

We've lost the order/sorting here?

Looking some more through the code, sorting filter instances should be responsibility of FilterBag; i.e., whenever you add a filter to the bag, their processing order must be guaranteed.

+++ b/core/modules/filter/filter.module
@@ -1060,10 +964,9 @@ function _filter_tips($format_id, $long = FALSE) {
+        $tip = $filter->tips($long);

Still ::tips().

+++ b/core/modules/filter/lib/Drupal/filter/FilterBag.php
@@ -0,0 +1,69 @@
+ * @todo.

.

+++ b/core/modules/filter/lib/Drupal/filter/FilterBag.php
@@ -0,0 +1,69 @@
+class FilterBag extends PluginBag {
...
+  public function __construct(FilterFormat $format, PluginManagerInterface $manager) {
...
+    $this->instanceIDs = $manager->getDefinitions();

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
@@ -118,6 +119,22 @@ class FilterFormat extends ConfigEntityBase {
+  public $filterPlugins;
...
+    $this->filterPlugins = new FilterBag($this, drupal_container()->get('plugin.manager.filter'));

The more I look into this, the more I believe that FilterBag needs to be lazy-instantiated - either through a dedicated getFilters() method, or through a magic __get() method.

But. Further investigation shows that FilterBag is already designed for lazy-loading!

But: The call to FilterPluginManager::getDefinitions() in FilterBag's constructor completely destroys that, as it triggers a complete discovery of all filter plugins in the system on every instantiation of FilterBag (which is supposed to lazy-load exactly that).

+++ b/core/modules/filter/lib/Drupal/filter/FilterBag.php
@@ -0,0 +1,69 @@
+   * Stores a reference to the view which has this displays attached.

+    // If the display was initialized before, just return.

"view", "displays"

+++ b/core/modules/filter/lib/Drupal/filter/FilterBundle.php
@@ -0,0 +1,25 @@
+    $container->register('plugin.manager.filter', 'Drupal\filter\FilterPluginManager');

Should be filter.plugin_manager

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -23,44 +24,50 @@ protected function preSave(EntityInterface $entity) {
-      // @todo Do not save disabled filters whose properties are identical to
-      //   all default properties.

We want to retain this @todo.

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.php
@@ -23,44 +24,50 @@ protected function preSave(EntityInterface $entity) {
     // Sort all filters.
-    uasort($entity->filters, 'Drupal\filter\Plugin\Core\Entity\FilterFormat::sortFilters');
+    uasort($entity->filters, function ($a, $b) {

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.php
-  public static function sortFilters($a, $b) {

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterBase.php
+  public static function sort($a, $b) {

Instead of inlining it, I think we want to move the sort callback onto the FilterBag. We could as well leave it on FilterFormat though.

Apparently, you moved it to FilterBase, but don't use it anywhere.

That said, unless we always work off filter plugin objects, we might need two separate sort helper functions; one for the form submit handler, and one for the FilterBag. Ideally, we'd only need one, and always work on objects.

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.php
@@ -0,0 +1,120 @@
+ * @return
...
+class FilterPluginManager extends PluginManagerBase {

I wonder whether the plugin manager's class phpDoc is the appropriate place for architectural/plugin documentation.

Alas, a class doesn't return anything. ;)

And yeah, this totally belongs onto FilterInterface instead.

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.php
+    $this->defaults += array(
+      'default_settings' => array(),
+      'settings' => array(),

Why do we have both default_settings and settings for plugin definitions?

settings only exist for instantiated plugins, so that should be superfluous.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterBase.php
@@ -0,0 +1,96 @@
+  /**
+   * An object representing the text format the filter is contained in.
+   *
+   * @var \Drupal\filter\Plugin\Core\Entity\FilterFormat
+   */
+  protected $format;
...
+  public function __construct(array $configuration, $plugin_id, DiscoveryInterface $discovery, FilterFormat $format) {

Prior comments in this issue strongly argued for replacing this with FilterBag.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterBase.php
+  public function __get($key) {
+  public function __set($key, $value) {
+  public function __isset($key) {

Still need to be removed.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtml.php
+ * @todo.
+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlCorrector.php
+ * @todo.
+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlEscape.php
+ * @todo.
+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlImageSecure.php
+ * @todo.
+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterInterface.php
+ * @todo.

.

Summary of changes:

1. Relocated ::tips() to come last in all classes and interfaces.

   Less important methods should come last.

2. Reworked FilterFormat::$filters to always operate on FilterBag.

   The $filters property became protected. Any outside code/callers need to use the ::filters() method instead. The method returns the FilterBag. ::filters($instance_id) returns the requested filter.

   This was an important change from an architectural perspective, since I really did not feel comfortable with the possibility of a potential disparity between FilterFormat::$filters (the configuration, as contained in the text format config file) and the actual configuration of a filter plugin instance. Therefore, all code that retrieves or calls into filters is guaranteed to work on the actual filter plugins and their configuration now.

   Alas, I did not really intend it, but apparently, this change also allowed to remove some larger chunks of code that previously dealt with synchronizing the two data sets (primarily in FilterFormatStorageController::preSave()). :)

3. Reworked FilterInterface and FilterBase to have proper properties and methods.

   As mentioned before, it took me quite some time to step through the new plugin architecture. The definition was not properly separated from the configuration, and the configuration was not properly separated from a filter plugin's $settings array. Apparently, my repeated questions in earlier patch reviews about the magic __get(), __set(), and __isset() methods on FilterBase actually turned out to be the key for resolving the problem space. ;)

   FilterInterface now defines proper methods for getting the type, label, and description of a filter plugin (which are all based on the definition, not configuration).

   FilterBase now defines proper class properties for the actual "configuration" properties of a filter plugin instance; i.e., $plugin_id, $module, $status, $weight, $cache, and $settings. In other words, exactly the properties you see in the text format configuration file.

4. With the two aforementioned changes combined, I also had to quickly invent a facility for "exporting" the configuration of all filter plugin instances in a FilterBag. That is, because FilterFormat::$filters only ever contains the initial configuration, as loaded from config. Any changes to the configuration happen on the filter plugin instances themselves. Therefore, the filter plugin instances need to be exported, in order to resemble back the complete FilterFormat::$filters data to be stored in configuration.

   In short:

   FilterBag::export() calls into all plugin instances in the bag, which implement a corresponding FilterInterface::export().

   Of course, I don't know whether the concept I'm proposing is completely out of line with the rest of the plugin system or any prior art. (Well, it's definitely "out of line", because there doesn't seem to be a facility for this yet :P) If there is anything and I happened to miss it, then I'd love to know. :)

   The idea here essentially was to "inherit" the facility-wise identical functionality of Entity::getExportProperties() to the PluginBag, and from there to all plugin instances in the bag/collection.

I hope this makes sense. Let me know what you think! :)

I didn't run any tests, but manual testing worked fine, so let's see what the bot thinks.

Weird. Not sure why earlier patches didn't fail on that, too.

Fixed $filters['all'] is undefined if there are no formats.

Fixed configured filters are not instantiated.

Hrm. Figuring out the test failures, and how to change the implementation, took me a range of hours...

I'm still not happy with the massive complexity being introduced. However, I'm slowly able to make more sense of the FilterBag.

1. Fixed FilterAdminTest.
2. Fixed FilterCrudTest. Filters need to be instantiable on demand, even if there was no instance ID before.
3. Fixed FilterDefaultConfigTest.
4. Fixed unserialized filter_list_format() cache contains multiple copies of FilterBag.
5. Fixed Filter::$cache is a declaration, not configuration.
6. Fixed FilterFormat::$filters is not updated when the configuration of a filter instance is changed.
7. Fixed API-only FilterFormat::save() should not add plugin configurations for all filters.
8. Fixed FilterSettingsTest.
9. Fixed FilterSecurityTest.
10. Fixed callers of filter_list_format() to work with FilterBag methods.

1. Good call on $this->remove($id).
2. At minimum, it saves function calls. But due to #1901380: [Block]PluginManager repetitively triggers full-stack Annotation parsing on a plugin instance ID miss, the actual purpose is really just to prevent repetitive calls into the plugin discovery stack. Due to the way FilterBag/PluginBag works, it has to lazy-instantiate individual plugins. Therefore, there are multiple origins of possible calls into the plugin manager, seeking for a particular plugin definition. I agree that this shouldn't be necessary in FilterPluginManager, but the entire design of PluginManagerBase + plugin discovery + CacheDecorator looks bogus to me right now, and in combination with a PluginBag, it is actually possible to trigger code execution paths that end up in class methods that should not be reachable, since the plugin manager is actually supposed to catch them upfront already (but does not).

The FilterAdminTest failure is most likely caused by the @todo I added very late to FilterBag::sort() — as of now, the sorting only affects the order of plugin instances, but the FilterBag is actually Iterable, so we actually need to sort the $instanceIDs array (which is the data base of the Iterable), AND the order in both $instanceIDs as well as $pluginInstances has to be *always* identical.

Apparently, this SHOULD include the situation when new filters are added to a format (because you can add a filter with a custom weight). However, that's really non-trivial to figure out, because we don't want to trigger the sorting function on every single plugin instantiation (performance). It should only be triggered when the configuration of a filter is changed, or when a new filter is added.

Hm. I looked at that issue/patch, but I'm not able to see any kind of similarities? (Nevertheless, I commented over there. :))

Merged HEAD, resolved conflicts, and corrected FilterBag::sort() as mentioned previously.

Changes:

1. Make PluginBag::removeInstanceID() also call remove($id).
2. Fixed bogus and unnecessary array_filter() in filter_get_filter_types_by_format().
3. Fixed fatal errors in text_summary(). [What the heck of a function is that, btw?]
4. Fixed FilterAdminTest expectations.
5. Fixed FilterBag::sort() does not actually sort the FilterBag Iterator.

Please note:

1. The FilterAdminTest changes with regard to sorting of filters are extensively verified. The previous/original sorting of filters within a format was not 100% bullet-proof. By coincidence, filter_autop and filter_url in the Standard profile were sorted in a way that filter_url came first.
2. Both have a weight of 0. "a" comes before "u", so filter_autop is sorted before filter_url now.
3. The changes to the default configuration files in Standard profile do not have any impact (since they do not define weights) and are merely updated to reflect the default ordering/weights.

1. Fixed additional fatal error in text_summary().
2. Updated CKEditorTest.

I studied TextSummaryTest, text_summary(), the default plain_text format, as well as the default filtered_html format of Standard profile, for two hours now, but I literally have no idea how the summary length test is able to pass in HEAD.

If I get it correctly, the existing code in HEAD merely checks for if (isset($filters['filter_htmlcorrector'])) — but as we all know or should know by now, text formats contain all filters, regardless of their status. Consequently, the mere existence of a filter in $filters implies no meaning at all.

And indeed, with that conclusion, I just found #1235062: text_summary() ignores filter status .....meh, will pedantry force us to fix that first?

Confirm this trouble, seems it's related to entity loading or other "internals" of manager/controller
EDIT tested on ubuntu+zend server+php 5.3.14

This ordering troubles beets me in #1799600: Add test of sorting for configuration entities once I tried to add taxonomy vocabularies from #1981418: Drop taxonomy_vocabulary_sort()

In addition, the default formats do not match their state when imported.

There is an issue for Plain Text: #1942122: Make Filter module active config save format match the default yml file.
And a meta: #1938580: [META] Make active config save format match the default yml file (order and quotes).

Here's a, hopefully useful, review. Looks great generally, just a bunch of questions and minor things...

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorTest.phpundefined
@@ -107,7 +107,8 @@ function testGetJSSettings() {
-    $format->filters['filter_html']['settings']['allowed_html'] .= '<pre> <h3>';
+    $settings = &$format->filters('filter_html')->settings;
+    $settings['allowed_html'] .= '<pre> <h3>';

Hm, this doesn't really seem to be simpler that way? Why not just access $format->filters['filter_html']->settings['allowed_html'] directly, is there a reason that doesn't work?

+++ b/core/modules/filter/filter.admin.incundefined
@@ -252,48 +225,54 @@ function filter_admin_format_form($form, &$form_state, $format) {
+    // Retrieve the settings form of the filter plugin. The plugin should not be
+    // aware of the text format. Therefore, it only receives a set of minimal
+    // base properties to allow advanced implementations to work.
+    $settings_form = array(
+      '#parents' => array('filters', $name, 'settings'),
+      '#tree' => TRUE,
+    );
+    $settings_form = $filter->settingsForm($settings_form, $form_state);

Is there a reason this is changed? Doesn't look like this was the case before?

+++ b/core/modules/filter/filter.moduleundefined
@@ -166,6 +174,8 @@ function filter_menu() {
+ * @todo Use entity_load().

I'm not 100% convinced about removing entity_load() wrappers. save/delete absolutely, but load I'm not sure. Doesn't seem to hurt to have them and you don't need to use them if you don't want to.

Or does this mean that the implementation should use entity_load()? :)

Probably off-topic anyway ;)

+++ b/core/modules/filter/filter.moduleundefined
@@ -595,51 +505,36 @@ function _filter_format_is_cacheable($format) {
-  return isset($filters[$format_id]) ? $filters[$format_id] : array();
+  return isset($filters[$format_id]) ? $filters[$format_id] : NULL;

Why is this changed? There was a recent discussion on twitter/planet that functions that return a list/collection should always return a list/collection and not NULL. field_get_items() was recently changed to return an array(), this seems backward? And unrelated, or am I missing something? If this doesn't return an array anymore, then @return needs to be updated. That still says array.

+++ b/core/modules/filter/filter.moduleundefined
@@ -712,29 +607,26 @@ function check_markup($text, $format_id = NULL, $langcode = '', $cache = FALSE,
-    if (!empty($filter->status) && isset($filter_info[$name]['prepare callback'])) {
-      $function = $filter_info[$name]['prepare callback'];
-      $text = $function($text, $filter, $format, $langcode, $cache, $cache_id);
+    if ($filter->status) {
+      $text = $filter->prepare($text, $langcode, $cache, $cache_id);

Those are the parts about plugin conversions that I love :)

+++ b/core/modules/filter/lib/Drupal/filter/FilterFormatStorageController.phpundefined
@@ -22,45 +22,17 @@ protected function preSave(EntityInterface $entity) {
+    // @todo This is a derived/computed definition, not configuration.
+    $entity->cache = TRUE;
+    foreach ($entity->filters() as $filter) {
+      if ($filter->status && !$filter->cache) {
+        $entity->cache = FALSE;

What should be done with this then? Maybe the @todo should say that? Saving this seems to make sense as it's the fastest way to get it again?

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.phpundefined
@@ -0,0 +1,50 @@
+    $this->discovery = new AnnotatedClassDiscovery('filter', 'filter', $namespaces, $annotation_namespaces, 'Drupal\filter\Annotation\Filter');

As mentioned in the Drupal::pluginManager() issue, $owner == $type seems to confuse people (why repeat, what's the difference). But I don't have a better idea, owner is obviously given and it is a filter...

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.phpundefined
@@ -0,0 +1,50 @@
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::createInstance().

@inheritdoc ;)

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterAutoP.phpundefined
@@ -0,0 +1,45 @@
+ *   id = "filter_autop",
...
+class FilterAutoP extends FilterBase {

I assume this is just taken from the existing definition? Seems to be a strange class name, especially because it doesn't just convert to p but also br. AutoLineBreak seems like a better name but I'm not sure how involving it would be to change this.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterAutoP.phpundefined
@@ -0,0 +1,45 @@
+   * Implements \Drupal\filter\Plugin\filter\filter\FilterInterface::process().
...
+   * Overrides \Drupal\filter\Plugin\filter\filter\FilterBase::tips().

I assume there are a lot more of those below but you already started using it above so it should probably be consistent? And if we don't do it now then we'll have to do it later.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtml.phpundefined
@@ -0,0 +1,161 @@
+    foreach ($entities as $entity) {

Noticed a $entity while skimming through the code which confused me for a moment. Nothing wrong with it obviously :)

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlCorrector.phpundefined
@@ -0,0 +1,34 @@
+ * @todo.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlEscape.phpundefined
@@ -0,0 +1,41 @@
+ * @todo.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlImageSecure.phpundefined
@@ -0,0 +1,42 @@
+ * @todo.

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterUrl.phpundefined
@@ -0,0 +1,58 @@
+ * @todo.

:)

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlCorrector.phpundefined
@@ -0,0 +1,34 @@
+  public function process($text, $langcode, $cache, $cache_id) {
+    return _filter_htmlcorrector($text);

I assume this function is used in other places as well?

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterHtmlImageSecure.phpundefined
--- /dev/null
+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterInterface.phpundefined

I know this was done in other places as well but having the interface within the plugin directory still seems a bit weird. Among other things, it is now in the middle of actual implementations, so while reading through the patch, there's a mix of plugins, the interface and base class.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAdminTest.phpundefined
@@ -209,7 +211,6 @@ function testFilterAdmin() {
-    $this->drupalLogout();
     $this->drupalLogin($this->web_user);

@@ -250,7 +251,6 @@ function testFilterAdmin() {
-    $this->drupalLogout();
     $this->drupalLogin($this->admin_user);

Makes sense but somewhat off-topic :)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterDefaultConfigTest.phpundefined
@@ -42,7 +42,7 @@ function setUp() {
-    $this->assertTrue($format);
+    $this->assertTrue((bool) $format);

Hm, shouldn't be necessary, assertTrue is essentially a assert((bool) $value), so will be exactly the same?

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterSettingsTest.phpundefined
@@ -7,18 +7,14 @@
+  public static $modules = array('filter');

I think the that needs a docblock according to our current standards (don't ask me why getInfo() doesn't need one but this does...)

+++ b/core/modules/filter/tests/filter_test/lib/Drupal/filter_test/Plugin/filter/filter/FilterTestUncacheable.phpundefined
@@ -0,0 +1,35 @@
+  /**
+   * Implements \Drupal\filter\Plugin\filter\filter\FilterInterface::process().
+   */
+  public function process($text, $langcode, $cache, $cache_id) {
+    return $text;

Wondering if that should be the default implementation :) Or is it already?

+++ b/core/modules/text/lib/Drupal/text/Tests/TextSummaryTest.phpundefined
@@ -7,13 +7,14 @@
-class TextSummaryTest extends WebTestBase {
-  protected $profile = 'standard';
+class TextSummaryTest extends DrupalUnitTestBase {

We should be careful with making large patches even larger with such conversions but double-yay for converting standard web tests to dubt ;)

+++ b/core/modules/text/lib/Drupal/text/Tests/TextSummaryTest.phpundefined
@@ -25,7 +26,10 @@ public static function getInfo() {
+    config_install_default_config('module', 'text');

I think there's a $this->installConfig() now?

+++ b/core/modules/text/lib/Drupal/text/Tests/TextSummaryTest.phpundefined
@@ -51,133 +55,174 @@ function testLongSentence() {
-    $expected = array(
-      "<p>\nHi\n</p>\n<p>\nfolks\n<br />\n!\n</p>",
...
+    $format = NULL;
+    $i = 0;
+    $this->assertTextSummary($text, "<p>\nHi\n</p>\n<p>\nfolks\n<br />\n!\n</p>", $format, $i++);

Why is this changed from explicit calls to a loop here? EDIT: The other way round I mean obviously.

+++ b/core/modules/text/lib/Drupal/text/Tests/TextSummaryTest.phpundefined
@@ -51,133 +55,174 @@ function testLongSentence() {
-  /**
-   * Test sending only summary.
-   */
-  function testOnlyTextSummary() {

Is this moved to another class?

+++ b/core/modules/text/lib/Drupal/text/Tests/TextSummaryTest.phpundefined
@@ -29,7 +29,7 @@ function setUp() {
-    config_install_default_config('module', 'text');
+    $this->installConfig(array('module', 'text'));

Just installConfig(array('text')).

That should fix the first fail, the second is a random one, there's an issue for it.

I guess this needs a re-roll now after the plugin directory change?

Looks good to me otherwise.

Was about to RTBC this yesterday but then noticed that there are still ~8 @todo. class docblocks in there which should be fixed.

Great. Thank you.

I've reviewed this a few times and while I'm not a filter.module nor plugin system maintainer, the sole filter.module maintainer worked quite a bit of this and I've RTBC'd a few plugin conversion issues and so far the world hasn't ended ;)

So...

Committed 9f57b6f and pushed to 8.x. Thanks!

+++ b/core/modules/filter/lib/Drupal/filter/Annotation/Filter.phpundefined
@@ -0,0 +1,26 @@
+class Filter extends Plugin {
+
+  public $title;
+  public $description = '';
+  public $weight = 0;
+  public $status = FALSE;
+  public $cache = TRUE;
+  public $settings = array();
+
+}

Lets get a followup to add document the variables...

Looks good to me!

A question about this: what we do with filters that are no files/classes? I am working with the module customfilter(porting it do d8) but now if must be a real file for the filter there is not a way to this module and other like this works.

As soon as the filter plugin manager is converted to extend from the default plugin manager, you can use derivatives for this. This means you add one class (which you need anyway to actually do things), specify a derivative class which basically exposes that same class as many times as you want.

See #2039521: Convert FilterPluginManager to extend DefaultPluginManager and MenuBlock and similar block plugins for an example.

Updated issue summary