[UX] Text formats: provide a way to re-enable disabled formats via the admin UI

[ghost]

A 'Disabled' text format is different than every other 'Disabled' thing in the admin Ui. A text format, once disabled, can not be enabled again. Everything else can. This can be a pain-point for admins who think they know what to expect.

Possible solutions that have been considered:

1. Allow admins to re-enable disabled text formats, making the operation reversible.
2. Change the name from 'Disabled' to something more like 'Hidden'
3. Allow admins to completely 'Delete' a text format (possibly dangerous!)

The final approach taken in this issue was to keep disabled formats in the text formats list in /admin/config/content/formats, denote them as disabled, and allow for them to be re-enabled (via a newly-introduced "Enable" operation). See #1575 (comment) for a screenshot of this implementation.

This fixes the actual UX issue here, which is that previously we were telling people that we are going to disable the text format, but what we were actually doing instead was to completely remove it from the UI, leaving the user wondering how to recover from this situation. Advanced users knew how to revert this, by manually editing the config files; the new implementation makes it so that even novice users can accomplish the same thing via the admin UI.

The actual "Remove" functionality, and whether we should implement that as complete removal of the text format config files, or simply hide them from the admin UI and keep the config files in place, was left as a follow-up task in #4013.

---

Original Issue

Most site admins think that something "disabled" can be "enabled" again.
Ok, there is a confirm screen, but not everybody read them, especially if they are written in a foreign language (and drupal-7.41.##.po does not include text of admin/config/content/formats/{format}/disable)...

---

Advocate: @klonos

---

Weekly update?

---

PR backdrop/backdrop#2821 by @BWPanda
PR backdrop/backdrop#2826 by @klonos (based on @BWPanda's original PR):
PR backdrop/backdrop#2845 by @quicksketch
PR backdrop/backdrop#2846 by @jenlampton
PR backdrop/backdrop#3024 by @klonos
PR backdrop/backdrop#3071 by @docwilmot

[quicksketch]

This might be a good idea, but I'll explain why it's "Disable" and not "Remove":

Text formats may contain sensitive information in them, such as executed PHP code or internal information that shouldn't be shown to a site visitor. If you "delete" a text format, it is not removed from configuration files or content in the database that currently uses that text format. The text format is removed from the admin interface, but it actually still exists and will be used on all existing content that had used that format previously.

So "disable" is definitely a more accurate word than "remove", but you're right that users expect that a format should be able to be re-enabled after it has been disabled. I could go either way on this one.

[klonos]

...then it seems to me that instead of retitling, we should perhaps change the way this works and have formats actually be disabled (as in not available to be used with new content unless re-enabled) instead of removed.

[ghost]

Thanks for the explanation, I was confused by the fact that it is not actually disabled until you manually flush caches...
If you keep on the concept of disable, it should stay in the list, with an "enable" option (like views)
If you don't, please remove the config file...

BTW, if the site is on a shared hosting with no ssh access, some admins will think that phpMyAdmin was not that bad to solve this kind of issue...

[klonos]

If you keep on the concept of disable, it should stay in the list, with an "enable" option (like views)

That would be the best way to go about it IMO.

[quicksketch]

Sure, sounds good to me.

[mikemccaffrey]

The text format is removed from the admin interface, but it actually still exists and will be used on all existing content that had used that format previously.

Wait, so disabling a text format doesn't actually keep it from continuing to be used on content? That seems dangerous if you are trying to disable a format that executes php code, and expect that all the php code that has been inserted into your content will no longer be run.

[ghost]

@mikemccaffrey : the content stays in the database, but it will not be rendered as soon as you flush caches.
a cache_clear_all() would be wellcome here...

[olafgrabienski]

I stumbled about this issue and saw that it's open for a long time. As far as I understood, everyone agreed that disabled text formats should stay in the list and get an "enable" option.

Just as a reminder: If we do so (I agree as well), we should also change the following text on the disable confirmation page:

Disabled text formats are completely removed from the administrative interface, and any content stored with that format will not be displayed. This action cannot be undone.

It could become something like:

Any content stored with the disabled text format will not be displayed.

[olafgrabienski]

Not remembering this issue, I just 'disabled' a text format for test purposes. Now I don't see an option to re-enable it, that's not good!

(Changed the label from "feature request" to "bug report".)

[klonos]

Until we actually support disabling (and re-enabling) text formats, I think that the "disable" action and any accompanying help text, page titles etc. should be renamed to "delete" or "remove".

Disabled text formats are completely removed from the administrative interface, and any content stored with that format will not be displayed. This action cannot be undone.

The above also reads data loss to me. Provide absolutely no way to recover content created using these formats is really bad.

[olafgrabienski]

Until we actually support disabling (and re-enabling) text formats, I think that the "disable" action and any accompanying help text, page titles etc. should be renamed to "delete" or "remove".

@klonos Why not allow disabling and re-enabling text formats straight away? I think, everyone agreed to it.

[klonos]

Why not allow disabling and re-enabling text formats straight away?

More work 😅

[klonos]

...I have tried editing the issue summary (original report), but my changes do not get saved (I'm assuming because the original reporter has deleted their GitHub account). Here's the summary and screenshots from #3962, which was closed as a duplicate of this:

Description of the bug / Steps To Reproduce

1. Go to admin/config/content/formats
2. Select the "Disable" action for any text format...

Actual behavior
Despite the "Disabled text format xyz" message, the format gets completely removed, and there is no way to get it back:

Even the confirmation message is confusing:

Disabled text formats are completely removed from the administrative interface, and any content stored with that format will not be displayed. This action cannot be undone.

Expected behavior
Either call the action "Delete" or "Remove", or make it so that text formats can be disabled and re-enabled.

[olafgrabienski]

Why not allow disabling and re-enabling text formats straight away?

More work

@klonos Can you estimate how much work it would be? Unfortunately, I'm not able to provide any code, but I'll be happy to test.

(Tagging with "help wanted".)

[klonos]

Can you estimate how much work it would be?

I am notorious for not being able to estimate dev work 😓

Here's what I am thinking:

1. This issue here should remain a bug report, to fix the UX/text bit of the current functionality, so that it either says "remove" or "delete" instead of the current "disable", which is misleading.

2. Follow-up issue, which will be a feature request, to allow for a status/state value to be saved for each text format config. I guess that would be a new disabled property, and have values of 1 or 0. We then need to update the text format listing page to accomodate for that, showing if each format is enabled or disabled, and also allow to enable/disable them.

I think we can do point 1 now-ish (perhaps even for 1.14); but don't think that we have enough time for point 2 (which will need to be included in a minor release - being a new feature and all), so that'll need to wait till 1.15.

We also need someone to validate all this, and to provide technical direction.

[olafgrabienski]

1. This issue here should remain a bug report, to fix the UX/text bit of the current functionality, so that it either says "remove" or "delete" instead of the current "disable" ...
2. Follow-up issue, which will be a feature request ...

Hm, as @quicksketch says in #1575 (comment), "remove" and "delete" are not appropriate. So the bugfix would be to allow re-enabling.

[klonos]

...that was from back in Jan 2016 😅 ...I'll have to review the code to see what actually happens, and where we can improve, but it does sound that the bug is that we do not provide a way ro reenable disabled text formats.

Thanks for pointing out that comment @olafgrabienski 👍

[jenlampton]

@klonos should we split the bug-fix text-change into a separate issue, ad keep this one, along with all it's discussion, for 1.15?

[klonos]

OK, I have reworked this in this new PR: backdrop/backdrop#3024

I have done the following:

• Moved all the code/logic from filter_formats() into a new filter_get_all_text_formats() function. No new parameters added - just the pre-existing optional $account parameter (which helps get formats allowed for a specific user account).
• Added a new filter_get_enabled_text_formats() function (along with a filter_text_format_is_enabled() function).
• Added a new filter_get_disabled_text_formats() function (along with a filter_text_format_is_disabled() function).
• Made filter_formats() a wrapper function for the newly-introduced filter_get_enabled_text_formats() function, so that we retain the functionality of filter_formats() returning only enabled formats.
• Kept wording and certain other improvements from @jenlampton's PR (like using weight to reorder the links in the dropbuttons, and using single quotes + concatenation where links included variables in them, etc.).
• Used a single table to hold both enabled and disabled text formats. This resulted in cleaner/less code, and more consistent styling/alignment of the table cells in general.
• I have tweaked tabledrag.js, so to be adding a tabledrag-handles-on class to help with styling rows that are disabled (and while at it, I've fixed an issue where clicking the show/hide weights toggle would leave behind empty style="" entries in the DOM).

I want feedback in general, but on the following things specifically:

1. Should the new function names be filter_get_*_text_formats() or filter_get_*_formats() (so basically, should they include the word "text" or just plain "formats")?

2. Should we now deprecate filter_formats() in favor of filter_get_enabled_text_formats()?

3. Should we allow disabled formats to be dragged too? We'd have to replicate the UI used in the "Manage displays" pages, where field rows can be dragged below the "Hidden" section to be hidden, and above it to be shown - but that's kinda complicated (uses a '#type' => 'field_ui_table' custom field type), and I may need some help with the JS to accomplish that.

4. The docblock of the filter_format_exists() function mentions the following:

    * @return
    *   TRUE if the text format exists, FALSE otherwise. Note that for disabled
    *   formats filter_format_exists() will return TRUE while filter_format_load()
    *   will return FALSE.

   Wondering if I should refactor to retain this, or if both filter_format_exists() and filter_format_load() should return TRUE now (since we allow disabled formats to be reenabled). Asking, because with the changes in my PR, people can use either filter_get_disabled_text_formats() or filter_text_format_is_disabled if they want to figure out if a text format is disabled.

[docwilmot]

I really dont think all of this is necessary TBH. As I mentioned at #1575 (comment) we only ever need to load all formats in one place; if we just did that it would be a much simpler PR.

In either case we should add another parameter to filter_format_load(format_id, $all = FALSE) with a load arguments in filter_menu() so we maintain the previous functionality.

[jenlampton]

I agree, this PR has a lot of change. Let's stick with the previously agreed upon approach @docwilmot mentioned.

[jenlampton]

We discussed this issue in last week's meeting and agreed that the changes needed are

• the additional parameter in filter_format_load(format_id , $enabled = TRUE) as @docwilmot mentioned (though we recommended $enabled = TRUE as opposed to $all = FALSE)
• as well as the additional parameter added to filter_formats($account = NULL, $enabled = TRUE) as is done already in the most recent PR.

However, since there have been no updates to THE PR since that discussion we're bumping this to 1.16.

[alanmels]

The issue was first created back in 2016 and it has not been addressed in 2020. I really do hope Backdrop won't follow Drupal in some of its worst practices of hanging important issues without resolution for many years.

[docwilmot]

The difficulty we have, and my greatest frustration with working on Backdrop, is that we have no way of deciding if this is an "important issue" nor any way to mark it as such if were did decide it was important. Tons of comparatively trivial stuff would have been committed since this issue was just posted, because of that.

I've tried to raise this concern about our lack of direction multiple times, the latest here: backdrop-ops/backdropcms.org#630

Part of my problem of course is that I dont attend the Thursday meetings, so I dont know if these concerns are addressed, but we havent so far had any good ideas come forward on how to prioritize things.

[docwilmot]

I've added the load arguments in a new PR based of @jenlampton's. This returns previous functionality to filter_format_load(), allowing by default only enabled filters to load. This was important especially where the load function is called in check_markup() to prevent content with disabled formats from being accessed.

[jenlampton]

I've tested the PR and it works great! Thanks @docwilmot for getting that change in.

[docwilmot]

RTBC?

[jenlampton]

still looking over code, but.... 🤞 edit: the code looks great, RTBC!

[quicksketch]

Merged backdrop/backdrop#3071 into 1.x for 1.16.0. Thanks for carrying this one home @docwilmot and @jenlampton! Thanks @klonos for advocating for this issue and being there for all those meetings. Thanks @BWPanda for the several iterations you put together. Thanks @olafgrabienski for your persistent testing and suggestions!