Missing filters result in Exception when the format is used

This is a possible fix.

Edit: it fails on admin/config/content/formats/manage. Yet, it's a start.

For review.

Perhaps use a different factory and use that to instantiate this filter_null. Probably better than messing with the Bag. Also we need to hide it from the UI... somehow.

The bag is the correct place, but the try/catch should be in initializePlugins, see BlockPluginBag

There was a meta issue about proper support for all plugins.
Each kind of plugin needs separate handling of absence the plugin handler, we have this implementation from VDC so probably it makes sense to implement base class/interface

At least no text should be displayed if there's no filter
Same applies for image effects - we should throw 403 to frontend if there's not effect implementation
But both needs ability to remove broken plugin prom list as view does

I attach fixing concept patch for make sure if I my undestanding is right.
It based on #3 so at this momment it must be applyed after that.

Tim, check if I think right please.

In short, I replace broken filter with filter_null and inform admin in messages about filter replacing.

Thanks, I think that looks good but surely you dont need that current now?

Also, } else { needs a newline.

Finally, I think that this way the null filter will show up and confuse people; we need some way to hide from the UI.

I'm pretty sure the try/catch can just go around the parent::pluginInitialize(), it should wrap the least amount of code possible.

@chx sure current in my patch come from #3 - it dont need and will be removed

As for hide broken filter in admin my idea is show in admin as disabled checkbox with title (near checkbox) something like "filter_id filter broken ". So we give ability to user delete broken filter and get config clean

timplunkett says this is related to a new issue I just filed:
#2058285: Enabling php.module results in uncaught exception on Modules/Extend page

Re #12 yes, that's one problem but the other problem I have is that if we add a filter_null filter then it'll show up for any format as a possible filter to be added and that's mighty confusing. We either hardwire skipping it or add a no ui capability to filters.

The tests need updating, but if a missing plugin is requested, the definition for the null filter is returned and watchdog is triggered.

1. @@ -115,6 +116,13 @@ public function form(array $form, array &$form_state) {
   +      if ($filter instanceof FilterNull) {
   +        $filters->removeInstanceID($filter_id);
   

   We might want a d_s_m here

2. @@ -14,18 +14,6 @@
   - * @Filter(
   - *   id = "filter_html",
   - *   module = "filter",
   - *   title = @Translation("Limit allowed HTML tags"),
   - *   type = FILTER_TYPE_HTML_RESTRICTOR,
   - *   settings = {
   - *     "allowed_html" = "<a> <em> <strong> <cite> <blockquote> <code> <ul> <ol> <li> <dl> <dt> <dd> <h4> <h5> <h6>",
   - *     "filter_html_help" = 1,
   - *     "filter_html_nofollow" = 0
   - *   },
   - *   weight = -10
   - * )
   

   Oops! This was debug code :)

3. @@ -73,6 +73,22 @@ public function testFilterFormatUpgrade() {
   +    check_markup($this->randomName(), 'format_two');
   

   This should assert that it returns an empty string.

Here we are. Should be good to go now.

1. +++ b/core/lib/Drupal/Component/Plugin/DefaultPluginBag.php
   @@ -133,4 +133,12 @@ public function setConfiguration($instance_id, array $configuration) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function removeInstanceID($instance_id) {
   +    parent::removeInstanceID($instance_id);
   +    unset($this->configurations[$instance_id]);
   +  }
   
   +++ b/core/lib/Drupal/Component/Plugin/PluginBag.php
   @@ -125,6 +127,17 @@ public function setInstanceIDs(array $instance_ids) {
   +   * Removes an instance ID.
   +   *
   +   * @param string $instance_id
   +   *   An image effect instance IDs.
   +   */
   +  public function removeInstanceID($instance_id) {
   +    unset($this->instanceIDs[$instance_id]);
   +    $this->remove($instance_id);
   +  }
   
   +++ b/core/modules/image/lib/Drupal/image/ImageEffectBag.php
   @@ -26,17 +26,6 @@ public function &get($instance_id) {
   -   * Removes an instance ID.
   -   *
   -   * @param string $instance_id
   -   *   An image effect instance IDs.
   -   */
   -  public function removeInstanceID($instance_id) {
   -    unset($this->instanceIDs[$instance_id], $this->configurations[$instance_id]);
   -    $this->remove($instance_id);
   -  }
   

   For some reason when I abstracted the plugin bags, I left this method only on image effects. It is very useful, and needed here.

2. +++ b/core/modules/filter/lib/Drupal/filter/FilterPluginManager.php
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Plugin\Exception\PluginException;
   

   Unneeded, whoops

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FilterFormatUpgradePathTest.php
   @@ -73,6 +73,24 @@ public function testFilterFormatUpgrade() {
   +    // Check that missing
   

   Ugh, need to fix this comment still.

Yay, fail/pass combo in 18 as expected.

> Wondering if this could use a different description, as *nothing* is allowed :) Not sure what to publicly display in this case, though.

I hope, that we could return just '' here cos we do not want these tips, this filter, ever to show up for the user, do we?

I have a suggestion for FilterNull class documentation:

/**
 * Provides a fallback placeholder filter to use for missing filters.
 *
 * The filter system uses this filter to replace missing filters (for example, if a filter module
 * has been disabled) that are still part of defined text formats. It returns an empty string.
 *
 * (annotation)
 *   title = @Translation("Provides a fallback for missing filters. Do not use."),
*/
class FilterNull ...

(check wrapping on that paragraph)

I also noticed that in the constructor, it logs a message about the filter but doesn't set the $logged variable to TRUE. Oops! There doesn't seem to be much point in having $logged if it isn't used... but since the log message is only in the constructor, maybe this is not needed?

I also think the tips() method should return t('Missing filter. All text is removed.') or something to that effect. And maybe also "Visit the Text Filters administration page to remedy.", possibly with a link?

I checked my watchdog, had way too many entries. Duh!

Responded to #21-23, thanks.

@chx Yes ideally this will never show up, but if it does we might as well explain ourselves.

I am happy with the patch in #24, documentation-wise. I have not tested the functionality of the patch, but the tests appear to be adequate.

Actually, it would be nice to see a patch with just the tests to verify that it fails without the code fix portion of the patch. Oh, wait, I see that this was done in #18. (That patch did include the FilterNull class but I don't think that changes the test result, which clearly matches this issue report, so that is good.)

I'll let other reviewers set this to RTBC.

#24: filter-2015313-24.patch queued for re-testing.

Testbot environment failure (missing menu_links table!)

I'm not sure the filter tips is enough - what about disabling the textarea on edit/create with a broken format?

Then do we have to deal with #states for the text format select list?
This fixes a bug, that sounds like a whole feature request we'd want to pull the UX team in on...

Yeah I wouldn't waste a lot of UX effort on a corner case like this.

Just trying to remember what we did in Drupal 7, I think we only prevented the text from being rendered so yeah, committed/pushed to 8.x.

Thanks!

Doh I was thinking of missing text formats which check_markup() checks for.