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.

CommentFileSizeAuthor
#5 reqs.patch3.26 KBbradweikel

Comments

ezyang’s picture

ezyang commented

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

chirale’s picture

chirale commented

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?

bradweikel’s picture

bradweikel commented

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.

bradweikel’s picture

bradweikel commented
Category: feature » bug

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.

bradweikel’s picture

bradweikel commented
Version: 5.x-1.3 » 6.x-2.0
Status: Active » Needs review
StatusFileSize
new reqs.patch3.26 KB

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.

ezyang’s picture

ezyang commented

Testing out the patch. (God I hate CVS so much.)

ezyang’s picture

ezyang commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.