Duplicate key errors from role_expire_write_record

The problem with using drupal_write_record is that it's also the caller's responsibility to know whether the record exists. If you attempt an update by passing the primary key as the last parameter, it will always return SAVED_UPDATED, even if it actually inserts a record.

This looks good. I'll apply it later today. I think you have one more patch coming?

There are a number of ways of fixing this:

1. Use MySQL specific SQL "INSERT INTO ... ON DUPLICATE KEY UPDATE" - clearly not good for compatibility
2. SELECT count(*) FROM ... before doing either the INSERT or UPDATE, this adds an extra query every time we want to write a record, which is also not great. We could also DELETE FROM... and then simply do an INSERT every time.
3. In hook_user('load') we could load all the user's roles and their expiration dates, so that when we are in hook_user('insert') or hook_user('update'), we can compare the current user expiry dates for all roles with the new ones that are incoming, and work out specifically whether we need to INSERT or UPDATE. This method probably wouldn't work with the rules integration in it's current form, and we'd have to use a method similar to #2 in the function role_expire_rules_action_set_role_expire.

I guess it's a tradeoff... do we care more about extra queries that will happen every time a user is loaded, or is it better to have them occur when a user is being updated/saved? I'm leaning towards updated/saved.

The cost should be on save, not on load. Save is a much more infrequent event.

Mark, Mike,

I've committed a change to CVS for this that checks for existence of a row to be updated. It then only updates the row if the expiry_timestamp is different, otherwise it inserts a row.

Opinions?

http://drupalcode.org/viewvc/drupal/contributions/modules/role_expire/ro...

I think this is fixed as of 1.10. Wake it up if it's not.