Disabled text formats can't be seen in the GUI

This patch change label and text for more reliable "Delete" instead "Disable".

Errors fixed.

1. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -126,6 +126,11 @@ public function getDefaultOperations(EntityInterface $entity) {
   +    // Change operation label to Delete which more reliable for user.
   +    if ($entity->hasLinkTemplate('disable')) {
   +      $operations['disable']['title'] = t('Delete');
   +    }
   +
   

   Isn't there a link template 'delete' which can be used?

2. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -265,7 +265,7 @@ function testFilterAdmin() {
   -    $this->drupalPostForm('admin/config/content/formats/manage/' . $format->id() . '/disable', array(), t('Disable'));
   +    $this->drupalPostForm('admin/config/content/formats/manage/' . $format->id() . '/disable', array(), t('Delete'));
   
   +++ b/core/modules/filter/src/Tests/FilterHooksTest.php
   @@ -69,7 +69,7 @@ function testFilterHooks() {
   -    $this->drupalPostForm('admin/config/content/formats/manage/' . $format_id . '/disable', array(), t('Disable'));
   +    $this->drupalPostForm('admin/config/content/formats/manage/' . $format_id . '/disable', array(), t('Delete'));
   
   +++ b/core/modules/filter/src/Tests/FilterSecurityTest.php
   @@ -75,7 +75,7 @@ function testDisableFilterModule() {
   -    $this->drupalPostForm('admin/config/content/formats/manage/' . $format_id . '/disable', array(), t('Disable'));
   +    $this->drupalPostForm('admin/config/content/formats/manage/' . $format_id . '/disable', array(), t('Delete'));
   
   +++ b/core/modules/text/src/Tests/TextFieldTest.php
   @@ -116,7 +116,7 @@ function _testTextfieldWidgetsFormatted($field_type, $widget_type) {
   -        $this->drupalPostForm('admin/config/content/formats/manage/' . $format->id() . '/disable', array(), t('Disable'));
   +        $this->drupalPostForm('admin/config/content/formats/manage/' . $format->id() . '/disable', array(), t('Delete'));
   

   The path should also be /delete

@vegantriathlete,

It's NOT actually deleting the text format.

I only reviewed the code, not the functionality even though i guessed as much. Which is why i hinted at using an actual delete link.

Is that an accurate title?

Ok, let's try one more time )
Now disabled format will appear in the formats list, with warning message.

The screenshot below shows the confirmation page. It's text is no longer correct.

The screenshot below shows the "Text formats and editors" page after disabling a text format. There is no way for the user to re-enable a text format, nor is it possible to create a new format with the same name. We should add an action button to enable the text-format once it has been disabled.

1. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -111,6 +101,12 @@ public function buildRow(EntityInterface $entity) {
   +      $roles_markup = SafeMarkup::placeholder($this->t('This format is disabled and no more available.
   

   This format has been disabled and is no longer available.

2. +++ b/core/modules/filter/src/FilterFormatListBuilder.php
   @@ -111,6 +101,12 @@ public function buildRow(EntityInterface $entity) {
   +        Any content stored with that format will not be displayed.'));
   

   Any content stored with this format will not be displayed.

Enable action added.

#17 seems to work in manual testing. I am tagging "Needs usability review" so a specialist can chime in.

Actually, there is no workaround for GUI users (imagine "I want my text format back!"), so this is major.

I hope it will pass review )

The status quo is the same behavior in Drupal 7. But, why disallow the re-enabling of a text format?

+++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -194,6 +196,21 @@ public function disable() {
+    // Allow modules to react on text format deletion.

enabling?

+++ b/core/modules/filter/src/Form/FilterEnableForm.php
@@ -0,0 +1,57 @@
+    return $this->t('You are going to enable text format.');

That seems obvious to me, I think this line should explain the result of that action. i.e. Any content stored with this format will be displayed again.

Also, this needs tests

Text fixed and test for format enable operation added.

Looks like writing tests it's not my best feature. Just adding patch with fixed text.

@shumer,

Reviewing your changes would be a lot easier for me if you would provide an iterdiff. As for the tests, I can probably take a look at those tomorrow.

@legolasbo, interdiff attached

Attached patch adds tests and some small textual changes.

This needs a beta evaluation or it should be pushed back to 8.1.x. I am thinking we should push it back because there is so much going on right now and HEAD is the same as the Drupal 7 behavior.

Added beta phase evaluation

I thought I'd chime in here to say that while I appreciate the impulse to push a solution for this to 8.1.x, it brought up another problem for a simple single-user site I'm building which feels like a showstopper:

The warning message when disabling a text format says "any content stored with that format will not be displayed." However, this is wrong (at least in my case), as content using a format I disabled is still live on the site (localhost), even after clearing the cache. However, what's odd is I am now prevented from editing any nodes that had used that format.

When I try to edit one of these nodes, I get the dreaded "The website encountered an unexpected error. Please try again later." message. I can just delete the nodes, of course, and a user shouldn't be too surprised to lose field content if they delete a text format, but for many users struck by this, content in other fields in affected nodes might be very important (assuming this issue is not just limited to me, which is possible).

The warning message says "this cannot be undone," which I assumed meant was that I was actually deleting the format, and could recreate it using the same machine name, but as pointed out, this is impossible, and leaves cruft in the database.

The attempt to explain what is actually happening in the warning text is a strange thing because what it actually does is reinforce Drupal's notorious reputation for obtuse helper text, so even if it were accurate, it's easy to ignore it's intended meaning.

At the very least, it should be an "expected" error, not an "unexpected" one, and if it's not just me, indicates that this is a bigger issue than already recognized.

@Mark LaCroix,

We can still address this in 8.0.x as per the beta phase evaluation. If you could review the code that would be another step in the right direction.

Can you provide steps to reproduce the unexpected error? If you can, please open a new issue for it and reference it here. What you are describing sounds like something that's major or even critical.

Done and done! Thanks for the clue:

https://www.drupal.org/node/2555973

Patch updated wrt latest code in 8.0.x branch.

@pjonckiere,

That code was added in #1987124: Convert filter_admin_format_page() and filter_admin_overview() to a Controller, when the callbacks were converted to a controller. The issue we are currently working on was actually discovered by dawehner while reviewing that patch. That method was added to keep the functionality the same as in D7. This behaviour is really strange however and should also be fixed in D7 in my opinion, which warrants it's removal.

Adding two related issues from the D7 era that may shed some light on why things currently work as they do. This is a complex topic :-)

Re-roll.

The enable and disable routes need CSRF tokens, because they respond to GET requests, but change the site's state.

+++ b/core/modules/filter/src/FilterFormatListBuilder.php
@@ -134,6 +129,16 @@ public function getDefaultOperations(EntityInterface $entity) {
+    // Remove Disable and Configure operation for already disabled formats.
+    if (!$entity->status()) {
+      if (isset($operations['disable'])) {
+        unset($operations['disable']);
+      }
+      if (isset($operations['edit'])) {
+        unset($operations['edit']);
+      }
+    }

Why was this added? We do not prevent any entities of other types from being edited when they are disabled, and the parent method already hides the disable operation for already disabled formats.

Patch no longer appies, rerolling and fixing tests.

Rerolled and fixed tests. Also made a small interface text change.

Attributed organisation.

Also fixed the remaining tests.

Oops

Nothing to review, sounds like a straight bug.

I think #45 is the latest patch and this is the reroll.

I am ignorant of how to handle CSRF in WebTests. Does anyone know how that is worked-around?

I was overthinking it.

Tests should pass now.

It is possible to reenable disabled text formats with the configuration manager:

• go to /admin/config/development/configuration/single/export
• choose Text format as the Configuration type
• choose the disabled text format as Configuration Name
• copy the configuration
• go to /admin/config/development/configuration/single/import
• again choose Text format as the Configuration type
• paste the configuration
• change "status" from false to true
• click on import

Voilá, the text format is available again. This is working but not user friendly :(

PS. It may be that I have not used the correct English labels; I'm currently working with German translations.

We should backport this.

TBH, I am not sure if I've tracked that issue correctly because it looks like it has been gone off course a little bit. To come back to the OP text:

When you disable a text format it is not possible to re-enable it.

Exactly. This is an obvious UI logic caveat against common UX standarts regarding the common understanding of visually "disabling" vs. "deactivating or deleting" something.

Since (like reported) the format is indeed NOT deleted and still there but disabled, with unxpected side effects like being hidden, disabling content which is using it, being unable to enable at the same place etc. We can talk about multiple issues here:

1. UI/UX issue: It should be greyed out, but not hidden, as long as the format exists in db.
2. it should be poss. to re-enable it on the same place: this is the UI-logic of "disabling" something.
3. It should not disable the content which uses it already, unless it is wanted (in a second step or other option)

Respecting all issues and side effects not obvious for all from the first look the selection dropdown should include:

1. delete (delete the format completely from db but keep the content with a warning and a fallback format for this content, like we did with deleted users in D7 with the content keeping option). Why? because how else would we finally delete a format then if not here? #UX
2. disable/turn off (disable the format and its content and make the format no more selectable for editors)
3. inactive (disable the format but keep its content with format fallback and make the format no more selectable for editors)

And only the delete option should make this text format disappear from the list of text formats if we want to stay in common practice of UI concepts and UX.

@diqidoq I agree with most of what you wrote. We should do one of the following:

• Change the issue to include everything in #62.
• Use this issue as a "quick win", change the UI a little but keep the current core functionality. Open a follow-up to do #62.

Thanks to Pepe Roni, #60 works.

By the way, if you have any text in those fields and they disappeared after disabling your text formats, don't panic. Just run update.php after re-enabling them and your stored content will return.

Also, if you'd rather make changes to the database, under D8, text format configurations are now in the "config" table.
Run this SQL Query:
SELECT `collection`, name FROM `config` WHERE name LIKE 'filter.format%';
and you'll find all four default text formats listed.
Your data is stored in a BLOB field. You can download it to examine what's inside. If you're comfortable with db queries you can run an UPDATE to change status from 0 to 1 but be careful with SET commands, they're powerful and can overwrite other data unless you know what you're doing. You can also expose BLOB contents by changing settings in your phpMyAdmin config file. Here's a helpful post to get you started:
https://groups.drupal.org/node/134269

Needs a new patch to reflect the comments in #62

Currently working with 8.3.x and faced the same issue so I have done all this by code.
I know its not a good solution but its works for me.

  $config_factory = \Drupal::configFactory(); 
  foreach ($config_factory->listAll('filter.format.') as $name) {
    $editor = $config_factory->getEditable($name);
    $editor->set('status', 1)->save();
  }

There is just another problem: I get a lot of messages in dblog if I "disable" a text format:

Filter 21.07.2019 - 13:28 Disabled text format: basic_html.

Really boring ...

Hugh, problem still not solved?

Just forgott the problem and deleted text format basic_html again and had problem again.

Content of pages with Text format are not shown. Some admin pages make fatal error. No error messages in dblog via web interface but via drush. And a lot of other strange behaviour.

Fortunately I have a new back.

Is it so impossible to prevent the default text formats to be deleted?
Or to put meanwhile a warning message to the admin page of text formats?

Upgraded to critical. Because deleting default text format can cause a lot of problems - whole content of body texts of website not shown - if you dont have a new backup!

In reply to previous post (#77), there's a trick to get the disabled text format back :

1. Go to Admin ->Config -> Dev -> Sync config (http://mywebsite.com/admin/config/development/configuration/single/export)
2. Select the Export tab and choose Single item
3. Then select Text format and the format you want
4. Copy all the config text
5. Now, go to the Import tab and choose Single item
6. Select Text format and paste the copied data in the textfield
7. Change the third line status: false to status:true
8. Click the Import button and it's done !

Drupal 7 users can re-enable the disabled formats by setting status to 1 in the filter_format table for the disabled file formats.

Re-rolled the patch.

Verified and test by applying the patch #82.It is working fine and looks good.

Pre-requisite -
Clear the cache of the site after applying the patch once.

Steps to test -
1. Go to the admin site.
2. Go to the config/content/formats.
3. Click on icon against the configure.
4. Click on disable.
5. Text format gets disable.
6. You will see the enable button.
7. On clicking on enable , we will get a pop-up and again click on enable.
8. You can disable and enable the text format field anytime.

The needs so automated tests to prove it works.

Also there are a number of other complications here. That kind of block or at least need to go long side this work:

• Like the FilterFormat::disable() method the new ::enable() method does not actually save entity. This is a good thing BUT it makes the filter_format_enable hook incorrect. The filter format is not yet enabled - this only happens when it is saved. Therefore:
  1. We shouldn't invoke an enable hook. You can use the entity save hooks instead and check the original entity.
  2. We need to file an issue to deprecate the disable hook so we can remove it in Drupal 10 - there are no implementations in contrib which is not surprising as it doesn't really work - http://grep.xnddx.ru/search?text=filter_format_disable&filename=
  3. We don't need to call filter_formats_reset() - that's already called in \Drupal\filter\Entity\FilterFormat::postSave() and in order to enable a format you need to save it.
• We need to do the work to understand why re-enabling filter formats is not part of the UI. I remember from discussions with @sun that this is a complex security issue and was designed to work this way. Reverting this choice needs to be clearly documented as to why this is safe in the issue summary.

Also note that this issue feels like a duplicate of #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) which has some relevant security discussion and UI discussion. It feels like there really should only be one issue. #914458: Remove the format delete reassignment "feature" Has some of the security discussion.

Reading those issues it seems that it is seems re-enabling a text format is considered ok from a security perspective. The problem is deleting a text format. However the other issue points out that one of the features of disabling a text format is that it is then hidden in the UI. So perhaps instead of having disabled formats mixed in the with enabled formats we should have a separate page that lists them instead.

It's a shame that this issue was not declared a duplicate of #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) before there was so much progress here. Now it's exceptionally hard to know what to do.

I guess one way of the duplicate issue is for this issue to be scoped back to the issue title which is

Disabled text formats can't be seen in the GUI

. So this issue could make the list visible and #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) could deal with providing a re-enable.

I vote that this issue be closed as a duplicate.
The work that's been done here is worth while, so also copy the latest working patch (#82) from here to that patch.
Obviously cross reference both patches.
Let this work live on in the other issue, and this one can worry us no more, and reduce issue queue clutter.

Would be worth at the same time, perhaps making reference on the other ticket of people who have contributed this to this issue.
To ensure when that ticket is evenuntually fixed, due credit is issued.

I know that typically the newer issue should be closed as a duplicate but in this case there is much more recent work in this ticket, so I'm closing #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) instead. I've moved credit from that issue also.

I stumbled into this and related threads when either I improperly installed or encountered a bug in a contrib module with the result that I could not save changes to the full_html text format. After some efforts I wound up disabling full html then could not view any content on my site ... or rather the content was badly mangled.

Somehow I managed to get full_html restored and my site working. But experimenting with a test version and looking at the database it appears that, even if you disable the text format, ignoring the warning emitted: while the text format is gone from the admin/config/content/formats/add screen and can't be added back because it is seen as a duplicate, the text format remains in the text format field of tables such as node_body. So you've got a bunch of content items which depend on a text format that doesn't exist in your system.
I should say that it doesn't exist operationally but it exists "enough" to keep you from re-enabling it.

Perhaps the fix should include not allowing any text format to be disabled that has content items using it? Re-enabling a disabled text format might be useful too, but the real hole that's dug here is disabling text formats that content items depend on in the first place.

I'd just like to mention; currently this behaviour makes the minimal installation profile useless. Just because noone mentioned anywhere that many modules reley on a text format called full_html. So if you do not create it or disable it in accident the installation is broken. Perhaps minimal should install all standard text fromats to avoid that bug. Maybe this is somewhat for another issue.

Since there is more and more understanding in the community about that text formats can not and should not be removed but rather disabled only and be re-enablable (all for good reasons) we should discuss somewhere else asap if we should still allow modules to install their text formats without warnings, since these modules aren't able to uninstall no more forever.

I created a new text format, which is no longer used on my website. I replaced it in all my posts.

How can I permanently delete it ? I don't want to disable it, I want to delete it.

It is no longer used and it is totally useless.

Why don't you leave the choice to the administrators ?

This thing is incomprehensible. Another weird Drupal thing. No logic there.

@zenimagine

This is an open, critical bug. Comments like the one you wrote are unhelpful. Instead, contribute to the effort to fix this.

It would be good to move this issue forward since it's marked as a critical, so I've rolled the patch against 10.1, changed the text to be more clear and attempted to address the feedback in #86.

I've also added a test for the enabling and disabling of the filters. We could probably expand this to test the formats are correctly enabled when doing through the enable form in the UI.

Hi i will review this patch.

Hi i applied patch#110. it is work fine and looks good.

Steps to test -
1. Go to the admin site/config/content/formats.
2. Click on icon against the configure.
3. Click on disable.
4. Text format gets disable.

Without apply patch there no option to enable.

After apply patch

1. You will see the enable button.
2. On clicking on enable.
3. we will get a pop-up and again click on enable.
4. You can disable and enable the text format field anytime.

adding screenshot for the reference.
Thank You

Thanks for the reroll and working on the issue! I've also tested the patch in #110 and it applies to 10.1.x-dev. Before applying the patch I've disabled a newly created text format. After applying the patch the text format that disappeared on disable was visible again with the default option Enable on the operations button. So the problem from the issue summary is fixed.

The only detail I wonder about there is no visual cue to distinguish the state a text format is in aside reading the operations button label (and there is also the slightly smaller button width due to the shorter character count of the Enable compared to the Configure label). For the Views overview page (/admin/structure/views ), a content types manage form display page (eg /admin/structure/types/manage/article/form-display), and a content types manage display pages (eg /admin/structure/types/manage/article/display) you have dedicated enabled and disabled sections. Manage form display and manage display have the same draggable item pattern like the text formats and editors page - in contrast on the Views page the items aren't draggable. I think it would be a good idea for the text formats and editors page to keep that interface pattern consistent with the aforementioned pages. But I am not sure if it would make sense to solve it within this issue or if creating a follow up issue would be the better choice? With a follow up the existing critical issue of a disappearing text format wouldn't be delayed any further.

#110 patch works perfectly fine, hence moving ahead.

@DanielVeza, thanks for the reroll. Remember to add an interdiff so the reroll can be reviewed.

The Issue Summary on this issue needs to be completed. A description of the proposed resolution is needed so the reviewer/committer knows what is expected. I am adding the tag and updating the remaining tasks.

This is making UX changes and should have a UX review, I am adding the tag.

The patch was rerolled and changes made to address #86. The reroll and those changes have not been reviewed.

Because this is critical, I am adding the interdiff.

A reminder that a patch must pass the core gates before it can be RTBC.

Usability review

We've discussed this issue at #3360113: Drupal Usability Meeting 2023-05-19. That issue will have a link to the recording of the meeting.

For the record, the attendees at today’s usability meeting were @aaronmchale, @benjifisher, @blackbamboo, @rkoller, and @simohell.

There were only two details noted during the discussion:

1. There was a consensus to clarify the implications what the sentence is communicating that is used on the confirmation page for disabling a text format. The current version of the sentence is:

Any content stored with the %format format will not be displayed.

The recommended change is:

Any content saved with the %format text format will not be displayed on the site until it is resaved with an enabled text format.

The new version clarifies that content which was saved with the particular text format won't be displayed on the frontend to any visitor until that content is resaved with an enabled text format. The change could already be done in this issue or moved to a followup issue.

2. In regards of the visual representation of the list of enabled and disabled text formats there was an agreement that the current state has downsides making it challenging to distinguish between enabled and disabled text formats. Currently there are no visual cues helping the user to clearly distinguish in between enabled and disabled text formats - you just have grey buttons so you have to read each of them.
one option how to tackle this is the suggestion brought up in #113. The state (enabled/disabled) would be clearly visible and it would be consistent with the pattern used for example on the Views-page. But the downside to this approach, in contrast to Views a site usually doesn't have that many text formats, plus in case no text format is disabled, there would be an empty disabled table at the bottom of the page. An alternativ option that was brought up instead was adding a column containing the information if a text format is enabled or disabled. The group agreed moving the discussion how the enabled/disabled state is communicated to a follow-up issue with resolution TBD.

Aside those two details the patch is in excellent shape with no complaints. I'll remove the needs usability review tag.

#110 did the job! Broken site is working again. Did not see that coming when I clicked "disable" :)
Thanks very much for the hard work on this long standing issue.

Thanks everyone for working on fixing this critical issue. I just got hit by it (see #3271741-15: Module doesn't delete own config after uninstall) and it would be awesome to move it forward.

I agree that we can open a follow-up for addressing #116.2. 👍 This is still a net win even if the UX isn't 100% perfect.

#116.1 seems like a great suggestion so let's address that here, since we're changing the text anyway.

Follow up #3406211: Improve representation of the list of enabled and disabled text formats for [#116.2]

@omkar.podey created followup #3406211: Improve representation of the list of enabled and disabled text formats so I'm removing that tag.

I updated the issue summary, so I'm removing that tag.

Concern

I manually created content with a text format, then disabled that text format. I am still able to access the content created with that text format despite what the confirmation warnings say. Perhaps this is because it's a full html format I tested? Will test further. I did get it so content using a disabled text format is not visible. It required a few extra steps - Disabling a text format does not seem to clear all caches that might be impacted by this. That is out of scope for this issue, and it's probably best if the warning remains as-is and makes the user aware of what will ultimately happen, even if it's not immediate due to caching.

👇 Look at this page content created with a disabled text format. It is still being displayed despite the confirmation page warning stating otherwise

This needs additional tests

The only test added testEnableAndDisableOfFilter() successfully confirms enable/disable operation links appear as expected, but we also need to test and confirm

• The before/after when enabling a text format
• What actually happens when a text format is disabled, because the current warnings don't seem to be accurate (or if they are, then the test can exist to demonstrate my manual tests are flawed)

Disabling a text format does not seem to clear all caches that might be impacted by this.

This nerdsniped me 🤓

1. config:filter.format.full_html should be invalidated when disabling.
2. config:filter.format.full_html should be associated with that output.
3. Therefore, that should have worked.

👇 Look at this page content created with a disabled text format. It is still being displayed despite the confirmation page warning stating otherwise

I'm not seeing that? I'm seeing that the node is still rendered, but the field of that node (which contains text processed by the now disabled text format) should not be rendered. And that works in a manual test I just did locally?

\Drupal\Tests\filter\Functional\FilterAdminTest::testDisabledFormat() already tests this?

Thanks for the extra context @Wim Leers -

\Drupal\Tests\filter\Functional\FilterAdminTest::testDisabledFormat() already tests this?

That existing test confirmed that disabling a filter would result in the field being unavailable, but there wasn't anything for the use case of re-enabling the format in the UI and confirming it was again visible. I expanded the test in the MR to cover this scenario so everything I mentioned in #125 is now addressed.

Oh man if this gets fixed it would be huge! Should there be a follow up for deleting the format? Seems out of scope of this.

Only moving to NW as there appears to be a test failure.

Haven't tested yet.

Also wrote a test, sorry couldn't push earlier but it's looking good already 👍, thanks.

    $this->drupalGet('admin/structure/types/manage/page/fields/node.page.body');
    $this->submitForm(["settings[allowed_formats][basic_html]" => 'basic_html'], 'Save settings');
    $node = $this->drupalCreateNode(['type' => 'page']);
    $nid = $node->id();
    $body = $node->get('body')->getValue()[0]['value'];
    // Assert that body text is visible.
    $this->drupalGet("node/$nid");
    $this->assertSession()->pageTextContains($body);
    // Disable Basic HTML text format.
    $this->drupalGet('/admin/config/content/formats/manage/basic_html/disable');
    $this->getSession()->getPage()->pressButton('Disable');
    $this->drupalGet("node/$nid");
    // Asserts the body text disappears when visited again.
    $this->assertSession()->pageTextNotContains($body);

The test I added demonstrated that different caches need to be considered when re-enabling a text format, so I'm glad the test was there. I addressed this by triggering a pretty heavy cache clear when calling enable() on a text format. Maybe there's a way for it to be less broad despite the difficulty targeting something that formerly used a format. I'd like to think this is reasonably safe since re-enabling is probably an uncommon occurrence, most likely done in a non-production environment, and it's not unheard of to lose some cache when content-impacting config changes.

Would this be worth a follow up ticket but the ability to configure a disabled format? Have definitely hit the situation before where I wanted to uninstall some ckeditor module and a disabled format had it as a dependency. If so I'll happily open it.

But testing the MR as is

Disabled basic HTML and verified I can still see it in the list.
Went to create a node it's not there, good.
Went back and enabled basic HTML, it shows as enabled again.
Went to create a node and it's there again.

This looks good to me and even though some follow ups may be needed I think this helps most cases right now.

Have definitely hit the situation before where I wanted to uninstall some ckeditor module and a disabled format had it as a dependency. If so I'll happily open it.

I don't think so. That would be fixed by another issue #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed or #3056633: It is not possible to uninstall a module that provides a filter plugin - Drupal\Component\Plugin\Exception\PluginNotFoundException: The "{filter}" plugin does not exist

#132 is right, that is a different problem.

I'm triaging RTBC issues.

It is always a pleasure to work on older issues.

I read the Issue Summary and scrolled down seeing that there was a Usability review and reviews by core committers. I then went to read the MR. I left suggestions for comments as well as an item for adding return type hints. I spent most of my time, so far, on reading the test. The fact that there are two filters, 'Enabled filter' and 'Disabled filter' and that the 'Enabled filter' is disabled is just confusing. I think the names need to change so that are not indicating the state of the filter, which is changing in the test. Adding detail to the comments would help as well. That will go a long way to help others working on the test in the future. I am also not sure why two test filters are needed.

Therefore I am setting to NW.

Updating credit.

Not super thrilled with the solution I did to check the edit (configure) links. But using linksHrefExists was giving false positives.

Thanks for all the work on this so far! Let's smash this very old, critical bug.

Opened a handful of MR threads. A few of them serious, mostly very nit-picky suggestions. But since @smustgrave is working on the code, I'll only make MR suggestions, not push commits, so I can be eligible to RTBC this...

Thanks again,
-Derek

Mostly looks great, thanks! Left a few more comments on the MR. Most of the threads could be closed, but a few still need another look before this is RTBC.

p.s. Crediting smustgrave for working on the MR, and quietone and myself for reviews. Haven't thoroughly reviewed the issue for any other credit updates, but wanted to get a little closer.

Addressed additional feedback.

Thanks for removing the new admin user and the extra drupalLogin(). I think that’s best.

I have no other concerns. All the MR threads have been addressed. Title and summary are accurate. Pipelines are green. All is well. RTBC!

Thanks again,
-Derek

AFAICT the cache invalidation logic in ::enable() is not correct nor necessary? 😅

I could totally be missing something though. 😇 But in that case, docs are needed to explain why this additional cache invalidation logic is needed. 🙏

AFAICT the cache invalidation logic in ::enable() is not correct nor necessary? 😅

I could totally be missing something though. 😇 But in that case, docs are needed to explain why this additional cache invalidation logic is needed. 🙏

Attached is a patch with the test that fails if I don't do that cache clear in enable(). I couldn't find a reliable tag to invalidate. When a text format was disabled, the entity would no longer have the corresponding config:filter.format.the_text_format tag. The scorched earth cache bin blowup was how I could get it to work, but I'd prefer a more targeted solution as well. Clearly there is data in the DB that is aware of formerly-used text formats, and perhaps that is the way to do it without annihilating entire bins?

I queued the patch in #143 for testing, and lo, it fails as expected:

There was 1 error:

1) Drupal\Tests\filter\Functional\FilterAdminTest::testEnableAndDisableOfFilter
Behat\Mink\Exception\ResponseTextException: The text "I belong to a filter that might be shut off!" was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:811
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/modules/filter/tests/src/Functional/FilterAdminTest.php:576

That's after enabling the filter, and no other cache shenanigans. The body field doesn't re-appear, since we didn't invalidate the caches.

There very well might be a cleaner, more targetted way to do this, but we need to do something. Back to Wim for a 2nd opinion.

Thanks!
-Derek

@Wim Leers wonder if you've had a change to take another look?

Going to go ahead and mark it to keep the issue from stalling. Re-tested and still seems to behave correctly.

@Wim un-assigning but please feel free to review or change status :)

There is a suggested change in the MR that has not been addressed. There is a request for documentation in #142 and a question in #144. Also, there are files here that need to be hidden. Since there are so many I have done a few. Setting to NW.

I have not reviewed the MR or tested the change.

Going to go ahead and mark it to keep the issue from stalling.

There is some misunderstanding here. Issues stall for many reasons (difficulty of problem, family, work etc) but the fix is not to set it to RTBC. In fact, that can have the opposite effect. Issues that are RTBC are often not looked at by non-committers, so it sits. Then when a committer does look at it they see the unfinished work and make a comment. And worse, this takes committer time away from issues that are truly ready for commit. It would be better to make sure that the Issue Summary is up to date, that the remaining tasks are clear so everyone knows what needs to be done and keep the issue at Needs Work or Needs Review as needed.

#143: that should not be possible 😬

So I stepped through it with a debugger. And sure enough, the cache tags for

entity_view:node:1:full:[languages:language_interface]=en:[theme]=stark:[timezone]=Australia/Sydney:[url.site]=http://core.test:[user.permissions]=391b575d7fe702dd6349da2ae38be4f9b2d581e0906be36e501d3d53a5ed6145

were:
node:1 node_view rendered user:2 user_view

… but \Drupal\filter\Element\ProcessedText::preRenderText() should have bubbled the cache tags:

    // Set the updated bubbleable rendering metadata and the text format's
    // cache tag.
    $metadata->applyTo($element);
    $element['#cache']['tags'] = Cache::mergeTags($element['#cache']['tags'], $format->getCacheTags());

So is there some special handling for disabled text formats that explains this? YES THERE IS! In that same method:

    // If the requested text format doesn't exist or its disabled, the text
    // cannot be filtered.
    if (!$format || !$format->status()) {
      $message = !$format ? 'Missing text format: %format.' : 'Disabled text format: %format.';
      static::logger('filter')->alert($message, ['%format' => $format_id]);
      $element['#markup'] = '';
      return $element;
    }

👆 This should do the same cache tag merging logic if ($format !== NULL).

Done.

Appears to be 1 more thread open from @Wim Leers around core/modules/filter/src/FilterFormatListBuilder.php

Hey everyone, I've updated the "Remaining Tasks" section of the issue description to provide clearer summaries and direct links to comments for better context. This should help streamline our focus and make it easier for contributors to understand and tackle the tasks at hand.

On another note, there's a mention about an unresolved aspect related to core/modules/filter/src/FilterFormatListBuilder.php attributed to @Wim-Leers. I'm having trouble finding the original discussion. Does anyone have the scoop on this? Would be great to clear that up so we're all on the same page. I found it https://git.drupalcode.org/project/drupal/-/merge_requests/5685#note_268326