ryancramerdesign / ProcessWire

Our repository has moved to https://github.com/processwire – please head there for the latest version.
https://processwire.com
Other
727 stars 201 forks source link

TextFormatter uninstall behavior #603

Open adrianbj opened 10 years ago

adrianbj commented 10 years ago

I am not really sure what the best behavior should be, but I just noticed that if you uninstall a TextFormatter module, any fields that have that formatter selected retain the selection, so when you visit a page with that field, the module gets automatically installed again.

Maybe the module should not be uninstallable if it is in use, or maybe the formatter should be removed from all fields that are using it? I think probably the former would be better.

teppokoivula commented 10 years ago

Former option would be in sync with how things like removing templates, fields etc. works. +1 for that.

ryancramerdesign commented 10 years ago

I'm not so comfortable with changing this behavior. Textformatter modules would have to know about all the places they can be used. Their usage isn't limited to text fields. They now show up as options in image fields, and potentially other places as well. This is fundamentally different from Fieldtypes, which have a single purpose and are actively monitored. There isn't any straightforward way for a Textformatter module, or the Modules class, to know where and what the Textformatter might be used for. The places using the Textformatter module would all have to monitor Modules::uninstall, and at that point we're talking about a lot of code, and multiple permanent hooks in memory, for a very rare need. Given that, I think that the current auto-installation behavior is desirable. If you are depending on TextformatterTextileRestricted output and that suddenly stops working because the module is uninstalled, you've got a potential security problem as output is no longer modified the way expected. Now you could go permanently erase the module from the file system and end up with the same problem, though user responsibility has to take over at some point. But I do think it might make sense for us to throw exceptions when the requested Textformatter module can't be loaded or installed, just in case.

teppokoivula commented 10 years ago

Makes sense, but it's still a bit confusing how currently one can uninstall textformatter and it seemingly just keeps getting reinstalled without any indication about what happened. Not sure what the best approach here would be, but current situation doesn't feel consistent.

Of course simply describing this behaviour in field settings (and perhaps some other places where textformatters are natively used?) would help a bit.

adrianbj commented 10 years ago

Ryan - from reading your explanation it sounds like you are suggesting the places using the textformatter would have to monitor uninstall, but what about taking a simpler approach and just scanning for fields that use the textformatter when attempting to uninstall it.

Obviously this doesn't take care of templates files or other modules that might use the textformatter, but at least it alerts the developer for the most common situations where attempting to uninstall a textformatter might cause some confusion when it gets reinstalled. This approach would ensure that template files and other modules that use a textformatter would still reinstall the module when called so I think the security concerns wouldn't be an issue.

Anyway, just an idea, but maybe this is even more inconsistent :)

Something as simple as this?

$usedFields = array();
foreach($fields as $f){
    if(is_array($f->textformatters) && in_array($moduleName, $f->textformatters)){
        $usedFields[] = $f->name;
    }
}

if(count($usedFields)) echo 'This module cannot be uninstalled because it is currently being used by these fields: ' . implode(', ', $usedFields);