In my experience, it would be better if the notice about the HTML Purifier version being out of date appeared on the admin/reports/status page instead of on every single page via drupal_set_message. This seems to be the standard convention for this sort of notification from a module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rupertj’s picture

+1 for this. The message has shown up for every admin user on all our client's sites today, resulting in a load of tickets raised with our client services team.

CinemaSaville’s picture

I've got it too. And I upgraded to the latest version, and then couldn't get into my site. Had to delete the module.

ezyang’s picture

CinemaSaville, the problem you're describing seems different (and far more serious) than the rest. Could you describe it further?

toddejohnson’s picture

FileSize
2.91 KB

Here is a patch that should do just that. Please review.

CinemaSaville’s picture

Status: Active » Fixed

Actually it looks like I installed the entire HTMLpurifier folder instead of just the library. I realized my error and applied the patch, and replaced the module. All is well now. Thanks. Great work.

TravisCarden’s picture

Status: Fixed » Needs work

Oops! I'm glad you fixed your problem, @CinemaSaville, but remember that this issue is about the "out of date" message showing one very page, and that matter isn't resolved yet. :)

Thanks for the patch, @toddejohnson. When I applied it I got the following PHP error:

Fatal error: Cannot redeclare htmlpurifier_requirements() (previously declared in /home/user/public_html/sites/all/modules/contrib/htmlpurifier/htmlpurifier.module:42) in /home/user/public_html/sites/all/modules/contrib/htmlpurifier/htmlpurifier.install on line 76

Incidentally, this whole issue would be moot if #708266: Decouple Location of htmlpurifier Library with Libraries API went through.


By the way, if anybody's looking for a fast way to get rid of this error message on a bunch of client sites, you can use the following commands over SSH on Linux servers:

wget http://htmlpurifier.org/releases/htmlpurifier-4.1.0-lite.tar.gz
tar xvfz htmlpurifier-4.1.0-lite.tar.gz
rm -r library
mv htmlpurifier-4.1.0-lite/library ./
rm -r htmlpurifier-4.1.0-lite*

You can put that in a Bash script (I called mine update-htmlpurifier-4.1.0.sh), give your user execute permissions on it (chmod 744 or similar) and execute it using the command ./update-htmlpurifier-4.1.0.sh.

toddejohnson’s picture

FileSize
4.52 KB

New patch will even add info to status report about lib version. The error will be a warning so it isn't enough to trigger check status messages.

@TravisCarden thanks for catching that It must have cached somehow. Fixed now.

toddejohnson’s picture

FileSize
4.5 KB

Today is not my day. New patch to remove hardcoded versions. (or delete line $current="4.1.1";) Rerolled please review.

TravisCarden’s picture

FileSize
5.21 KB

That's better! Thanks, @toddejohnson.

I have three concerns about the requirements message:

  1. I'm afraid that people might just copy the contents of their new library/ directory over the contents of their old one instead of deleting and replacing it as the message is phrased now.
  2. Not everyone's module will be at modules/htmlpurifier/, for example if they use the new modules/contrib//modules/custom//modules/features/ structure, as I do.
  3. The English could be cleaned up and shortened just a little bit

I propose changing the message to read "Your HTML Purifier library is out of date. The most recent version is 4.1.0, which you can download from htmlpurifier.org. To update, replace sites/all/modules/contrib/htmlpurifier/library/ with the library/ directory from the downloaded archive." Attached is a patch... in which I made a couple of other changes to clean up the code and improve consistency with conventions in the rest of the module and on the Status Report page.

As long as we're in here, perhaps we should improve handling of a missing library. If I delete the htmlpurifier/library/ directory I get the following fatal PHP error on the Update Status screen:

Fatal error: require_once() [function.require]: Failed opening required 'sites/all/modules/contrib/htmlpurifier/library/HTMLPurifier.auto.php' (include_path='.:/usr/lib/php:/usr/local/lib/php') in /home/ihcc/public_html/sites/all/modules/contrib/htmlpurifier/htmlpurifier.module on line 131

Obviously that's no good.

BenK’s picture

Subscribing...

TravisCarden’s picture

FileSize
5.63 KB

On lines 81-86 of htmlpurifier.install we have the following code to update our library version number:

    $ours = variable_get('htmlpurifier_version_ours', FALSE);
    if (!$ours || version_compare($current, $ours, '>')) {
      // Update our version number if it can't be found, or there's a mismatch.
      _htmlpurifier_load();
      $ours = variable_get('htmlpurifier_version_ours', FALSE);
    }

If there's no variable for the version number we're using _htmlpurifier_load() to check for the version number and set the variable. But the first thing _htmlpurifier_load() (line 136 of htmlpurifier.module does is this:

if (class_exists('HTMLPurifier')) return;

...which kicks the flow right back without doing anything, with the result that the version number isn't retrieved nor the variable for it set—in short, it fails to do what it was called to do. What's that return there for on line 136? Just a performance measure, I assume. If I comment it out everything works as intended.

Also, it occurs to me that if we give the user a message indicating that their library is out-of-date, we should tell them what version they're using, not only what the latest version is. So I've replaced the "Out-of-date" status with the version number.

Finally, I replaced my use of drupal_get_path() with the $module_path variable. Here's a new patch.

ezyang’s picture

I'm currently working on incorporating this branch into my tree. Please stand by.

The class_exists check is to ensure we don't try to load two versions of HTML Purifier into memory. The initial check is not intended to be called when the versions are old; it's just to be called if we don't know AT ALL what version of HTML Purifier is to be used. I can clarify the conditional there.

ezyang’s picture

Status: Needs work » Fixed

Fixed in CVS. Testers would be appreciated.

Status: Fixed » Closed (fixed)

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