Disabling a module which provides a text format leaves the text format behind

Hm. This is kind of a tricky problem to solve...

a) Disabling a module should be non-destructive. But the same time, it definitely is logical that the stuff added by a module go away when it's disabled.
b) How do we know what to switch it back to? We don't store anywhere "What the default input format used to be" unless I'm mistaken.... and someone can delete "Filtered HTML" format altogether.
c) Once we disable the module and re-enable it, how do we know what the filter ID was so that nodes that used to be using the PHP input format still link to the same thing? I guess we should variable_set() it on install or something.. hm...

Ok... So it looks like when the module's disabled, the PHP code input format is just treated as "filtered HTML". Same if I delete the PHP code format from filter_formats altogether.

It seems like the fix would be something like the following, which isn't working. Maybe someone else can take it from here.

it is very tempting to include a filter_format_delete() in the patch, as this take form submit is not something I would like to see committed.

filter_format_delete() was now added. It was basically stolen from filter_admin_delete_submit(). Code is much cleaner now!

Yes, this looks much better. But seems you identify the format by name, which is user modifyable, so it does not seem like good to catch for deletion.

But that's the hardcoded value:

db_query("INSERT INTO {filter_formats} (name, roles, cache) VALUES ('PHP code', '', 0)");

You could argue that that should be solved then, but how could you possibly identify it if it was renamed to "yarrrrr, I'm a pirate"? Or am I missing something?

Unfortunately it is possible to rename it to whatever on the admin interface, and people could easily do it. Seems like the 'PHP Code' format only has the PHP filter, so we can also check whether we have any format, which only has the PHP filter. If the format was customized with more filters, we can just remove the PHP filter and leave the format there. If the format title was the only thing modified and there is still just the PHP filter present, we can remove the format.

So check for the format which only has the PHP filter, and remove that. This avoids the title change issue.

Hah. Seems there's a variable that keeps track of the format (the id) of the "PHP Code" filter. Problem solved. Sorry for missing that.

Note that while the beginning of the issue looks a bit asinine , php filter should never ever be default but regardless of that, enabling/disabling is a real issue here.

can no longer apply patch

andre

The patch #9 applies cleanly and works as advertised, except that once you disable the PHP filter module, there is no default filter selected, and you can't create new nodes (unless you select one of the other filters as default).

CNR

I was alarmed by this but probably a false alarm. The "PHP code" input format is just a name, and you can enable/disable whatever available filters in it. The PHP evaluator filter does not really remain in there, and there is no general practice requiring from the last remaining filter to delete its containing input format.

Hardly critical.

Just to add that if there is anything critical about this, it is that you can enable the PHP evaluator filter in the "Filtered HTML" input format if you feel like it. There are a few things against which nothing can protect you...

cog.rusty makes sense. If it's just the case that your php won't run with the module turned off (which would be the point of having the module in the first place right?), then I think this is just a documentation issue. Setting to needs work.

The issue is not that the PHP filter is run or not when the module is disabled, but that the PHP filter is available in the filter list, right?

So the only issue with the latest patch from Wim Leers as far as I see now is that although the PHP filter module saves an input format, it might have evolved into a different format with a different name and purpose but we still remove it when we remove the module. So nothing guarantees the PHP filter is in that input format. cog.rusty: there might be no practice of removing an input format when the last filter was uninstalled and the input format remains "empty" of filters, but it does not mean we should not deal with this situation.

As I commented before:

If the format was customized with more filters, we can just remove the PHP filter and leave the format there. If the format title was the only thing modified and there is still just the PHP filter present, we can remove the format.

So check for the format which only has the PHP filter, and remove that. This avoids the title change issue.

This is still how I see this issue.

The issue is not that the PHP filter is run or not when the module is disabled, but that the PHP filter is available in the filter list, right?

This is not what happened when I tried to reproduce it with the 6.x dev version at the time I saw this issue. The way I tried to reproduce it was:
- With the PHP Filted module enabled, I made the PHP Input format the default. Then I ran a simple script in a story and it worked.
- I disabled the PHP Filter module. The PHP input format was still the default but the PHP evaluator filter had disappeared from its filters list. I ran a script in a story again and it didn't run.

This seemed acceptable to me. Of course it could be improved in one way or another.

I just tested this on latest checkout and cog.rusty is correct.

Enabled PHP filter module.
Enabled the PHP evaluator on "filtered html".
Set PHP input format.
Disabled the module, and it told me that any nodes with php in would display in plain text in drupal set message.
Went back to my input formats, and the PHP evaluator does not appear in the list on any input formats.

As far as I'm concerned this is completely fine behaviour, and what I'd expect. It's only going to be remotely confusing for people who set PHP to their default input format, and those people are going to be confused anyway ;)

The only thing I could possibly suggest is upgrading the message when the module is disabled to a warning so it's a bit more obvious what's happening. But I don't think this is particularly necessary though.

In which case, this isn't critical, and the title of the issue is incorrect.

Sorry that should be Set PHP input format to default.

Also setting back to active since there's no reason to delete the input format if myself and cog.rusty are right.

I tried this out on a fresh installation of drupal-6.0-beta4. I enabled the PHP code module, and then disabled it, and the input format 'PHP code' is still there in the lists of Input formats. I know the PHP filter isn't actually active, but it may be confusing to still have an "empty" input format left behind..

Why not add an uninstall function for the 'PHP code' module, that removes the filter like goba described in #18?

ximo: because there might be posts that use the filter, it could have html, bbcode and other filters attached to it. Other input formats might have been created with the php filter applied but there's no way to remove them etc. etc.

Ok, never mind then :)

Close. :)

- At least one of the lines is indented with an extra space.
- Need space after , when listing parameters to functions
- Need space between // and beginning of comments.
- php is PHP, html is HTML
- "fromat" should be "format"

Patch in #32 applied to D6-head and tested. Works as intended with no known side effects.

I changed it a bit so that it prompts the user to review their published content once the php filter has been disabled, explains why they need to perform a review, and gives them a direct link to a page of their published content.

- Patch has code style problems. In "if" bracket formating and spacing.
- Patch changes strings. This is a no-no for Drupal 6 unless absolutely necessary.
- Patch does not solve one problem that make this misleading (including the code without and with patch). The nodes, blocks or whatever with PHP input formats are set to the default input format, which might expose the code in plain text or escaped, or not. This depends on the input format.
- Especially the patch uses filter_default_format as if it is the Filtered HTML format. While even a format with this name could have any filters in it, the default format is absolutely not guaranteed to be Filtered HTML.

Moving to Drupal 7. Code needs work.

(Haven't tested, just commenting on the future).

This fixes the immediate issue, but I believe that for 7.x we should think about a solution for the root problem, which is this: While input filters (like most blocks, menu items, etc.) are module-generated with a hook (and therefore deletable), they are applied to input formats which are user-generated (and therefore data content).

The result is that we are meddling with user data, completely bypassing the module/hook tradition. The current alternative - leaving formats alone - is unintuitive, because a user can reasonably expect that a module starts to work when it is enabled, without requiring manual creation of a special format to apply the filter to.

Thus, for 7.x, I would suggest that filter.module applies the same logic to input formats as block.module and menu.module do to their stuff: Allow modules to provide input formats using a hook, and implement this hook in filter.module itself for custom formats. Module-provided input formats could still be customized, but also reset to their "factory values".

Cross-post.

Also, as this is now 7.x, we should really try for a long-term and general solution that applies to contrib filters too. I have made one (non-public) contrib filter module and I know how annoying it is to have to "auto-create" formats.

Duplicated at http://drupal.org/node/242129.

Also reverting the title since you do currently get left with an empty "PHP code" input format on disabling the module (that was in 6.1 but the php.module's code is essentialy unchanged since then AFAICS).

Also, as noted in the other issue, if you delete the input format and then re-enable the module you have to re-create the input format by hand.

IMO this might need a "fix" for D6 as well as a "long-term and general solution that applies to contrib filters too" for D7+ (Arancaytar@#37).

Re-roll

Possible security issue, so mark as critical.

<2ct>
We have the good custom of asking the user:

• Are you sure you want to delete these items?
• a list of items
• This action cannot be undone.

I would expect the same here.

Maybe give the user the alternatives of "Delete these items" or "Assign new default filter"?

Besides usability, could anyone review the security part of it too so this issue can move forward?

It's not working for me.

$ patch -p0 <php-on-uninstall-remove-filter-v7.patch

(Stripping trailing CRs from patch.)
patching file modules/php/php.install
Hunk #1 FAILED at 17.
Hunk #2 succeeded at 41 (offset 12 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/php/php.install.rej

pp

About security:
Like chx stated already: I can not imagine to set the PHP filter as my default filter! Maybe we should make that impossible?

As it is now, all content with PHP filter is reset to "Filtered HTML" (assuming format ID 1 has not changed). At rendering the content is filtered, so I think there is no security issue.

I would not like to show my PHP code on the site after disabling PHP filter, so apart from assigning the new default filter, I would unpublish the content. Let the user go over the content again and decide what they want to do with it.

I would have a preference to delete the content. Apparently the content is not needed anymore?

I reroll the patch.

Thanks Frans for your review!

I think the uninstall should check if any node is using the php filter, if so, maybe unpublish it as you suggested, set the filter of that node to 1 and display an aggregated error message about which nodes needs to be reviewed because they contained (or not) content (php code or not..) ??

franskuipers: I would have a preference to delete the content. Apparently the content is not needed anymore?

I think we do two thing:

a. hide this content (see the patch)
b. do nothing(not change filter format.), the content is not shown.

pp

I think the hide content isn't good way because input format is used other place. (eg. block)

pp

Shouldn't the new "php_code_filter_id" variable be called "php_format_id", since it is an input format and not a filter fid?

Also, some of the comments are a bit cryptic.

About deleting content during uninstall, my opinion is that nothing irreparable should happen, not even changing the format id of the nodes. Just display them with the default format or with a failsafe format or replace them with a message if their own format doesn't exist.

Of course if you reinstall the php module you will get new different format and filter ids and the old ones won't work, but that is always repairable if you haven't changed all format ids to the default.

Generally, I think that input formats should stay out of managing nodes and they should only care about how to display them.

This isn't critical unless we also stop all other potential security issues from filter misconfiguration, which we don't.

Changing the title, because this bug is in no way specific to the PHP format. Any solution to it should be in filter.module itself.

Also, I don't think deleting the module-provided format is ever a good idea, because we have no idea how they are using it on their site. Although it started off as a PHP-specific format (in the above examples), they may have edited it over time and repurposed it for something completely different. In that case, they certainly won't appreciate it being deleted just because the PHP module has been uninstalled.

I would suggest a solution where, when a module that provides a text filter is disabled, filter.module checks to see if any that module's filters are in use in any text format on the site. If so, it provides some kind of warning to the site admin that these formats are now "messed up", and they might want to consider editing or deleting them, etc.

Somewhat related issue: #562694: Text formats should throw an error and refuse to process if a plugin goes missing

subscribing

.

This one won't fix. I'm sorry.

#562694: Text formats should throw an error and refuse to process if a plugin goes missing is not only related, but disapproves and invalidates the idea of what is being considered a bug in this issue.

I'd similar problem described here:
#811072: warning: Invalid argument supplied for foreach() in filter.admin.inc on line 387
I thought if I uninstall and install again filter module, everything back to normal, but it simply can't be installed again, because there is no any uninstallation process.
So how you can really re-install filter module?

Here is "simply" instruction how to re-install Filter module:
1. Disable Filter module.
2. Go to: admin/settings/filters and remove 'PHP Code' filter
3. Confirm removal.
4. Run this query in your database client:
UPDATE system SET schema_version = -1 WHERE name LIKE 'php'
5. Go to: admin/build/modules and enable 'PHP Code' module
6. If you are had use PHP Code in some nodes, probably you have to check for broken dependencies (additional queries).
WooHoo !!!!

Here is "simply" instruction how to uninstall Filter module:
1. Disable Filter module.
2. Go to: admin/settings/filters and remove 'PHP Code' filter
3. Confirm removal.
4. Run this query in your database client:
DELETE FROM filters WHERE module = 'php'
5. Run this query in your database client:
DELETE FROM filter_formats WHERE name = 'PHP code'
6. Run this query in your database client:
DELETE FROM system WHERE name = 'php'
WooHoo !!!!
7. Probably you have to check for broken dependencies in nodes which are using this filter.

Unfortunately if you have no direct access to the database of your website, you have to contact your admin until some patch will be provided here.
Unfortunately I don't have access to my live server, I'll wait for some other solutions.

This issue won't fix due to aforementioned issue, but also because it requires #573852: Rename {filter_format}.name to .title and replace .format with machine-readable .name, which won't happen before D8.