There are currently no tests for the actions that system.module provides. Add them.
Related issues: #714190: system_send_email_action uses $account without initializing it: It would have shown up on a test long ago. The set of tests I provide here will test that.

However, these are basically untestable until we get #601398: Simpletest does not allow assigning actions to triggers. I'll provide a beginning patch here, but since it will fail, I will wait to flesh out the rest until that goes in.

CommentFileSizeAuthor
#52 721086-system-actions-tests-d7.patch27.61 KBandypost
#48 drupal.system_actions_721086_48.patch26.71 KBrfay
#44 drupal.system_actions_721086_44.patch26.12 KBaspilicious
#43 drupal.system_actions_721086_43.patch25.85 KBaspilicious
#42 drupal.system_actions_721086_42.patch26.17 KBrfay
#39 drupal.system_actions_721086_39.patch21.05 KBrfay
#34 drupal.system_actions_721086_34.patch20.77 KBrfay
#25 drupal.system_actions_721086_25.patch22.11 KBrfay
#21 system_actions_721086_21.patch22.16 KBrfay
#19 system_actions_721086_19.patch15.91 KBrfay
#16 drupal.system_actions_721086_16.patch7.15 KBrfay
#14 drupal.system_actions_721086_14.patch8.02 KBrfay
#8 721086-system_actions-d7.patch8.53 KBandypost
#4 drupal.system_actions_721086_04.patch7.79 KBrfay
#1 drupal.system_actions_721086_01.patch6.62 KBrfay
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Active » Needs review
FileSize
drupal.system_actions_721086_01.patch6.62 KB

Here is a first run. This one tests system_message_action and system_send_mail_action.

It won't actually pass the testbot until #601398: Simpletest does not allow assigning actions to triggers lands, as without that it's not possible to assign advanced actions to triggers. It will also have assertions until #714190: system_send_email_action uses $account without initializing it lands.

When those prereqs are met we can add support for the remaining actions: system_block_ip_action and system_goto_action.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_01.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

There is another test in #286668: Trigger "User logging in" doesn't work with Action "Forward to URL" but it depends on #601398: Simpletest does not allow assigning actions to triggers

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
drupal.system_actions_721086_04.patch7.79 KB

Here's the patch rerolled after various things have moved around. After #601398: Simpletest does not allow assigning actions to triggers lands, this should be added in. See #1.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

doc-blocks should be formatted as proposed Doxygen formatting conventions

+   * Assert that the most recently sent e-mail message has the specified pattern
+   * in it.

The first line of the block should contain a brief description of what the function does, limited to 80 characters

+   * @param $message
+   *   Message to display
+   * @return
+   *   TRUE on pass, FALSE on fail.

There should be a blank line between the @param section and @return directive.

+  /**
+   * Test system_m
+   * Configure system message action. Associate it with taxonomy term insert
+   * trigger. Create a taxonomy term. View the message.
+   */

Something wrong with brief description and need a new line after

+   * Configure an advanced action.
+   * @param string $hook
+   *   The name of the hook. For example: 'user_presave', 'user_login'
+   * @param array $action_configure_edit
+   *   The $edit array for the form to be used to configure.
+   *   Example members would be 'actions_label' (always), 'message', etc.
+   * @return integer
+   *   the aid (action id) of the configured action, or -1 if none.

All the above

forestmars’s picture

forestmars CreditAttribution: forestmars commented

subscribing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review

Re-roll with fixes for doc-block

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
721086-system_actions-d7.patch8.53 KB

And patch

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Thanks. I don't think it can pass until #714190: system_send_email_action uses $account without initializing it is committed. Or unless we comment that out for now, or something.

Status: Needs review » Needs work

The last submitted patch, 721086-system_actions-d7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

There's a bug introduced in #601398: Simpletest does not allow assigning actions to triggers
I found it when working on #403446: Increase default weight of trigger module

Most of all tests are used

$this->drupalPost('admin/structure/trigger/node', $edit, t('Assign'));

Without pointing which hook is used, so I plan to file another issue

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review

#8: 721086-system_actions-d7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 721086-system_actions-d7.patch, failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
drupal.system_actions_721086_14.patch8.02 KB

This is andypost's #8 rerolled to please the bot. No changes made. #714190: system_send_email_action uses $account without initializing it has been committed, so maybe we can get on with this one.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_14.patch, failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
drupal.system_actions_721086_16.patch7.15 KB

Whoops, somehow a piece of an old version of #714190: system_send_email_action uses $account without initializing it had snuck into this. Removed in this roll.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_16.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

rfay this caused by http://drupal.org/cvs?commit=346054

This path not wrong admin/structure/taxonomy/1/add should be something like admin/structure/taxonomy/tags/add

There's no IDs - it's machine readable names

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
system_actions_721086_19.patch15.91 KB

This patch expands the scope of this issue quite a bit.

  • Exhaustive new tests for user triggers, user token replacement, and system actions. Near 100% coverage on those.
    • Tests for all the user triggers.
    • Tests for each of the two system actions
    • Tests for all the testable token replacements in those actions
  • Fixes a bug where system_send_email_action could not be configured with a token for the recipient address, even though the instructions offered that. Changes the error message on destination validation to the apparent intent, rather than the apparent D6 behavior.
  • Fixes an exception condition: trigger_user_logout() was calling _trigger_user() with a NULL for the $edit, where an array by reference is called for.
  • trigger_user_presave() could not present usable information in most cases because the interesting information is almost always in the $edit. Merged $edit into $account so that the expected information would be available.
  • trigger_user_delete() was using undefined $method, causing E_NOTIFY exception.
  • _trigger_user was setting $context['account'] instead of the correct $context['user'], causing token replacement to fail in all cases. This one was migrated here from #709798: User-type triggers can't be token-replaced (provide improper context).
  • user_tokens() generated exceptions with the user_presave trigger in many cases, since parts of the user object have not yet been created. Made this friendlier for [user:uid], [user:url], and [user:edit-url], [user:last-login], [user:created].
  • [edit - in #21] Fixed #755034: User-type actions cannot be assigned to any trigger: The user actions were not declared properly in hook_action_info() so could never be assigned to any triggers. This fixes that and adds tests.
  • [edit - in #21] the watchdog() call in user_block_user_action() was using $user without it necessarily being initialized, causing exceptions. Rewrote how that is done.
  • Added new drupalWebTestCase utility functions assertMailString(), assertMailPattern(), verboseEmail().

It is truly astonishing how many errors can be discovered by writing tests. Just how did I get sucked into this anyway?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Title: Create tests for system actions » Create tests for system actions, clean up token replacement, clean up triggers 
@@ -527,7 +531,7 @@ function trigger_user_cancel($edit, $account, $method) {
  * Implements hook_user_delete().
  */
 function trigger_user_delete($account) {
-  _trigger_user('user_delete', $edit, $account, $method);
+  _trigger_user('user_delete', $edit, $account, NULL);
 }

$edit is undefined

$form_html_id = preg_replace('/_/', '-', $form_name);

http://api.drupal.org/api/function/drupal_html_id/7 uses
$id = strtr(drupal_strtolower($id), array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));
which is much faster then preg_*

EDIT: in this test only "_" is used so I recommend strtr($form_name, '_', '-')

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
system_actions_721086_21.patch22.16 KB

This patch addresses @andypost's #20, and in addition

  • Fixed #755034: User-type actions cannot be assigned to any trigger: The user actions were not declared properly in hook_action_info() so could never be assigned to any triggers. This fixes that and adds tests.
  • The watchdog() call in user_block_user_action() was using $user without it necessarily being initialized, causing exceptions. Rewrote how that is done.
  • Adds tests for user actions. It's pretty hard to test the block IP action without having everything break down, so I didn't do that one.

There's probably quite a bit left to go to clean up this stuff, but I'd rather tackle it in a follow-up. I never intended to spend the rest of my life cleaning up code that has apparently? never been executed. I wonder if all this cruft is new in D7 or if it was never executed/tested/debugged in D6... D5... D4.7?

Essentially: Every action needs to be tested, with token replacement, perhaps like the user triggers tests the system actions. The patterns that are in place here make that easier to do, but since you have to keep stopping to fix breakage, it takes a while.

Note: There is a change to drupalLogout() here that is also in #363580: OpenID login fails when in maintenance mode, so whichever one goes in second will need a reroll. Essentially, it changes from trying to do the logout in one hit to doing 'user/logout' and following with 'user'.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@rfay patch is getting bigger, are you really sure that it'a good idea to put all trigger fixes in in one tremendous patch? Much harder to review but I'll try.

Also there are some another issues with actions/triggers
#420124: DX: Remove module actions on uninstall
#241013: Actions only trigger one action per node page load
#403446: Increase default weight of trigger module

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

This change a bit strange because it calls to user_save()
This probably could lead to circular action loop so I think it's definition and maybe implementation should be changed with

'behavior' => array('changes_property'),

Same way as http://api.drupal.org/api/function/node_action_info/7 http://api.drupal.org/api/function/node_unpublish_action/7

@@ -3290,22 +3290,22 @@ function user_action_info() {
  * @ingroup actions
  */
 function user_block_user_action(&$entity, $context = array()) {
+  // First priority: If there is a $entity->uid, block that user.
+  // This is most likely the author if a node or comment.
   if (isset($entity->uid)) {
     $uid = $entity->uid;
   }
   elseif (isset($context['uid'])) {
     $uid = $context['uid'];
   }
+  // If neither of those are valid, then block the current user.
   else {
-    global $user;
-    $uid = $user->uid;
+    $uid = $GLOBALS['user']->uid;
   }
-  db_update('users')
-    ->fields(array('status' => 0))
-    ->condition('uid', $uid)
-    ->execute();
+  $account = user_load($uid);
+  user_save($account, array('status' => FALSE));
   drupal_session_destroy_uid($uid);
-  watchdog('action', 'Blocked user %name.', array('%name' => $user->name));
+  watchdog('action', 'Blocked user %name.', array('%name' => $account->name));
 }
rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Thanks, Andypost. I definitely don't want to add more. This was all an accident: All I was trying to do was write some tests. And every test uncovered a bug.

Let's not add more to this. We might have to do another omnibus one after this, but let's try to finish this one. Thanks for playing along.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
drupal.system_actions_721086_25.patch22.11 KB

re: #23: I just went back to the old database-manipulation technique. I think you were right that the user_save() route was not right. Nice catch, and thanks for your patience with this mongo-patch.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

just a remark about changing logout link #144538: User logout is vulnerable to CSRF

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

#25: drupal.system_actions_721086_25.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

tests for tokens are commited #701818: Several tokens are broken, especially file tokens; test coverage of every core token

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I'm just assuming this should wait for #732542: system_goto_action breaks core APIs. Do you agree, @andypost? It seems to me there was a dependency.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@rfay Exactly, let's wait for commit #732542: system_goto_action breaks core APIs

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I think we should probably give up on #732542: system_goto_action breaks core APIs.

I note that #655252: Custom e-mail action fails to accept token [node:author:mail] or [comment:author:mail] just went in, which probably conflicts with this.

Probably time to just finish this one.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

#25: drupal.system_actions_721086_25.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_25.patch, failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
drupal.system_actions_721086_34.patch20.77 KB

Reroll - See what the bot says. After the test I'll take another look to see if I did the merge right.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_34.patch, failed testing.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review

#34: drupal.system_actions_721086_34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal.system_actions_721086_34.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

md5 replaced with sha1 in #723802: convert to sha-256 and hmac from md5 and sha1

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs work » Needs review
FileSize
drupal.system_actions_721086_39.patch21.05 KB

I think this has it all cleaned up, with the patch rerolled properly, and using drupal_hash_base64().

If the bot thinks this is OK, I'll write an issue summary.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Issue Summary

First and foremost, this adds badly needed tests for system actions and some user actions, and tests token replacement. However, along the way writing tests uncovered all manner of little problems. Since they were small (and perhaps have been here through several Drupal releases) it seemed worth it to just fix them here.

I do apologize for so much ending up in this patch, but the small size of the fixes, infrequent use of the features (obviously), and inability to get things done due to dependencies among the fixes argued for doing it. Even though this touches a number of areas, it doesn't make things worse and probably makes them quite a bit better for the very few Drupal instances that will ever exercise this code.

  • Exhaustive new tests for user triggers, user token replacement, and system actions. Near 100% coverage on those.
    • Tests for all the user triggers.
    • Tests for each of the two system actions
    • Tests for all the testable token replacements in those actions
  • Fixes a bug where system_send_email_action could not be configured with a token for the recipient address, even though the instructions offered that. Changes the error message on destination validation to the apparent intent, rather than the apparent D6 behavior.
  • Fixes an exception condition: trigger_user_logout() was calling _trigger_user() with a NULL for the $edit, where an array by reference is called for.
  • [edit]trigger_user_presave() could not present usable information in most cases because the interesting information is almost always in the $edit. Merged $edit into $account so that the expected information would be available.. Removed the trigger for user_presave() as no information was available for its use.
  • trigger_user_delete() was using undefined $method, causing E_NOTIFY exception.
  • _trigger_user was setting $context['account'] instead of the correct $context['user'], causing token replacement to fail in all cases. This one was migrated here from #709798: User-type triggers can't be token-replaced (provide improper context).
  • user_tokens() generated exceptions with the user_presave trigger in many cases, since parts of the user object have not yet been created. Made this friendlier for [user:uid], [user:url], and [user:edit-url], [user:last-login], [user:created].
  • Added new drupalWebTestCase utility functions assertMailString(), assertMailPattern(), verboseEmail().
  • Fixed #755034: User-type actions cannot be assigned to any trigger. The user actions were not declared properly in hook_action_info() so could never be assigned to any triggers. This fixes that and adds tests.
  • The watchdog() call in user_block_user_action() was using $user without it necessarily being initialized, causing exceptions. Rewrote how that is done.
  • [Edit] Improve docblocks per Aspilicious' comment in #41.
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Please reroll this once more and read the much improvement doc standards. ==> http://drupal.org/node/1354

... there should be a blank line between the @param section and @return directive. ...

... The first line of the block should contain a brief description of what the function does, limited to 80 characters, and beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such"). ...

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
drupal.system_actions_721086_42.patch26.17 KB

@aspilicious, it's a good thing you're on the prowl. This patch was sitting waiting during the time your campaign started. I think I fixed all instances of that issue in the two files drupal_web_test_case.php and trigger.test, which I believe is what you were seeing. That expands the scope of this patch slightly, as I've noted in the issue summary, #40.

#40 contains the issue summary for reviewers.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
drupal.system_actions_721086_43.patch25.85 KB

1)

+  /**
+   * Generates a comparison message to match the pre-token-replaced message.
+   * See generateMessageWithTokens().

After the summary we also put a newline. (Even if the sentence behind it is really short)
OR
you can place and @see generateMessageWithTokens() after the @return (with a newline)

*** I chose the second option feel free to correct ***

Found more of these:

+   * Tests several content-oriented trigger issues.
+   * These are in one function to assure they happen in the right order.

and

   /**
+   * Returns some info about each of the content actions.
+   * This is helper function for testActionsContent().

2)

Even removed an unneeded newline ;)

+/**
+ * Tests token substitution in trigger actions.
+ *
+ * This tests nearly every permutation of user triggers with system actions
+ * and checks the token replacement.
+ *
+ */

3)

#40 Still covers the summary

*** thnx for appreciating me being anoying ;). Trigger test file looks awsome now ;) ***

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
drupal.system_actions_721086_44.patch26.12 KB

After reviewing my own patch I found another style issue.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ modules/trigger/trigger.test	2 Jun 2010 18:22:32 -0000
@@ -239,7 +247,302 @@
+    // @todo: Below can't work until http://drupal.org/node/754560 lands.
+    // $this->assertSystemMessageTokenReplacement('user_logout', $test_user);
+    $this->assertSystemEmailTokenReplacement('user_logout', $test_user);

Should we RTBC this?

+++ modules/trigger/trigger.test	2 Jun 2010 18:22:32 -0000
@@ -239,7 +247,302 @@
+    // @todo: The following will not work until http://drupal.org/node/754560
+    // lands.
+    // $this->assertSystemMessageTokenReplacement('user_insert', $new_user);

Same

+++ modules/trigger/trigger.module	2 Jun 2010 18:22:32 -0000
@@ -488,14 +488,17 @@
+ // Here we merge $edit into $account, as the new account information
+ // is almost certainly what is wanted.
+ $account = (object)(array_merge((array)$account, $edit));

I think it's a bad idea to merge $account and form values!
$edit passed to actions_do() as $content['form_values']

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@andypost, thanks for the review.

On the first two: I am torn. Commented-out code is not normal in core, but on the other hand we'll want it there in the future. I'd rather leave it, but not sure the committers will agree. I hate to lose the logic.

On merging account and form values to provide information for presave, I'm not sure that there is an option if we want to provide a presave option. Basically, without doing this, there is no information for a presave trigger. Do you have an alternate proposal? We could certainly get rid of the user presave trigger, as it's (almost) useless without any information. We can also probably rewrite it so that it's different from all the other triggers so doesn't create all the "uninitialized" warnings.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@rfay because hook_user_presave mostly used for adding module's values to user[data] we should not merge! I think it's better to replace $account with $edit.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
FileSize
drupal.system_actions_721086_48.patch26.71 KB

After reading the intent of hook_user_presave() I'm convinced that it should not be an available trigger. It should just be removed. This patch removes the user_presave trigger.

This is in fact an obscure API change, but a necessary one, as there is no reason that it's a viable trigger. hook_user_presave() is usually used for adding data to the user object, which an action has no business doing (and I don't think it can, can it?)

So two changes in this patch from #44:
1. Remove the user_presave trigger
2. Remove the @todo's - They are now waiting for us in #754560-5: Messages are lost if set just prior to logout

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@andypost: Reactions?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Patch looks solid and well tested. So I glad to RTBC but I think removing _user_presave should be discused with @fago to ensure that this does not break RULES2

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Thanks, but no problem - Rules doesn't rely on the triggers at all.

 function user_block_user_action(&$entity, $context = array()) {
+  // First priority: If there is a $entity->uid, block that user.
+  // This is most likely the author if a node or comment.

This looks wrong to me, as the usual case here should be that $entity is a user account object. If not, it gets the author.

@@ -81,7 +81,8 @@ function user_tokens($type, $tokens, array $data = array(), array $options = arr
       switch ($name) {
         // Basic user account information.
         case 'uid':
-          $replacements[$original] = $account->uid;
+          // In the case of user_presave uid is not set yet.

This comment doesn't really fit to user_tokens() - what is user_presave? The hook, or the removed trigger? Anyway I'd suggest writing it more generic, as there is there is no uid for any unsaved user account that's passed to token.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
721086-system-actions-tests-d7.patch27.61 KB

Re-roll with changed comments 

-// This is most likely the author if a node or comment.
+// This is most likely a user object or the author if a node or comment.

Because mostly it's a really user account object === user entity since #707724-13: Call them entities instead of objects

// In the case of hook user_presave uid is not set yet.

This patch removes user_presave trigger but some contrib could introduce it back So we should care about UID emptiness

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Needs review » Reviewed & tested by the community

Thanks, @andypost. I think we've been pretty well over this one.

The issue summary with all the nits addressed is in #40.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Another one is #420124: DX: Remove module actions on uninstall

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

This looks good to me -- extra test and some subtle but important clean-ups/fixes. Committed to CVS HEAD. Thanks all!

Status: Fixed » Closed (fixed)

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