Deleting filter formats may result in data loss

I think this can be done by implementing onDependencyRemoval(array $dependencies) in Drupal\field\Entity\FieldStorageConfig.

Basically, something like:

  public function onDependencyRemoval(array $dependencies) {
    $changed = parent::onDependencyRemoval($dependencies);
    if ($changed) {
      return TRUE;
    }

    if (!isset($dependencies['config'])) {
      return FALSE;
    }
    
    foreach ($dependencies['config'] as $config) {
       // If $config is a filter format, return TRUE.
    }
  }

My suggestion to implement onDependencyRemoval() in #2 was in the wrong place. TextItemBase makes more sense on where to implement it, and that's what is in MR 12823.

This prevents the data from deleted, but there is a display issue, in that when the format is deleted, existing content with text fields using that filter format can not display the content. However, this can be addressed by editing the entity and setting the filter format to something else.

Leaving at NW for tests.

I haven't read the solution in full yet, but it is extremely important that we block deletion of the filter format when stuff still uses it rather than anything else like altering config, since changing a filter format can have security implications.

Whatever solution should also have subsystem and/or framework manager review.

So FilterFormatAccessControlHandler::checkAccess() has this:

    // We do not allow filter formats to be deleted through the UI, because that
    // would render any content that uses them unusable.
    if ($operation == 'delete') {
      return AccessResult::forbidden();
    }

I used drush to reproduce steps in the IS: drush php-eval 'Drupal\filter\Entity\FilterFormat::load("restricted_html")->delete();', which I guess bypasses checkAccess somehow. I haven't looked into why, and there's a note in the comment about deletion via UI and not CLI.

Regardless, maybe a better approach is hardening filter format entity delete access?

ETA. I think checkAccess isn't invoked by $entity->delete(), so it might be too late to prevent deletion once that is called.

This prevents the data from deleted, but there is a display issue, in that when the format is deleted, existing content with text fields using that filter format can not display the content. However, this can be addressed by editing the entity and setting the filter format to something else.

This is also kinda problematic, replacing data loss with data integrity problems and/or "perceived" data loss.

Oops, xpost. Reviewing #11 now.

#11 does indeed also sound like something we should confirm or revisit. It might be worth a git log -L on the comment to see why the issue that added it made the choice to differentiate the UI versus API deletion in this way. (There might be useful discussion on the original issue.)

Thanks @godotislate!

This will also eventually need usability review on whatever proposed solution we fix, and should be fixed in a way that's consistent with these other "config dependencies eat my config" issues.

Also, bumping to critical; nothing should be able to delete a populated storage in this way without an explicit user decision. When we uninstall a module, for example, the user has to do a bulk operation to remove module content before they can uninstall it.

@xjm I think I got confused in #11.

Looking at the code again now, it doesn't look like calling $entity->delete() invokes entity access. I believe the access check needs to happen before delete() call. Once delete() is called, I don't think there's any way to stop that except by throwing an exception.

@godotislate Ah, yeah, the internal API call should probably only do what it says on the tin and not vary according to the logged-in user account or anything like that. So changing things earlier in the callstack seems like the right approach. We might not be able to stop people from making bad out-of-context direct API calls with Drush; that is a Drush "feature" I guess.

In either case, we should manually test it carefully.

Re #6/#12 this is a security feature, because the filter format may previously have sanitised some HTML, and once that is removed we can no longer safely display the content. I believe this is also why deleting a format from the UI is not allowed either.

I guess the question here is, given that filter formats are not intended to be deleted, what handling should there be in the case that a drush command, or contrib/custom code was not sufficiently careful to prevent deletion?

Agreed with #19: if a filter format is improperly deleted somehow, this should not result in content data loss. If, for example, the project has the filter format configuration in version control, the format could be restored somewhat easily. But if all the data is gone, the restoration effort would be more difficult and would require database backups to be available.

What are next steps? Should this be submitted for usability review?

Adding #3326447: Deleting a Media view mode should not result in the deletion of an entire text format as related because it seems to follow that if you delete a Media view mode that deletes a text format, this could end up deleting content.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

#2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed is in, which resolves #3326447: Deleting a Media view mode should not result in the deletion of an entire text format and removes one vector of (accidentally) deleting filter formats.

There is code inside TextEditorObjectDependentValidatorTrait.php at lines 34- 38 which seems related?: Is there scope to update any ckeditor tests as part of this issue?

      // This validator must not complain about a missing text format.
      // @see \Drupal\Tests\editor\Kernel\EditorValidationTest::testInvalidFormat()
      if ($text_format === NULL) {
        $text_format = FilterFormat::create([]);
      }

I wonder if it would make sense to show on entity view a message that the formatted text field can not be displayed because of the missing filter format. This message would be limited to users with a specific level access, perhaps having permission to edit the field? Though I think that could be something to be done in a follow up while the data loss concern gets addressed here first.

Re: #25 That sounds like it would tick a few boxes test wise.

Fixed the logic in onDependencyRemoval a bit and rebased.

Tagged for usability review to evaluate whether something like #25 is needed and updated the IS to note that as a remaining task.

We discussed this issue at #3552911: Drupal Usability Meeting 2025-10-24 (@rkoller and @simohell) and #3554117: Drupal Usability Meeting 2025-10-31 (@benjifisher, $rkoller, and @simohell). Those issues each have a link to a recording of the meeting. I am giving issue credit here to the attendees at the usability meetings.

This comment is not a usability review. Instead, it is about the results of some manual testing. (At the second meeting, we were a little confused: when we did not see data loss, we thought the issue was outdated. We forgot that we should have tested with 11.x instead of the branch from the MR.)

The issue summary refers to #3326447: Deleting a Media view mode should not result in the deletion of an entire text format. That issue was closed as a duplicate of #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed, which is now Fixed. We have not tested to confirm, but it seems as if the only ways to delete a filter format are using PHP code (as in the Steps to reproduce) or perhaps using the jsonapi module. I am adding the tag for an issue summary update and setting the status back to NW. Perhaps the priority should be downgraded from Critical, too.

The Steps to reproduce are incomplete if there is more than one field using the same field storage. For example, this is the case when testing with the Standard or Umami installation profile. If only one of the field configurations is removed, then the field storage configuration is not removed, and that means that the database table is not removed. In this case, it is possible to restore the data from the deleted field:

1. Import configuration to restore the field, along with the entity form and display modes. (Untested, but it is probably has the same effect if you create a "new" field with the same machine name through the UI.)
2. Make sure the field is configured to allow text formats that have not been deleted.
3. Update the database table, setting deleted = 0 for all rows (or only the ones affected by deleting the filter format, if you can think of a way to do that).

Related: the issue summary states,

Reason is that the field storage has a config dependency on the filter format, when it is configured as allowed-format.

The field, not the field storage, has a dependency on the filter format.

I also made some comments on the MR. These are not related to the usability meeting, and they are just my own opinions.

Thanks @benjifisher for the review and to you, @rkoller, and @simohell for looking at this in the usability meeting.

Were you able to discuss the question in the "For usability review" section in the IS? #12 raised a question about data integrity problem or perceived data loss because of the behavior not to show the content of a formatted text field on entity view when the field's text format does not exist. I proposed one solution: on entity view, display a message or markup to users who have access to edit the field value that the format is missing.

Updated the IS and downgraded to Major since #3326447: Deleting a Media view mode should not result in the deletion of an entire text format was fixed via #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

@godotislate:

Thanks for updating the Steps to reproduce. I made a few further tweaks and also updated the Proposed resolution. I am removing the tag for an issue summary update.

Personally, I agree with the last sentence in your Comment #25:

Though I think that could be something to be done in a follow up while the data loss concern gets addressed here first.

Perhaps we should open a follow-up issue and have the usability review there.

Still not a usability opinion (and still my personal opinion): removing a filter format (however that happens) is an edge case, so it is not worth a lot of extra code to alert site admins/editors to the problem on the View page. The code would have to worry about permissions, caching, and whether the notice would appear in other places, like listings from Views.

It is much simpler to work with the node-edit form, which is already restricted to appropriate permissions. Often, there is already some indication on that form: a Text format select list with nothing selected (and I think that will generate an error if you try to submit the form). We could add a more explicit warning in this situation, but I am still not sure it is worth the effort.

Usability review

The decision at #3552911: Drupal Usability Meeting 2025-10-24 is that we should add a notification on the view page when a field has an invalid filter format. That notification might be in a status message or it might be in place of the field that is not rendered. In either case, the notification should be restricted to users with edit permission for the entity.

Following Comments #25 and #30, we can address this in a follow-up issue, so I added #3556673: Alert users with edit permission when a field is not displayed because of invalid text format.

I am removing the tag for a usability review. I am not sure this issue needs all of the other issue tags currently attached, but removing any of those is outside the scope of a usability review.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Addressed or asked for clarification for outstanding MR comments.

Putting this back into review.

This issue is scoped to prevent data loss in the case that a filter format is deleted, though that is not supposed to happen. Since #3326447: Deleting a Media view mode should not result in the deletion of an entire text format has been addressed, a possible cause for a filter format being deleted has been removed, but it is possible that there are other ways that a filter format could be deleted (such as other bugs, contrib/custom code, or CLI commands.)

Removing Needs security review because it seems like security issue might have been addressed in #17

Removing Needs change record because there's no API change here.

@godotislate:

I replied to your comments on the MR. I also added a draft change record in case there are contrib or custom modules that extend TextItemBase and override onDependencyRemoval.

Back to NW.

Addressed MR comments. Back to NR.

@godotislate:

Thanks for the updates.

There is still one open thread on the MR, but I do not see a specific suggestion for changes. This issue is also tagged for subsystem maintainer review, but I am not sure that is needed.

Reminder: @godotislate and I agree (Comments #25, #30) that the usability concerns raised in this issue can be postponed to the followup issue #3556673: Alert users with edit permission when a field is not displayed because of invalid text format.

I did one more round of manual testing, following the Steps to reproduce in the issue summary. As expected, the new field was not displayed (but the label was) when viewing the node, but there was no data loss: the field config, field storage config, and database table were all preserved. Editing the node and selecting a valid text format restored the text when viewing the node.

3 subsystems and I never am needed for sub maintainer review. Benjifisher pinged me about this one. Assuming to look at the approach of using the onRemoval approach +1 for me, pretty inline with how we do other places.

Did take a lap at saving credit.

Just reiterating for posterity: We do not defer usability reviews to followups. Usability is a core gate, and hard-blocking. 🙂

Very rarely, we will grant an exception and temporarily defer gates to followups to more quickly mitigate data loss or the like. In this case, though, user expectations about what should happen are deeply intertwined with what the user considers data loss versus intentional deletion, so it is an essential part of the issue.

Setting back to NR to get it picked up for another usability review (per #37) with the tag.

Does #28/#31 not count as usability? Also issue was worked on by benjifisher who is on the UX team.

Sorry, that's on me. I missed that #31 was a follow up usability review, and for whatever reason I thought the review was going to be done in the follow up. Anyway, NW for #31. Basically, bring #3556673: Alert users with edit permission when a field is not displayed because of invalid text format back into scope here.

Tried to address that warning part.

Rebased

Added a comment to the MR.

Also, we should make sure that the message only shows to appropriate level users.

Sounds like needs more work. Will try and get to it after my trip if no one beats me to it.

Yes, sorry, meant to set back to NW.