Hours instead of days?

Have you tried putting in a default expiry of .5 for half a day before we code it up?

The module uses mktime() to generate the date in the future. I guess it depends on whether that supports it?

ok then. This is a feature request. It will need to be coded.

If I was to develop this now to only support hours instead of days (which I'm not doing as if we do it then we should support both)

then I would edit this function

function role_expire_process_default_role_duration_for_user($role_id, $uid) {
  // Does a default expiry exist?.
  $role_day_period = role_expire_get_default_duration($role_id);
  if ($role_day_period) {
    $user_role_expiry = role_expire_get_user_role_expiry_time($uid, $role_id);
    // If the expiry is empty then we act!.
    if (!$user_role_expiry) {
      // Add the current date to the role day period and save it back to the role_expire row.
      role_expire_write_record($uid, $role_id, mktime(0, 0, 0, date("m"), date("d") + $role_day_period, date("Y")));
    }
  }
}

and I'd change following line editing the mktime call to add hours instead of days.

 role_expire_write_record($uid, $role_id, mktime(0, 0, 0, date("m"), date("d") + $role_day_period, date("Y")));

You could check out http://php.net/manual/en/function.mktime.php to see what you need if you want to hack away at it.

@sfdrummer Would you be so kind as to share your patch? or at least the lines you changed

Here's an actual patch and I changed the UI text to match.

I can't commit this. We have a user base that expect this to do days. I'm not against adding something to the UI to expose hours as an option but the default must be days as it is now.

If we wrote an update hook to update the database so the default expiry period is stored as hours rather than days, would you accept it then? Or are you looking for a complete rewrite? I don't have a paying client to cover the time for a rewrite now, so will just have to continue using this patch until someone else can provide an acceptable solution.

I don't think I want an update function. If we want this functionality I think the UI needs to allow people to type something like "3 days "or "3 hours", defaulting to days if no type is entered into the text box and have the module process what they entered.

Alternatively we could have a select box that contains the type. I think I prefer the first option. What do you think?

The first option is appealing, but if working on a non-English or multi-lingual site, could be confusing or difficult to use, so atm, I'm leaning towards the select box - or if just two options, maybe a radio button is better? not sure.

Seems that this can be done more simply. Here's an implementation I have working:
1. Slightly modify the description of the field so that users can know they can enter fractional days.
2. Modify the schema of the role_expire_length.duration column to be float instead of int.
3. Add an update to the .install file to modify the schema.
4. Modify role_expire_process_default_role_duration_for_user to accommodate the integer and fractional day parts.

If this is OK, then what remains to be decided is the minutes/seconds parts. That is, when mktime is called, should we just use 0 for the minutes and seconds?

Alternatively, if this seems like a real burden on users, then an hours field should be added to the default-setting page at admin/user/roles.

What do you think? Seems pretty easy to implement fractional days.

I don't think that fractions of days will provide a good user interface. I don't know of anyone who talks about fractions of days rather than hours so I believe we should stick to real world terms in the user interface.

The lines of code that calculate the expiry time based on a default are something like this...

   role_expire_write_record($uid, $role_id, mktime(0, 0, 0, date("m"), date("d") + $role_day_period, date("Y")));

However when somebody edits a user and adds an expiry it gets assigned using something like this

    $expiry_timestamp = strtotime($value);
    role_expire_write_record($account->uid, $rid, $expiry_timestamp);

Why don't we just change the default box to also be calculated by strtotime also and include suitable directions in the user interface? If you agree this would allow people to enter "4 hours", "5 days", "3 months", "1 year" e.t.c.

Also we'd need an update function that converts the role_expire_length.duration from an int to a string to store the strtotime modifier e.g. 1 day. Because altering that table will likely loose all of the data we'd need to be very careful with the update function. Perhaps the update function would first add a new column to keep the duration, migrate the rows by doing something like

  ...
  $string_duration = $row['duration'] . "days";
  ...

When this is complete we could drop the original column. We might have to make the update function batch the conversion. I'd still like the resulting column to be called duration at the end of the update.

Thoughts?

I've thought long about your approach, and can't think of any way to improve it, except that _validate() needs to be solid.

1. You're right. No one uses fractional days. So I like the idea of modifying the instructions and then allowing strtotime to get the duration out.
2. The strtotime function allows all kinds of strings. A strong validation function will be required to ensure that the user hasn't specified something that would result in a non-positive duration.
3. With strtotime, it's conceivable that users would specify a duration string that computes out to a different number of seconds every time it is evaluated. (E.g., "second Wednesday at noon".) Therefore, the computed value for that duration cannot be cached. (I was thinking about proposing adding a column to role_expire_length s.t. the schema would be rid, duration_string, duration_seconds, but this point invalidates that approach.) This means that the actual resulting role_expiration_date would have to be computed using the strtotime every time.

So, unless you see something else, our direction is pretty clear. And of course, this should be done for 7.x as well. Were you going to work on this, or were you looking for someone else to?

I'll give it a go.

Patch for your review:

NOTE: There is one functional behavior change. Default durations now generate expiration dates that are relative to the date and time the role was added to the user. Previously, the time was always midnight.

Changes

role_expire.info

Version string is 6.x-1.10. This is a pretty significant change. Should probably be version 2.0.

role_expire.install

1. Added role_expire_update_6124
2. Converts duration column from int to smalltext
3. Adds ' days' to all duration values

role_expire.module

function role_expire_get_default_duration

Updated doxygen comments

function role_expire_set_default_duration

Modified sql queries to use string duration (instead of int)

function role_expire_form_user_admin_role_alter

Changed title, description, size, maxlength to support strtotime-compatible durations

function role_expire_user_admin_role_validate

Added doxygen comments

Checking that the role_expire form value is a future, relative strtotime string

function role_expire_add_expiration_input

Changed title of the role-expire field to include "date/time", not just "date".

Changed format of the role_expire date to Y-m-d H:i:s.

function role_expire_process_default_role_duration_for_user

Using strtotime to process default duration instead of mktime

How to Test

Setup (before applying this patch)

1. Create some roles ("Add role" on admin/user/roles)
2. Give some of those roles default durations. Examples:
   • role a: 1
   • role b: 10
   • role c (leave blank)
3. Create a user with just role a.

Tests

1. Apply the patch and verify that it applies.
2. Run update.php (http://www.example.com/update.php)
3. Examine the roles created in the Setup above (admin/user/roles/edit/#) and verify "days" added to durations as appropriate. E.g.,
   • role a: '1 days'
   • role b: '10 days'
   • role c: (blank)
4. Attempt to create invalid durations and verify that you are unable:
   • Invalid strtotime string
   • strtotime string that evaluates to a past date (e.g., '-1 week')
   • strtotime string that is not relative (e.g., '2012-01-01')
5. Examine the roles of the user created in the setup. Verify that the role expiration date is the same as it was before the patch.
6. Add role b to user and click Save.
7. Examine role expiration date/time for user's role b.
   • Verify that the role expiration date/time is 10 days from the time you clicked "Save".

If this is OK, I'll create a 7.x patch as well.

Hi,

This is great. A small point.

+    '#description' => t("Value must be !l-compatible relative date. Leave blank for no default duration.",
+                        array('!l' => l('strtotime',
+                                        'http://php.net/manual/en/function.strtotime.php'

We should always assume the users are not programmers so lets leave in an indication that this is a strtotime compatible function but have examples in there of working parameters. Something like...

"You can use things like "1 day, 3 days, 4 weeks, 3 months, 1 year" here or leave it blank for no default duration. If you speak php then this any !l duration text is valid." 

Thoughts?

Good point, Stew. Here's the update. Some important comments:

1. The new instructions now say this:
   

   Enter the time span you want to set as the default duration for this role. Examples: 12 hours, 1 day, 3 days, 4 weeks, 3 months, 1 year. Leave blank for no default duration. (If you speak php, this value may be any strtotime-compatible relative form.)

2. I added check_plain() calls in role_expire_user_admin_role_validate() and role_expire_set_default_duration(). Don't know if that was necessary since strtotime gets mad pretty easily.
3. I haven't tested role_expire_update_6124() against postgresql. (Mysql converts ints to text very nicely. Don't know if postgresql does.)

db_change_field will drop the column and re add it for postgres which will result in a loss of data. You can use "USING to_char" or similar for postgres. I wonder whether we have any postgres users? I guess we should assume we do have some.

Are you sure you'll lose data? When I look at the code for db_change_field (http://api.drupal.org/api/drupal/includes--database.pgsql.inc/function/d...), it looks like it:
1. Renames the original column to {orig}_old.
2. Creates a new column called {orig}.
3. Calls update_sql with a CAST to convert the old type to a new type as it puts data from {orig}_old into {orig}.

I've never used postgresql and don't know if this is sufficient to handle it, but it looks to me like it should.

Go for it then!

What version would you say this should be? 1.10 or 2.0?

I would say 1.10 as we haven't added a lot of extra features but if you feel strongly about 2 then go for it.

Fixed and now addressed in the new release 6.x-1.10 that incorporates the patch at #23.

YAY! Congrats andy on your first role expire release.

@stella We'd love some feedback on whether this fixes your problem.