Problem/Motivation

hook_filter_info() is a hook for defining a set of callbacks that effectively map to a single object

Proposed resolution

That's almost the definition of what our plugins are. Let's convert it!

Remaining tasks

None now that #1779026: Convert Text Formats to Configuration System is in

User interface changes

N/A

API changes

Replaced with \Drupal\filter\Plugin\filter\filter\FilterInterface:

  • hook_filter_info()
  • hook_filter_FILTER_settings()
  • hook_filter_FILTER_prepare()
  • hook_filter_FILTER_process()
  • hook_filter_FILTER_tips()

Removed:
filter_get_filters() (Previously used in conjunction with a format's filter, now the information is stored directly on the filter)

Files: 
CommentFileSizeAuthor
#102 filter-1868772-102.patch134.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es). View
#102 interdiff.txt4.26 KBtim.plunkett
#100 filter-1868772-100.patch133.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,432 pass(es). View
#100 interdiff.txt1.32 KBtim.plunkett
#98 filter-1868772-98.patch134.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,588 pass(es). View
#95 filter-1868772-95.patch134.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,418 pass(es), 2 fail(s), and 0 exception(s). View
#95 interdiff.txt23.79 KBtim.plunkett
#91 filter-1868772-91.patch136.48 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es). View
#91 interdiff.txt890 bytestim.plunkett
#89 filter-1868772-89.patch136.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,379 pass(es), 2 fail(s), and 0 exception(s). View
#89 interdiff.txt1.31 KBtim.plunkett
#87 filter-1868772-86.patch136.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,787 pass(es), 26 fail(s), and 2 exception(s). View
#87 filter-1868772-86.patch136.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,280 pass(es), 26 fail(s), and 2 exception(s). View
#87 filter-1868772-86.patch136.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,828 pass(es), 26 fail(s), and 2 exception(s). View
#87 filter-1868772-86.patch136.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,321 pass(es), 26 fail(s), and 2 exception(s). View
#87 interdiff.txt10.04 KBtim.plunkett
#84 filter-plugins-1868772-82.patch129.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es). View
#84 filter-plugins-1868772-82.patch129.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,830 pass(es). View
#84 filter-plugins-1868772-82.patch129.85 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,305 pass(es), 1 fail(s), and 0 exception(s). View
#84 filter-plugins-1868772-82.patch129.85 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,299 pass(es), 1 fail(s), and 0 exception(s). View
#84 filter-plugins-1868772-81.patch130.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,774 pass(es). View
#84 filter-plugins-1868772-81.patch130.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,797 pass(es). View
#84 filter-plugins-1868772-81.patch130.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,859 pass(es), 1 fail(s), and 0 exception(s). View
#84 filter-plugins-1868772-81.patch130.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,711 pass(es), 1 fail(s), and 0 exception(s). View
#82 filter-plugins-1868772-82.patch129.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es). View
#82 interdiff.txt694 bytestim.plunkett
#81 filter-plugins-1868772-81.patch130.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,430 pass(es), 1 fail(s), and 0 exception(s). View
#79 filter-1868772-79.patch129.32 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 29 fail(s), and 1,824 exception(s). View
#76 filter-1868772-76.patch129.32 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,359 pass(es), 1 fail(s), and 0 exception(s). View
#74 filters-1868772-74.patch129.32 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,046 pass(es), 33 fail(s), and 7 exception(s). View
#69 filters-1868772-69.patch130.5 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filters-1868772-69.patch. Unable to apply patch. See the log in the details link for more information. View
#69 interdiff.txt2.12 KBtim.plunkett
#67 filter-1868772-67.patch131.11 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,817 pass(es), 2 fail(s), and 0 exception(s). View
#67 interdiff.txt10.42 KBtim.plunkett
#63 filter.plugins.63.patch130.97 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filter.plugins.63.patch. Unable to apply patch. See the log in the details link for more information. View
#63 interdiff.txt17.41 KBsun
#61 filter.plugins.61.patch115.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,647 pass(es), 11 fail(s), and 0 exception(s). View
#61 interdiff.txt2.02 KBsun
#59 filter.plugins.59.patch114.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,340 pass(es), 3 fail(s), and 2 exception(s). View
#59 interdiff.txt7.22 KBsun
#53 filter.plugins.53.patch110.94 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,255 pass(es), 6 fail(s), and 4 exception(s). View
#53 interdiff.txt25.54 KBsun
#51 filter.plugins.51.patch99.37 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,052 pass(es), 20 fail(s), and 15 exception(s). View
#51 interdiff.txt1.79 KBsun
#50 filter.plugins.50.patch99.31 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,909 pass(es), 86 fail(s), and 22 exception(s). View
#50 interdiff.txt485 bytessun
#48 filter.plugins.48.patch99.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,938 pass(es), 88 fail(s), and 293 exception(s). View
#48 interdiff.txt49.9 KBsun
#43 filters-1868772-43.patch89.36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,243 pass(es). View
#42 filters-1868772-42.patch89.36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,858 pass(es). View
#42 interdiff.txt3.27 KBtim.plunkett
#40 filters-1868772-40.patch89.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,618 pass(es), 9 fail(s), and 2 exception(s). View
#40 interdiff.txt8.68 KBtim.plunkett
#38 filters-1868772-38.patch88.76 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,564 pass(es), 63 fail(s), and 0 exception(s). View
#38 interdiff.txt19.66 KBtim.plunkett
#33 filters-1868772-33.patch87.84 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,676 pass(es), 1 fail(s), and 0 exception(s). View
#33 interdiff.txt21.72 KBtim.plunkett
#31 interdiff.txt8.18 KBtim.plunkett
#31 filters-1868772-31.patch88.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,644 pass(es). View
#30 filters-1868772-30.patch89.28 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,654 pass(es). View
#30 interdiff.txt1001 bytestim.plunkett
#28 filters-1868772-28.patch89.45 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,698 pass(es), 20 fail(s), and 6 exception(s). View
#28 interdiff.txt2.73 KBtim.plunkett
#26 filters-1868772-26.patch89.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 50,667 pass(es), 5 fail(s), and 0 exception(s). View
#24 1868772-filters-as-plugins-24.patch98.98 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1868772-filters-as-plugins-24.patch. Unable to apply patch. See the log in the details link for more information. View
#22 filters-1868772-22.patch146.94 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filters-1868772-22.patch. Unable to apply patch. See the log in the details link for more information. View
#16 filters-1868772-16.patch152.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,319 pass(es). View
#14 filters-1868772-13.patch152.26 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,259 pass(es), 47 fail(s), and 7,803 exception(s). View
#12 filters-1868772-12.patch151.59 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,347 pass(es). View
#12 filters-1868772-12-without_other_patches-do-not-test.patch86.71 KBtim.plunkett
#10 filters-1868772-10.patch150.2 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,208 pass(es), 37 fail(s), and 0 exception(s). View
#8 filters-1868772-8.patch138.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,517 pass(es), 58 fail(s), and 1,324 exception(s). View
#6 filters-1868772-7.patch135.87 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,319 pass(es). View
#4 filters-1868772-5.patch135.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,157 pass(es), 20 fail(s), and 5 exception(s). View
#2 filters-1868772-2.patch155.18 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,066 pass(es), 23 fail(s), and 4 exception(s). View
#2 filters-1868772-2-without-formats-do-not-test.patch95.67 KBtim.plunkett
#1 filters-1868772-1.patch120.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 4 fail(s), and 2 exception(s). View

Comments

tim.plunkett’s picture

FileSize
120.24 KB
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 4 fail(s), and 2 exception(s). View

This is a work-in-progress, and includes the text format conversion patch, which isn't currently passing tests.
I'll post a sandbox of this later.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
95.67 KB
155.18 KB
FAILED: [[SimpleTest]]: [MySQL] 49,066 pass(es), 23 fail(s), and 4 exception(s). View

Okay, this is with the callbacks converted.

Some unit tests will fail, because they call private _filter_*() functions directly that are moved.

Also, filters are passed around as stdClass in a lot of places

Status: Needs review » Needs work

The last submitted patch, filters-1868772-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
135.54 KB
FAILED: [[SimpleTest]]: [MySQL] 49,157 pass(es), 20 fail(s), and 5 exception(s). View

More fixes, cleaner patch.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-5.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
135.87 KB
PASSED: [[SimpleTest]]: [MySQL] 49,319 pass(es). View

Fixed those three failures, the fourth was random.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-7.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
138.8 KB
FAILED: [[SimpleTest]]: [MySQL] 49,517 pass(es), 58 fail(s), and 1,324 exception(s). View

Okay, trying out loading the plugins.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-8.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
150.2 KB
FAILED: [[SimpleTest]]: [MySQL] 49,208 pass(es), 37 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, filters-1868772-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
86.71 KB
151.59 KB
PASSED: [[SimpleTest]]: [MySQL] 49,347 pass(es). View

Okay, this should be working now.

Also attaching the patch without the other two issues.

tim.plunkett’s picture

FileSize
152.26 KB
FAILED: [[SimpleTest]]: [MySQL] 47,259 pass(es), 47 fail(s), and 7,803 exception(s). View

Okay, after this I think I'm happy. Just some @todo's to fill in now.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-13.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
152.92 KB
PASSED: [[SimpleTest]]: [MySQL] 49,319 pass(es). View

Whoops. A little overzealous.

yched’s picture

Yay, thanks for attacking this !

[edit: numbered my items]

1.
This is a typical case where having #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface 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 ?

tim.plunkett’s picture

3) I started with that and just thought it looked silly. All Filters are plugins, but we don't need to remind people of that. It is the manager of the filters. And yes, you can tell it's my opinion, because I wrote EntityManager too :)

4) Filters are unique in that they DO have a weight. "Limit allowed HTML tags" should generally come first, since it might restrict BR/P tags, but "Convert line breaks into HTML (i.e. <br> and <p>)" will add them back. It's an important part of the architecture.

5) D'OH!

6) Unlike the other order, this one is stupid. Agreed on removing.

7) Out of scope, but I might go ahead an open a separate issue.

8) Yep, I copied most of that and it's mostly a duplicate. I had forgotten that 7) WASN'T done upstream

9) Sure, I think that is clearer and matches other places

10) That was a 1:1 conversion from the hooks to the methods, and I didn't want to needlessly break contrib by removing something. But I also agree that you shouldn't need that

yched’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work

Meant to do this earlier.

tim.plunkett’s picture

Status: Needs work » Postponed

#1779026: Convert Text Formats to Configuration System is still changing slightly, but is very very close, so I'm going to temporarily postpone this so I don't have to keep working on a moving target. I'll be back soon.

tim.plunkett’s picture

FileSize
146.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filters-1868772-22.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled to keep up with HEAD. Still need to respond to #19.

heyrocker’s picture

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

heyrocker’s picture

FileSize
98.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1868772-filters-as-plugins-24.patch. Unable to apply patch. See the log in the details link for more information. View

Here, have a free reroll to HEAD

yched’s picture

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 :-).

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
89.83 KB
FAILED: [[SimpleTest]]: [MySQL] 50,667 pass(es), 5 fail(s), and 0 exception(s). View

That last reroll had some parts missing. And the formats patch changed a good deal since I worked on this, so I have some catching up to do. Just posting a sloppy reroll for now.

And yes @yched, I'll switch the naming around... :)

This has a lot of cobwebs, no reviews yet please :)

Status: Needs review » Needs work

The last submitted patch, filters-1868772-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
89.45 KB
FAILED: [[SimpleTest]]: [MySQL] 50,698 pass(es), 20 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, filters-1868772-28.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1001 bytes
89.28 KB
PASSED: [[SimpleTest]]: [MySQL] 50,654 pass(es). View

I think this will be green, but there's still some needless changes made by this patch that I'd like to clean up.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
88.78 KB
PASSED: [[SimpleTest]]: [MySQL] 50,644 pass(es). View
8.18 KB

Because of #1779026-176: Convert Text Formats to Configuration System, and various other possible contrib usecases, I'm not going to remove the format being passed to filter methods.

I'd switch to injecting the format into the filter constructor like I did in the block issue, but #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin happened, so who knows :)

If this fails then I'll clean it up, I removed a couple things I hope we don't need :)

yched’s picture

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"

tim.plunkett’s picture

FileSize
21.72 KB
87.84 KB
FAILED: [[SimpleTest]]: [MySQL] 50,676 pass(es), 1 fail(s), and 0 exception(s). View

Fair point :)

Here's that taken care of.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-33.patch, failed testing.

sun’s picture

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 &lt;p&gt;)"),
+ * 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 :)

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Thanks for the review! Working through this now.

yched’s picture

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 ConfigurablePluginInterface 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."

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.66 KB
88.76 KB
FAILED: [[SimpleTest]]: [MySQL] 50,564 pass(es), 63 fail(s), and 0 exception(s). View

Pushed up filter-plugins-1868772-tim. Don't mind the messy history, my commit messages were not written with the plan to push this publicly :)

Removed drupal_static

I've removed passing the filter to itself :)
I stopped passing the format to each method, because it's passed to the constructor and stored as $this->format. If we want to pass the other filters instead, that's a good follow-up.

I'm just swapping the "tips callback" out for a tips() method, it's not really in-scope for a conversion to start making changes like that.

Changed the comment.

I switched to NestedArray, and in general cleaned up how the defaults are provided.

While just working with this now, having FilterBag in \Plugin threw me off, so I moved it :)

Good call, we don't need any of that drupal_map_assoc() call.

I'm punting on breaking up the annotation, so that it stays alterable. See @yched's comment.

I hadn't meant for them to be temporary, but we should decide if we want to leave it, and if not, how to replace it (true get()/set() calls? direct access of configuration?)

The cache bit it in the annotation now. Same problem as above.

---
Leaving assigned in case I have any stupid errors or test fails to fix up.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-38.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.68 KB
89.51 KB
FAILED: [[SimpleTest]]: [MySQL] 50,618 pass(es), 9 fail(s), and 2 exception(s). View

Argh, I always get the order of NestedArray::mergeDeep() wrong.

Also, now that we don't have the drupal_static in filter_get_filters(), there is a bunch to be cleaned up.

ec8543b Fix order of NestedArray::mergeDeep().
f2363b7 Remove sortFilters().
fc3e34a Revert unnecessary changes to filter_get_filter_types_by_format().
c465514 Kill filter_get_filters() once and for all.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-40.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » sun
Status: Needs work » Needs review
FileSize
3.27 KB
89.36 KB
PASSED: [[SimpleTest]]: [MySQL] 50,858 pass(es). View

c7fa141 Pass the correct parameters to filter methods.
d5b9d0b Restore functionality removed by last patch.

Okay sun, it's all yours. Don't change *too* much please :)

tim.plunkett’s picture

FileSize
89.36 KB
PASSED: [[SimpleTest]]: [MySQL] 49,243 pass(es). View

Rebased the branch against 8.x

Status: Needs review » Needs work

The last submitted patch, filters-1868772-43.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Testbot fluke. Sun, you implied you had more feedback to give, or are we done?

tim.plunkett’s picture

Issue summary: View changes

following issue summaries

tim.plunkett’s picture

Updated the issue summary.

sun’s picture

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.

.

sun’s picture

FileSize
49.9 KB
99.29 KB
FAILED: [[SimpleTest]]: [MySQL] 48,938 pass(es), 88 fail(s), and 293 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, filter.plugins.48.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
485 bytes
99.31 KB
FAILED: [[SimpleTest]]: [MySQL] 48,909 pass(es), 86 fail(s), and 22 exception(s). View

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

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

sun’s picture

FileSize
1.79 KB
99.37 KB
FAILED: [[SimpleTest]]: [MySQL] 49,052 pass(es), 20 fail(s), and 15 exception(s). View

Fixed configured filters are not instantiated.

Status: Needs review » Needs work

The last submitted patch, filter.plugins.51.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
25.54 KB
110.94 KB
FAILED: [[SimpleTest]]: [MySQL] 48,255 pass(es), 6 fail(s), and 4 exception(s). View

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.
tim.plunkett’s picture

I didn't fully review the changes in #48-51 yet, this is just of the interdiff in #53.
Most of this looks good, some is a little odd. I'm glad you're warming up to PluginBag.

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -105,6 +105,16 @@ public function addInstanceID($id) {
+    unset($this->instanceIDs[$id]);

Wouldn't this then need to call $this->remove($id);

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.phpundefined
@@ -44,6 +49,38 @@ public function __construct() {
+  public function getDefinition($plugin_id) {
+    if (!isset($this->definitions[$plugin_id])) {
+      $this->definitions[$plugin_id] = $this->discovery->getDefinition($plugin_id);
+    }
+    return $this->definitions[$plugin_id];
+  }
+
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::getDefinitions().
+   */
+  public function getDefinitions() {
+    // We do not know whether getDefinition() was called for particular plugin
+    // IDs already, so we retrieve and reset all.
+    $this->definitions = $this->discovery->getDefinitions();
+    return $this->definitions;
+  }
+
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::clearCachedDefinitions().
+   */
+  public function clearCachedDefinitions() {
+    // Clear the local property cache.
+    $this->definitions = array();
+    // Clear the persistent CacheDecorator cache, if there is one.
+    if ($this->discovery instanceof CachedDiscoveryInterface) {
+      $this->discovery->clearCachedDefinitions();
+    }

What does this gain us over just relying on the CacheDecorator static caching?

+++ b/core/modules/filter/tests/filter_test/lib/Drupal/filter_test/Plugin/filter/filter/FilterTestUncacheable.phpundefined
@@ -20,7 +20,7 @@
- *   cache = FALSE
+ *   cache = false

Er, okay.

Status: Needs review » Needs work

The last submitted patch, filter.plugins.53.patch, failed testing.

sun’s picture

  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.

tim.plunkett’s picture

The changes to FilterPluginManager look a lot like #1903346: Establish a new DefaultPluginManager to encapsulate best practices (which was actually written after, and with no influence from this issue, amazing!), can we punt on that here?

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
7.22 KB
114.46 KB
FAILED: [[SimpleTest]]: [MySQL] 50,340 pass(es), 3 fail(s), and 2 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, filter.plugins.59.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
115.46 KB
FAILED: [[SimpleTest]]: [MySQL] 50,647 pass(es), 11 fail(s), and 0 exception(s). View
  1. Fixed additional fatal error in text_summary().
  2. Updated CKEditorTest.

Status: Needs review » Needs work

The last submitted patch, filter.plugins.61.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.41 KB
130.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filter.plugins.63.patch. Unable to apply patch. See the log in the details link for more information. View

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?

Status: Needs review » Needs work
Issue tags: -Plugin system

The last submitted patch, filter.plugins.63.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#63: filter.plugins.63.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Plugin system

The last submitted patch, filter.plugins.63.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.42 KB
131.11 KB
FAILED: [[SimpleTest]]: [MySQL] 53,817 pass(es), 2 fail(s), and 0 exception(s). View

Rerolled. Still haven't groked all the changes since my passing patch.

Status: Needs review » Needs work

The last submitted patch, filter-1868772-67.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
130.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filters-1868772-69.patch. Unable to apply patch. See the log in the details link for more information. View

!!
Somewhere along the way, the above patches started rearranging filters in core formats, not test ones. And with the addition of the WYSIYG formats, the tests broke.

So this interdiff reverts the core formats to the way they are in HEAD, and fixes the test checking them.

It's possible that we're done here...

tim.plunkett’s picture

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.phpundefined
@@ -0,0 +1,112 @@
+  /**
+   * Local definition cache.
+   */
+  protected $definitions = array();
...
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::getDefinition().
+   */
+  public function getDefinition($plugin_id) {
+    if (!isset($this->definitions[$plugin_id])) {
+      $this->definitions[$plugin_id] = $this->discovery->getDefinition($plugin_id);
+    }
+    return $this->definitions[$plugin_id];
+  }
+
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::getDefinitions().
+   */
+  public function getDefinitions() {
+    // We do not know whether getDefinition() was called for particular plugin
+    // IDs already, so we retrieve and reset all.
+    $this->definitions = $this->discovery->getDefinitions();
+    return $this->definitions;
+  }
+
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::clearCachedDefinitions().
+   */
+  public function clearCachedDefinitions() {
+    // Clear the local property cache.
+    $this->definitions = array();
+    // Clear the persistent CacheDecorator cache, if there is one.
+    if ($this->discovery instanceof CachedDiscoveryInterface) {
+      $this->discovery->clearCachedDefinitions();
+    }
+  }

This seems like premature optimization. The CacheDecorator provides static (property-based) caching itself, in addition to cache() calls.

+++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.phpundefined
@@ -0,0 +1,112 @@
+    // @todo Remove this check once http://drupal.org/node/1780396 is resolved.
+    if (!module_exists($definition['module'])) {
+      $definition = NULL;
+      return;
+    }

This is obsolete now, I'll remove it in the next patch (waiting for tests results first)

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/Core/Entity/FilterFormat.phpundefined
@@ -119,22 +126,53 @@ public function id() {
+      $this->filterBag = new FilterBag(drupal_container()->get('plugin.manager.filter'), $this->filters);

This, and possibly elsewhere, should use \Drupal now

+++ b/core/modules/filter/lib/Drupal/filter/Plugin/filter/filter/FilterBase.phpundefined
@@ -0,0 +1,157 @@
+      'plugin_id' => $this->plugin_id,

Er, this is $pluginId in PluginBase, not sure how this works. Or is it never called?

Status: Needs review » Needs work
Issue tags: -Plugin system

The last submitted patch, filters-1868772-69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#69: filters-1868772-69.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Plugin system

The last submitted patch, filters-1868772-69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
129.32 KB
FAILED: [[SimpleTest]]: [MySQL] 54,046 pass(es), 33 fail(s), and 7 exception(s). View

Okay, there were a couple upstream changes to filter.module.

Also, adjusted to use a custom @Filter annotation.

Status: Needs review » Needs work

The last submitted patch, filters-1868772-74.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
129.32 KB
FAILED: [[SimpleTest]]: [MySQL] 54,359 pass(es), 1 fail(s), and 0 exception(s). View

Misnamed the annotation on the PHP filter.

Status: Needs review » Needs work

The last submitted patch, filter-1868772-76.patch, failed testing.

tim.plunkett’s picture

Well that passes locally, run-tests.sh and GUI. Might have to enlist jthorson's help.

Latest is in http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
129.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 29 fail(s), and 1,824 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, filter-1868772-79.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
130.22 KB
FAILED: [[SimpleTest]]: [MySQL] 55,430 pass(es), 1 fail(s), and 0 exception(s). View

Reroll for various commits.

tim.plunkett’s picture

FileSize
694 bytes
129.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es). View

Okay, this interdiff may look like its cheating, but its actually reverting the order back to the way it is in HEAD now.

Status: Needs review » Needs work

The last submitted patch, filter-plugins-1868772-82.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
130.22 KB
FAILED: [[SimpleTest]]: [MySQL] 55,711 pass(es), 1 fail(s), and 0 exception(s). View
130.22 KB
FAILED: [[SimpleTest]]: [MySQL] 55,859 pass(es), 1 fail(s), and 0 exception(s). View
130.22 KB
PASSED: [[SimpleTest]]: [MySQL] 55,797 pass(es). View
130.22 KB
PASSED: [[SimpleTest]]: [MySQL] 55,774 pass(es). View
129.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,299 pass(es), 1 fail(s), and 0 exception(s). View
129.85 KB
FAILED: [[SimpleTest]]: [MySQL] 55,305 pass(es), 1 fail(s), and 0 exception(s). View
129.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,830 pass(es). View
129.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es). View

Okay, so I retested each of those twice, and each one passed once and failed once... So that's not good.

Let's settle this.

Status: Needs review » Needs work

The last submitted patch, filter-plugins-1868772-81.patch, failed testing.

andypost’s picture

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()

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
136.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,321 pass(es), 26 fail(s), and 2 exception(s). View
136.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,828 pass(es), 26 fail(s), and 2 exception(s). View
136.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,280 pass(es), 26 fail(s), and 2 exception(s). View
136.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,787 pass(es), 26 fail(s), and 2 exception(s). View

Wow. Turns out this is actually caused by a bug in PHP's uasort.

If two members compare as equal, their relative order in the sorted array is undefined.

That's why the test is unreliable.

But why are they undefined?

Because #1911884: Enable CKEditor in the Standard install profile changed the test. Before that issue, this tested two enabled filters on a format, and their sorting. But for no apparent reason, it switched formats, and was comparing the weight of two disabled filters. Because of the way filters weights were previously stored, the test obediently gave the result expected, without actually testing anything.

Once the sort was switched, the two disabled filters were assigned "undefined relative orders", causing the random failures.

I'm now testing the same two filters, but on a format they actually are used on.

In addition, the default formats do not match their state when imported. They are pared down to the bare minimum, which is not how we've been handling other default config. I've rexported them to get all of the default values in them, and in the right order.

Finally, getAll() needs to be called *before* sort(), or else there are notices due to try/catch. See https://bugs.php.net/bug.php?id=50688.

The interdiff is against #81.

Status: Needs review » Needs work

The last submitted patch, filter-1868772-86.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
136.71 KB
FAILED: [[SimpleTest]]: [MySQL] 55,379 pass(es), 2 fail(s), and 0 exception(s). View

Well, that was unfortunate. We do need the full list of definitions, not just what's passed in.

Status: Needs review » Needs work

The last submitted patch, filter-1868772-89.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
890 bytes
136.48 KB
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es). View

Ah. That failing assertion (added by this issue) is just not how the CMI system works. Every setting is exported, whether or not it is being used.

Okay. We should finally be done here!

Eric_A’s picture

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).

Berdir’s picture

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?

tim.plunkett’s picture

FileSize
23.79 KB
134.96 KB
FAILED: [[SimpleTest]]: [MySQL] 56,418 pass(es), 2 fail(s), and 0 exception(s). View

Pretending that each dreditor block is numbered:

1) Fixed
2) This was changed by sun.
3) I think it means use it inside the function. Otherwise it would be @deprecated, not @todo
4) Fixed
5) :)
6) sun added the @todo, I'm not worried about it either way
7) Incorporated part of #1987298: Shorten directory structure and PSR-0 namespacing for plugins here and made the switch, this will need a reroll once that is in
8) Fixed :)
9) If we want to rename these, we can in a follow-up, but I'm okay with matching the pre-existing name for now.
10) Fixed
11) Heh
12) @todo
13) Yes, _filter_htmlcorrector is used in many other places
14) Moved down to \Drupal\filter\Plugin
15) I defer to sun
16) Actually, this is just for debugging sanity, in some cases it was an object, and not casting it to a bool meant it was var_dump'd for the assertion message, which hit recursive nesting
17) Fixed
18) Since this is the whole reason to implement a filter, I'd prefer it not have a default. Then OO will kick in and remind you to implement it
19) *points at sun*
20) Fixed
21) Yeah, the Filter maintainer made this change.
22) See #1235062: text_summary() ignores filter status. Sun asked to not require this to be split out.
23) Apparently it is unneeded, see ^

Status: Needs review » Needs work

The last submitted patch, filter-1868772-95.patch, failed testing.

Berdir’s picture

+++ 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
134.95 KB
PASSED: [[SimpleTest]]: [MySQL] 55,588 pass(es). View

lol, whoops.

Berdir’s picture

Status: Needs review » Needs work

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

Looks good to me otherwise.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
133.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,432 pass(es). View
Berdir’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
134.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es). View

Finally. Sorry.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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...

tim.plunkett’s picture

#102: filter-1868772-102.patch queued for re-testing.

alexpott’s picture

Title: Convert filters to plugins » Change notice: Convert filters to plugins
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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...

tim.plunkett’s picture

tim.plunkett’s picture

Assigned: sun » tim.plunkett

Seems like writing change notices is the cool thing to do today. I'll work on this.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Active » Needs review
Berdir’s picture

Title: Change notice: Convert filters to plugins » Convert filters to plugins
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good to me!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

yukare’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Issue summary: View changes

Updated issue summary