Disabled text formats can't be seen in the GUI

Usability review

We've discussed this issue at 📌 Drupal Usability Meeting 2023-05-19 Fixed . 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 🐛 Disabled text formats can't be seen in the GUI Fixed . 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.

omkar.podey → made their first commit to this issue’s fork.

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

@omkar.podey created followup 📌 Improve representation of the list of enabled and disabled text formats Active 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. I'm not surprised since it is displaying the field contents as saved in the DB - not passing it through any text format code.

👇 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 glad the test was there, and this has now been addressed.

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 🐛 Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed Needs work or 💬 Drupal 10 updating issue - Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" plugin does not exist Needs work

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

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.

Wim Leers → changed the visibility of the branch 11.x to hidden.

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 tested this on a fresh install and found that it works correctly. I have addded before and after screenshots.

I changed the remaining task to prioritize code review and moved the item about exploring caching to be a follow up, if that is needed. That is because this is a critical issue and having it fixed is a priority.

Have also verified that the MR is still working as expected, won't reupload screenshots since @quietone has already.

Code wise believe it looks fine, know it went through some reviews a few months ago to address some issues.

I'll happily create the follow up the ticket but want to make sure it's needed. Maybe a committer would know?

On the other older issue @David_Rothstein wrote:

I'm not a big fan of that solution because it means there's no way to remove an unused text format from the admin UI. (If it were coupled with a way to delete text formats also, it would be a good solution - but that's a whole other can of worms.)

Unfortunately, though, it is hard to think of too many other ways to fix this. When this issue was originally filed it was about human-readable names only (which would have been easy to address) but shortly thereafter formats got machine-readable names too, which makes this much more difficult because unlike human-readable names you absolutely can't let machine names duplicate each other.

Maybe the disabled text formats should be listed on a separate page so they are at least out of the way?

I do think we need to address the UI concerns here and address this issue. I'm not sure whether listing on a separate page or somehow providing a toggle on the list page to include disabled formats is the way to go.

Unfortunately I do not think adding a delete is a good idea at all because of the complexities around ensuring security if a new filter format is created with the same machine name later. @zenimagine as you can see there are good reasons to not allow text formats to be be deleted via the UI.

This issue was made critical in #77 but I'm not actually sure it is. The previous issue #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) → has some issue status discussion between normal and major and at best I think this issue is a major. As #60 points out it is possible to re-enable a text format via the single config export/import screens.

Just adding a link to the comment quoted in #157. It is at #949220-49: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) → .

I have created a custom module patch disabled_text_formats_18062024.patch → by listing the disabled formats with enable links

Step 1 :

Step 2 :

Step 3:

Step 4:

@vijayavelr, Welcome to Drupal! Thanks for the interest in this issue. Since your solution is a custom module and will never get committed to core I suggest you create a contributed module → for that work. That is, if you wish to.

I was looking at other other configuration forms for examples of having a separate page for disable 'things'. There is none that I saw. The closest was search pages which simply add a column for 'Status'. That is easy to implement but does not fulfill the suggestion of a second page. For me, I think before coding and changes this should get direction from usability. I also am inclined to think that this should go in as an incremental step towards a better solution. I will ping in #usability.

I also checked the priority and agree with alexpott that this is not Critical. There is not data loss and there is a work around. I have added the work around to the issue summary, so it is can be found.

@vijayavelr, Welcome to Drupal! Thanks for the interest in this issue. Since your solution is a custom module and will never get committed to core I suggest you create a contributed module → for that work. That is, if you wish to.

I also checked the priority and agree with alexpott that this is not Critical. There is not data loss and there is a work around. I have added the work around to the issue summary, so it is can be found.

Regarding #157 and the idea to have a second page to list disabled formats I think this was answered in point 2 of the href=" https://www.drupal.org/project/drupal/issues/2502637#comment-15058495 🐛 Disabled text formats can't be seen in the GUI Fixed ">Usability report in comment #116. Specifically they state "The group agreed moving the discussion how the enabled/disabled state is communicated to a follow-up issue with resolution TBD."

After looking at other configuration pages I think the simplest solution is to add a status column. That will help make the state of the format easier to see and uses an existing pattern for configuration.

steveoriol → changed the visibility of the branch 11.x to active.

The patch is applied and the deactivated format appears in the list, but there's no button to reactivate deactivated formats.
I tried with versions D10.2.7 and D10.3.0 but no "enable" button is displayed.

@steveoriol You may delete a text format completely with drush command. Make sure it is not being anywhere in the site:

drush cdel filter.format.[format_name]

Tested MR 5685

Verified I was getting the same results as @quietone

On a standard profile install
I was able to toggle the default Full HTML text format to disabled/enabled

@steveoriol would see #157 and #161 to see why it could be an issue to delete a text format.

Committed to 11.1.x and backported to 10.4.x as a usability bug fix. Not eligible for 11.0.x or 10.3.x as it changes the user interface.

Committed and pushed afdee270cb to 11.x and c0a5216300 to 10.4.x. Thanks!

The suggested followup already exists, will un-postpone it after submitting this comment.

Yet another little paper cut bites the dust, thanks everyone who made this happen!

longwave → closed merge request !5685

This was a long time coming! I remember starting work on this issue 8 years ago :) Glad to see it finally get over the line!

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

FYI this results in the disabled formats appearing in the allowed list in field config, created 🐛 Disabled text formats appear in the list of allowed formats in field config Active

Nevermind, that isn't actually related, it's only the Webform one doing it. I assumed it was new but it's always (?) been that way.