Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled)

Thanks for splitting, David! Definitely makes sense to tackle this separately.

My personal UI experience as an experienced developer hitting this for the first time. This was very perplexing, or a polite way of saying wtf?

I disabled it and it just disappeared. There were probably warnings, but I was only disabling the filter, so I ignored these.

Why? Is there a reason for this? Almost any other disable switch has a corresponding enable switch on it.

There has been a last minute API change allowed for the disable, so wouldn't the fix be as easy as creating a new enable callback and a new hook to alert modules that the change? Just my 2c

I agree.
If I can disable something, I don't expect to be able to create something new using the same name, but I do expect to be able to enable the item using that name again.
If I remove something I don't expect it to block a new item from attempting to replace it using the same name - as I expect the first item to be gone forever.

We can't remove the item completely in this case, so why not make it possible to re-enable it?
If that's out of the question, can we instead display a list of what formats have been disabled, saying there might still be content using them? The message reported in the original post becomes much less of a WTF if the user can quickly confirm it by looking in that list.

Ideally, I'd expect an "invalidated" format to stay in such a "disabled formats"-list - perhaps with the possibility of enabling them again - until no more content is using them, then they'd be completely removed.
Of course, that would require either some sort of reference count, or another hook to ask modules if they still use a format. But then what happens to disabled, but not uninstalled, modules? Sounds like a much too complicated solution this close to D7 RCs.

PS: For clarification, the new hook to alert modules about the change was only intended for caching issues. This really should be done asap, so that both API changes here are introduced at about the same time.

Any tracking of formats would be a bit of a nightmare, and definitely be Drupal 8 or 9 material. Hook callbacks would pass the required info to contributed modules, but such a deep hook would not be used on the majority of cases as contributors simply would not know about it without an in depth knowledge of the core hook system.

This would be the simple fix. Didn't think through all possible consequences, but yeah.

Can somebody confirm that there are reasons not to simply re-enable? Is this a permissions / security issue?

This is a quick hack to provide a starter for the enabled process .

I'm not sure whether it's still possible to fix this by introduction of an '(re-)enable' functionality. Technically, that sounds simple to do. Off the top of my head, I don't see any negative consequences. Doing so would resolve a couple of the remaining issues for D7.

Bumping this to major and tagging for a potential API change.

The notion of disabled formats had to be introduced only a few weeks ago, so sadly, it's just natural that we're facing follow-up problems now.

UX feedback: I just 'disabled' a text format assuming that it could be 're-enabled'. This affected all the comments on my site, and I've had to restore an older version of my site database to revert that change.

So yes, it would be great if you can re-enable ... or changed to 'delete text format' to avoid confusion.

Can't believe I'm doing this, but yes, this is annoying, and it's a task for D8 now. Whether it's backportable depends on the final resolution.

marked #1216318: Users can delete default text formats but they cannot be restored as dupe.

After scratching my head over this one for a while, I used phpMyAdmin to edit the row representing the disabled filter. I changed the value of "status" from 0 to 1, and lo and behold, the disabled filter was back.

That suggests to me that it would be fairly simple to add an "enable" link to the Content authoring>Text formats page to do the same thing.

As a side-issue, it might make sense to add another flag to filter_formats:
function filter_formats($account = NULL, $include_disabled = FALSE) {

That would remove the need for the first lines in Alan's patch, while keeping the API backwards compatible.

Not funny at all, I also swallowed this bait. Now finding out there is no elegant solution to the issue going to fix manually in DB.

Here's a patch to get this rolling again. It's a bit different than Alan D. 's approach. I opted for a separate table because a disabled format shouldn't have configure options or the ability to being ordered. I've attached a screenshot of that screen with this patch. Still needs work on a couple of levels:

- UI: are 2 separate tables a good thing or not.
- If so, should we adding labels above the table or not
- Needs test, obviously. A simple enable test and also a test with a field that had a disabled format that can be seen again. Others that I might miss.

Feedback appreciated, let's get this thing fixed :)

Setting to review though to see if the bot already catches some tests errors or not (I guess it will, but was a bit lazy to run it on my local machine).

This seems like a good candidate for tests.

+++ b/core/modules/filter/filter.admin.incundefined
@@ -60,7 +75,46 @@ function filter_admin_overview_submit($form, &$form_state) {
+        ->addTag('translatable')
+        ->fields('ff')
+        ->condition('status', 0)
+        ->orderBy('weight')
+        ->execute()
+        ->fetchAllAssoc('format');

Two space indent.

+++ b/core/modules/filter/filter.admin.incundefined
@@ -60,7 +75,46 @@ function filter_admin_overview_submit($form, &$form_state) {
+    $output = theme('table', $variables);

Why call theme('table') directly when you're returning a render array? Use '#theme' => 'table'

+++ b/core/modules/filter/filter.admin.incundefined
@@ -336,7 +390,7 @@ function filter_admin_format_form_submit($form, &$form_state) {
+ * Menu callback; confirm disabling of a format.

See http://drupal.org/node/1354#menu-callback

+++ b/core/modules/filter/filter.admin.incundefined
@@ -363,3 +417,31 @@ function filter_admin_disable_submit($form, &$form_state) {
+ * Process filter enable form submission.

See http://drupal.org/node/1354#forms

+++ b/core/modules/filter/filter.moduleundefined
@@ -132,21 +139,21 @@ function filter_menu() {
+ * Access callback for enabling or disabling text formats.

See above about callbacks

+++ b/core/modules/filter/filter.moduleundefined
@@ -132,21 +139,21 @@ function filter_menu() {
+ * Load an enabled text format object from the database.

s/Load/Loads

+++ b/core/modules/filter/filter.moduleundefined
@@ -164,6 +171,28 @@ function filter_format_load($format_id) {
+ * Load a disabled text format object from the database.

s/Load/Loads

Re-roll based on Tim's comments. Tests and a small rework of the ui for disabled filter formats next.

D'oh... That's just wrong. See what happens when I try to do too many things at once in the middle of the night?

Fixing...

I don't actually believe that this is major. There's an easy workaround which is just to call it something else.

Can the format(s) be removed manually from the database if I get desperate? If so what tables need to be searched?

In Drupal 7, you can re-enable a disabled filter as follows:

1. With phpMyAdmin or the equivalent look at table filter_format. Find the row with status=0. Edit that to status=1.

2. Clear the caches (button at Administration>Configuration>Development>Performance)

3. Visit Administration>Configuration>Content authoring>Text formats. You should now see the disabled format.

Thanks that did it. Can the formats be deleted safely from the database, or is that going to cause problems?

I tried the following on my Drupal 7 system:

1. Used phpMyAdmin to delete the row 'Filtered HTML' from my filter_format table (I don't need that one, editing is done with wysiwyg_html)
2. Went to Administration>Configuration>Development>Performance and clicked Clear all caches.
3. Went to Administration>Configuration>Content authoring>Text formats and verified that all the other formats were still there.
4. Went to a node and edited it successfully with each of the remaining formats.

You owe me a beer :-)

-- Walt

Can the formats be deleted safely from the database, or is that going to cause problems?

If they are not being used, no problems. However, if it is being used then you may get issues. Text fields with processing get run through check_markup() that will return an empty string if the format is missing.

function check_markup($text, $format_id = NULL, $langcode = '', $cache = FALSE) {
  if (!isset($format_id)) {
    $format_id = filter_fallback_format();
  }
  // If the requested text format does not exist, the text cannot be filtered.
  if (!$format = filter_format_load($format_id)) {
    watchdog('filter', 'Missing text format: %format.', array('%format' => $format_id), WATCHDOG_ALERT);
    return '';
  }
  ...

CNW for tests.

This tests expects that the creation of new text format with same machine-name and human-readable-name must both be allowed.

Thanks @eltermann! Setting to needs review so testbot can run the test.

@eltermann - Can you please upload a combined patch as well? We usually upload two patches, in this case we should combine the test with the patch in #21 as shown in http://drupal.org/node/1468170.

sure

Thanks! For future reference: Testbot looks at the last tested patch to see if it needs to update the issue status.

So if you attach the test only patch first, followed by the combined patch (still in the same comment), testbot won't set the issue to needs work and post a fail message to the issue (as in #33). Provided the combined patch passes, of course :)

looks like tests were added so removing needs tests tag.

code standards review:

+++ b/core/modules/filter/filter.admin.incundefined
@@ -11,7 +11,21 @@
+ * Form callback; Displays a list of all enabled text formats and allows them to be rearranged.

comments must wrap at 80 chars.

these one line summaries must be under 80 chars.

So more details should be added as a separate paragraph in the doc block.
http://drupal.org/node/1354#functions

(check rest of patch for same)

+++ b/core/modules/filter/filter.admin.incundefined
@@ -363,3 +418,33 @@ function filter_admin_disable_submit($form, &$form_state) {
+}
\ No newline at end of file

newline is needed.
see: http://drupal.org/coding-standards#indenting

+++ b/core/modules/filter/filter.moduleundefined
@@ -303,6 +327,26 @@ function filter_format_disable($format) {
+ * Enable a text format.

one line summary comments should start with third person verb like:
Enables ...
(check rest of patch for same)

+++ b/core/modules/filter/filter.moduleundefined
@@ -303,6 +327,26 @@ function filter_format_disable($format) {
+ * @param $format
+ *   The text format object to be enable.
+ */
+function filter_format_enable($format) {

type needs to be specified in @param
http://drupal.org/node/1354#types

Should we postpone this on #2502637: Disabled text formats can't be seen in the GUI?

I am all for displaying the disabled formats the way we do with Views.

I've been working on a patch for this as I find this problem rather infuriating. It still needs work (and tests) but the basics are there! Happy to backport this to D7 once I've finished with D8.

I prefer #2502637-63: Disabled text formats can't be seen in the GUI.

Thanks @welly for the patch. Here are some points.

1. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -20,7 +20,7 @@
   - *       "disable" = "Drupal\filter\Form\FilterDisableForm"
   + *       "disable" = "Drupal\filter\Form\FilterDisableForm",
   

   Shouldn't we add enable here as well?

2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -193,6 +194,19 @@ public function disable() {
   +
   +    parent::enable();
   +
   +    // Clear the filter cache whenever a text format is enabled.
   +    filter_formats_reset();
   +
   +    return $this;
   

   No need for extra new lines.

Thanks for the review, jibran!

Point 1. I'm not submitting a form to enable the filter format - I'm just using a text link that calls FilterController::enableFilter() to enable the filter. It didn't make sense that a confirmation form needed submitting to enable the format as it's not a destructive call.

Point 2. Haven't touched FilterFormat::disable() :) See: https://api.drupal.org/api/drupal/core%21modules%21filter%21src%21Entity... to compare.

I've noticed that disabled text formats still have the Configure link in the operations drop down and that needs removing when a text format is disabled as it returns a 404. Will fix that up next.

Updating issue title to match the patch.

from #3

If I can disable something, I don't expect to be able to create something new using the same name, but I do expect to be able to enable the item using that name again.

Since this happend already multipe times for many users with default (standart) text fomatting settings disabled temporary and disappearing forever while they where awaiting to be able to reenable them and this can cause a lot of mess with already created content, I really really would ask for setting this long term issue to major before it turns into a Drupal #WTF.

@webchick:

I don't actually believe that this is major. There's an easy workaround which is just to call it something else.

While agree with you to rather de-escalate issue queues often being too much knocking for their level of priority, I maybe have a conclusive argument to bring up here: There are modules providing text formats, like the php module and the script filter module does, and others. Having a one way ticket like it is at the moment regarding creating and "disabling" formats forever in the GUI, it will most likely make it unpossible to install such modules if it collides with disabled formats, will it make inpossible to uninstall and reinstall such modules too. Especially if the module uninstallation requires to disable the format, like the php module does. It also will create conflicts with older maybe forgotten formats with same names turning the installation of such modules into a possible mess I can't oversee yet. Additionally we can see how the few attention of this issue turned it down a lil bit lately and we definitely need to get rid off it. Would you agree with me on this?

And since 8.4 is out we maybe need to set it to 8.5.

Oh sorry, I didn't saw your post @cilefen

I prefer #2502637-63: Disabled text formats can't be seen in the GUI

But since this issue here is older and we actually follow the rule to keep the older and set the newer to duplicate, we should consider to merge both attempts somehow. I agree with you on the "what should be fixed" priority in the newer one. So could we woop this all over here? I set it to "related"...

@David_Rothstein

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

I was intuitively thinking the same like @cilefen on this @ #40 when I was hitting this issue first ime:

I am all for displaying the disabled formats the way we do with Views.

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.

I know that typically the newer issue should be closed as a duplicate but in this case there is much more recent work in #2502637: Disabled text formats can't be seen in the GUI so I'm closing this one instead. I've moved credit from here to that issue also. Hopefully we can progress the other issue now.