Check library folder before module activation
chirale - May 23, 2008 - 09:58
| Project: | HTML Purifier |
| Version: | 6.x-2.0 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
If you forget to copy library directory from HTML purifier, or change to 777 the permissions to the file contained in, no warning message is presented to user, and the module let you activate filter without checking it, leading to a blank screen (failed php require).
A Status Report error message should be presented after module activation, and the filter use should be blocked while library folder doesn't exist / isn't writable.
#1
This is partially a bugfix (we actually have code for checking the version of HTML Purifier, but it wasn't being called by the install script) and partially a feature request. I can set a watchdog/global regular fairly easily, but I don't know how to disable a status report module with some custom userspace code; apparently the only way to do it is through Drupal core incompatibility, or the like. I'm going to ask around, but would you happen to know how?
Thanks,
Edward
#2
I'm not such a Drupal API expert, but if the check before activation is problematic could an error message be presented in the input filters page before filter activation, blocking input filter activation instead module activation?
#3
Both blocking installation AND adding lines to the status report page are done by implementing hook_requirements in your .install file.
http://api.drupal.org/api/function/hook_requirements
In the hook, you can specify different output for installation vs the status report. So, for instance, you could throw a warning on install but still allow installation to proceed, while throwing an error on the status report page. I think that's the ideal behavior, in this case, provided the installer doesn't break the site if the library is missing.
#4
Anything that results in a PHP failure on install ought to be considered a bug. I'd like to see this get fixed, so I'll try to submit a patch for the 6.x-2.0 version soon.
#5
OK, I've attached a patch that replaces the custom version checking function with htmlpurifier_requirements, which implements hook_requirements. I rolled the patch against the latest 6.x code in CVS, but it should be very simple to backport to 5.x.
Advantages of this patch:
1) Better user experience - instead of a white screen with a fatal error, it gives a less frightening and fully themed drupal error and returns you to the module page.
2) Translatable error strings.
3) This is just a first step, but it sets this module up for more improvements that will make it much more robust. See below.
Moving forward, I'd suggest adding more error checks throughout the module, with the goal of allowing it to be fully installed BEFORE the external library is added. Why? At least three reasons:
1) It is a better user experience.
2) It will catch a lot of other potential library problems, rather than just "is it there" and "is it current".
3) This is a biggie -- as more and more developers use Drush to administer Drupal from the command line, it'll be increasingly important that modules install on the first try, or they may fall out of favor.
Anyway. Let me know if there are any issues with the patch.
#6
Testing out the patch. (God I hate CVS so much.)
#7
Committed.
#8
Automatically closed -- issue fixed for 2 weeks with no activity.