Port drupal 7 - patch

Wow. It is great that you tried this. Did you port CVS HEAD or the current release? The current release has some bugs that I wanted to squish before I push on to 7. Just interested.

Is there a reason you didn't move the queries over to DBTNG format? Did you attempt it and it didn't work for some reason?

If not I'd say this needs work.

If we use the PDO libraries we'll get support for other DBs almost out of the box. We also need to add tests. If you want you can wait on this one. I plan to get a release of 6.x out the door with a few bugs fixed. What bugs did you fix in #2 (did they have issue numbers?)

We also need to add SimpleTests to this module to make a d7 release. I'll be more responsive next week. I'm on a big client job this week that is getting finished

Subscribing. Thanks

Subscribing.

+1

I won't commit this patch without it using the DBTNG. I've started some work with the DBTNG and will be releasing that shortly. I guess I could post that patch here for feedback.

Subscribing

subscribing

@stewsnooze any word on when a patch or 7.x development branch will be out?

I'm beginning to build out a site based on D7 and we'll need this module. I'd be happy to test data migration as well as normal use.

@zabelc I have a d7 version essentially working that I need to commit. If I'm being honest I was waiting for d.o to switch to git as CVS hurts my head. When are you looking at starting your project. The git change is next week I think

I am also interested in your latest version for d7. Will it be release anytime soon, I may look at downgrading back to d6 if role expire don't have support for d7...

@hazuremon Role Expire will be out for d7 soon. See comment #13

Git migration happened a couple of days ago - will you be committing soon?

Hi stewsnooze, I hate to bug, but it's been a while, will the D7 branch be created any time soon?

Same here, stewsnooze, if you need any help debugging or writing patches for the Drupal 7 version, including rewriting the DB Queries to use DBTNG please let me know, I'd be more then happy to help out!

I hear your pain. I have a week off work next week. I'll get on to it. Can you guys have a look at beta versions. I could do the quick and easy thing and maintain the existing tables or move the role expiry dates into user entities. Any thoughts on which approach to take?

I was thinking about using entities but I'm not completely sure whether that is just because I want to...

One other benefit of entities is for sites that don't use SQL storage and may have millions and millions of rows that they feel are better suited in a Document Database.

Well I'm always for embracing Drupal 7 when making a version of the module rather then just simply porting over. I feel like in the long run it's the best path, though it might make things a bit more work in the short term. Just for reference, what I want to use this for is in conjunction with Drupal Commerce and Rules so that users can buy membership to my site that expires after 1, 2, or 3 years.

It seems that rules can actually do most of this, but having a quicker and easier to setup method like this module would be nice.

Just poking my head in to see if you need any help porting things over to DB:TNG! Or help with anythings else.

Subscribing

Any update on this?

How about making it into a generic Entity Expire module, that could replace this and Node Expire module as well? :)
http://drupal.org/project/node_expire

I don't think Roles are entities in Drupal. I was thinking about using entities to store the expiry and default lengths of roles rather than converting roles into entities and having a property. However I might have misunderstood the extent you mean. Please explain if there is more detail

Ah good points Stew, I may have gotten myself carried away with entities :)

subscribing

subscribing

+1

Just checking in to see if there is any progress. stewsnooze, in #13 you had mentioned that you were very close to releasing a earlier D7 version. Is this still the case? I know you are really busy, so if you need someone to take over as a maintainer because you time is to tight right now I'd be happy to do that. I really need this module for a client site. Git doesn't look like it's been updated since the Git migration so I'm not sure if the 7.x branch has the latest code or not. If we could get the latest code, even if it's not release ready that way I could start patching it up that would be great.

Thanks for all of your hard work!

Subscribe

@stewsnooze I hate what I'm doing here, but could you please upload the module somewhere so we can grab it and test it? Even if it's not that polished, we could sure use it!!!
I looked at your github but it's not there and I have a 2-week deadline for a project so... I'd really appreciate it!
I don't see a reason why I should re-invent the wheel since you've already done it and from what I've seen you do a damn good job! :)

I've used this module on D6 and LOVED it, so.... thanks for all the hard work you've done!

There is a 7.x.x-dev release now. Give d.o time to publish it. The edit role portion is working and expiring roles on cron should work however editing the user to give them roles is not done yet. I started the 7 version by using the excellent coder upgrade module. I think the 1st real version should be fully ported to DBTNG e.t.c and have old stuff removed. Also I would like the views integration done e.t.c. Jump in and help. I'd apply fixes from your sandbox or github forks of this if you want to also work on the port.

Awesome news, thank you! I think I should have some spare time this week so I'll get this version running and see if I can't contrib some bug fixes etc back.

I posted a patch to fix the user edit form and user view under #1214164 issue. Not sure I should have posted it here.

stewsnooze, I recommend you update the 7.x-dev with prossel's patch (q.v.). It's a big leap forward. I'm going out on a limb here and re-versioning this issue to 7.x-1.x-dev.

Attached is a patch that folds in prossel's work under #1214164 and the issues I posted in response to that. I feel it's a better fit here than on the UI bug since this is work that's required to make role_expire actually work in D7.

I invite you to test the following use cases:
1. Create a user (admin/people/create):
-Page load without errors?
-Role checkboxes show/hide role expiration text boxes when checked/unchecked? (NOTE: role expiration does not apply to anonymous and authenticated user)?
2. Save user without any special roles and verify you get no error messages.
-user/# - no error messages on the user’s view page.
-user/#/edit – no error messages on the user’s edit page.
3. Create a user *with* a special role, but no role expiration date and verify you get no error messages.
-user/# - no error messages on the user’s view page. Verify that you do not see a role expiration date.
-user/#/edit – no error messages on the user’s edit page.
4. Create a user *with* a special role and *with* a role expiration date. Verify no error messages.
-user/# - no error messages on the user’s view page. Verify that you see the role expiration date.
-user/#/edit – no error messages on the user’s edit page.
5. Create a role with a default duration. Then create a user and give them that role. Observe that you get a blank expiry field and don’t see the default duration anywhere. Save and view the user's profile page and verify that the role does expire at a date that corresponds to the specified default duration.
6. Remove the default duration from the role created in #5, above. Then create a user and give them that role. Verify that you are able to specify an expiration date for that role.

Hi sharplesa,

Great work - thanks!

Ran a few tests - everything seems to work, but some notices/warnings surface. In case my lines differ from yours...line 321:
foreach ($del_roles as $role_id) {

Created a new user:
...no errors

Added a role:
expiration date/time set according to parameters
•Notice: Undefined variable: del_roles in role_expire_user_update() (line 321 of role_expire.module).
•Warning: Invalid argument supplied for foreach() in role_expire_user_update() (line 321 of role_expire.module).

Changed expiration time on user edit:
change is accepted
•Notice: Undefined variable: del_roles in role_expire_user_update() (line 321 of role_expire.module).
•Warning: Invalid argument supplied for foreach() in role_expire_user_update() (line 321 of role_expire.module).

Created new user with special role (no entered exp date)
role created
expiration date set
no errors, notices, or warnings

Created new user with special role entered exp date - "200 days")
role created
expiration date returned as default 365 days
...you can override date with yyyy-mm-dd hh-mm-ss

Run cron:
role removed
expiration time deleted
•Notice: Undefined variable: del_roles in role_expire_user_update() (line 321 of role_expire.module).
•Warning: Invalid argument supplied for foreach() in role_expire_user_update() (line 321 of role_expire.module).

Remove role:
role removed
expiration date removed
•Notice: Undefined variable: del_roles in role_expire_user_update() (line 321 of role_expire.module).
•Warning: Invalid argument supplied for foreach() in role_expire_user_update() (line 321 of role_expire.module).

Role created by placing an order (rule to change "pending" order (Drupal Commerce) to "completed" creates role):
role created
expiration date set
•Notice: Undefined variable: del_roles in role_expire_user_update() (line 321 of role_expire.module).
•Warning: Invalid argument supplied for foreach() in role_expire_user_update() (line 321 of role_expire.module).

Interesting. I can't get that behavior. What version of php are you using? I'll go ahead and check isset(del_roles), though, and roll a new patch.

BTW, your line numbers and mine agree perfectly. That's what I'd expect with patch.

New patch that theoretically addresses all items in #37 and #38. Try it and post your feedback here, good or bad. I think we're close to a 7.x-1.0 version!

Excellent work sharplesa!

No errors, notifications, or warnings.

I did a few tests and found in your #6 scenario that you can't set a non-default expiration date for a new user (if existing users have roles). You can edit the date after the initial save.

The role (with expiration date) can be set from an order in Drupal Commerce. ...sweet!

In answer to your question in another issue... users are not able to edit their roles and expiration date.

PLEASE...put it out as a new release...I don't want to work through yet another patch ;-)

Thanks sooo much for taking the lead on this effort.

According to the issue queue guidelines, patch status needs to move to "reviewed and tested by the community" (RTBC) before the patch can be committed. (http://drupal.org/node/156119) So if one or two developers are able to agree with the patch, please chime in. Once the patch is RTBC, I'm sure stewsnooze will gladly commit. Right Stewart?

...I would certainly like to see the patch move forward. ...and have the visibility raised with Drupal Commerce. This seems to be a much cleaner solution than Commerce Subscription.

@sharplesa yes that is right.

After a quick look I've noticed that the js uses document.ready and I think we should be using Drupal.behaviours instead of document.ready. Opinions?

Why have we removed the rid parameter from the help comment when the function still takes it in certain places?

However I've committed it so we can move forward with this. I don't think we are ready for 1.0 but I think we are getting closer. Hopefully today d.o will push a new version of role_expire 7.x.x-dev

So I've added a default expiry to a role and gone over to

/user/1/edit

I tick the role check box but leave the box blank and I get a whole heap of errors on submit.

which are....

Notice: Undefined index: original in role_expire_user_update() (line 314 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Notice: Trying to get property of non-object in role_expire_user_update() (line 314 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Warning: array_keys() [function.array-keys]: The first argument should be an array in role_expire_user_update() (line 314 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Warning: array_diff() [function.array-diff]: Argument #2 is not an array in role_expire_user_update() (line 314 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Notice: Undefined index: original in role_expire_user_update() (line 322 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Notice: Trying to get property of non-object in role_expire_user_update() (line 322 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Warning: array_keys() [function.array-keys]: The first argument should be an array in role_expire_user_update() (line 322 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).
Warning: array_diff() [function.array-diff]: Argument #1 is not an array in role_expire_user_update() (line 322 of /Library/WebServer/Documents/role_expire7/sites/all/modules/role_expire/role_expire.module).

I can't reproduce that. Might it be an issue with trying to override the user 1 role settings? Does the same thing happen with an authenticated (non-admin) user?

What do you have for your error_reporting setting in php? Set it to e_all and check again. I think this error occurs for all users

My error reporting is set to E_ALL, and I cannot duplicate the problem.

The only object whose properties we are requesting is $edit['original']. How can $edit['original'] *ever* be unset? user.module line 425-426 guarantees that we'll have an original value.

My Drupal install (7.4) is very vanilla: core (default modules enabled) and role_expire. Can you provide more information about your configuration that may contribute to these error messages?

1. document.ready vs Drupal.behaviors -- You're right. After reading http://drupal.org/node/756722, I figured out how to do that and updated role_expire.js on my machine to use Drupal.behaviors instead of document.ready. (Patch forthcoming...)

2. rid parameters had been removed from the code comments only for functions that don't have rid parameters. I also had rearranged a few of the params in the comments to match the order of the params in the functions.

In response to Stew's comment (#44), the attached patch updates the javascript to use Drupal.behaviors rather than document.ready. No change to the module to address Stew's #46. Is anyone else seeing what he reported there?

IMPORTANT: This patch works against the role_expire_7.x-1.x-dev dated 2011-07-27.

Here's what I'm now seeing in page footers after updating to D7.7:

•Notice: Trying to get property of non-object in role_expire_cron() (line 415 of /home3/...../public_html/sites/all/modules/role_expire/role_expire.module).
•Warning: Cannot modify header information - headers already sent by (output started at /home3/...../public_html/includes/common.inc:2567) in drupal_send_headers() (line 1018 of /home3/...../public_html/includes/bootstrap.inc).
•PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '' for key 'name': INSERT INTO {users} (uid, created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 146 [:db_insert_placeholder_1] => 1311893939 ) in drupal_write_record() (line 6861 of /home3/...../public_html/includes/common.inc).

I'm running D7.7 with role_expire downloaded from d.o (2011-07-27). Cron runs just fine for me, even with error_reporting(E_ALL).

My role_expire.module line 415 is this:

      $edit = $account->roles;

If that's what yours is, then there's something terribly fishy going on here. It looks like this is saying that the role_expire table has a uid in it that doesn't exist. Did you do any funny business to your data outside of Drupal? Did you somehow delete a user with a role that was expired before running cron?

Otherwise, I cannot conceive of how you could get that error message at line 415.

Bingo!

I had created a number of test users and deleted them, but their uid stayed in the role_expire table. So, a user shouldn't (can't) be deleted if they have a role expiration date that is not NULL.

I guess a feature request is due... Is there any way to have the module drop (or ignore) any uid from the role_expire table that doesn't exist in the users table?

That would be fairly easy to implement. But might it mask a bona fide data integrity issue that we don't want to cover up? As I reflect, it *seems* harmless. Whatever we do, we should certainly make a point of noticing that condition and logging it.

Whaddya say, want a new patch? ;)

LOL

I can't think of a "normal" reason to keep deleted users in the role_expire table. There may be conditions when historical information might be of value...but I think there are other tables that will capture the data.

I think the downside is more serious - that Cron won't run properly if there is a conflict between uid in the role_expire and users tables.

I like your patches ;-)

It looks like the functionality to "Remove expire time from role" when a user is deleted has been considered in the capacity of rules. Perhaps the rule component might be the best approach as sites can turn it on or off.

...of course that means bringing role_expire.rules.inc to life.

Patch addresses joemaine's comment thread #52-#56, plus some.
1. Includes everything in patch @ #51.
2. Fixes cron (developer-only?) edge-case issues:
a. Writes to "recent log messages" (admin/reports/dblog) when cron deletes a row from row_expires but there's not a corresponding user in the user table.
b. Gracefully handles the situation where the role id is a bizarre number that doesn't correspond to a role name.
3. Removes DBTNG question comments.

To test, feel free to write anything you want into the role_expire table.
a. uid=1, rid=12345, expiry_timestamp=100 (valid uid but invalid rid)
b. uid=123456, rid=12345, expiry_timestamp=100 (invalid uid and invalid rid)
c. a valid uid and valid rid

So what's *required* for a 1.0 version?

All of the stuff that does if (!empty($account)) { to check whether the account exists in my opinion shouldn't be in here. It seems like one of the developers has been deleting users manually from a database rather than using the site normally. I just tried for a while and couldn't get that clause to run unless I deleted a user row from users table manually. So if there is a way to reproduce this bug then please can we document it a little better so it can be reproduced.

Lets pull that code out. Otherwise we are getting there.

For 1.0 I think we would need some simpletests and we need to make a decision on whether we convert the database tables into entities. On larger sites that will use a document database the role_expiry stuff might like to live in something like mongo. If we don't do it in 7.0 then any future code will have to migrate.

Additionally we need to update any tables from d6. At the moment I don't think their are any changes to make.

Stew, thanks for checking out our work. Now I've got to lean on you for your drupal development experience because I'm not sure about the community culture and philosophy about some of these things. I hope you can spare the time to provide a little education.

1. The if (!empty($account)) code: You're exactly right, the reason that code is there is because of the bizarre edge-case of when a developer (or maybe a renegade module?) gets the users table out of sync with the role_expire table. The upside of keeping this is that Drupal doesn't break when well-intentioned but errant admins get those tables out of sync. Please help me understand the downside of keeping that code in. The processing cost is essentially only the evaluation of one if-statement.
2. I thought about simpletest but wasn't convinced that pre-scripted function-level testing of the role_expire functions offered a very high return on investment. Am I missing something here?
3. About Entities -- I get the concept of entities in general terms. How would entities apply here? (A link to good documentation would be helpful.) Are you thinking of considering users as entities? Can role_expire be in the driver's seat about that? Would be glad to help once I understood the philosophy and application better.
4. Are you saying we need an (empty) entry in the role_expire.install file for role_expire_update_7000? Is it a good practice to add entries to this file for major drupal version updates even if there are no updates required? I wouldn't think so...

Created a simpletest script, but can't get all the tests to pass. I think it's a bug in SimpleTest, though. Updates in the attached patch:
1. role_expire.info - includes a files[] line for role_expire.test
2. role_expire.js - converts from document.ready to Drupal.behaviors.
3. role_expire.module - removes TODOs and handles bizarre (developer-induced) edge cases. NEW: Changed date format from 'Y-m-d G:i:s' to 'Y-m-d H:i:s'.
4. tests/role_expire.test - NEW simpletest file.

NOTE: One of the simpletests fails and I need help figuring out why. For some reason, when the role_expire module is enabled, and you uncheck a role on user//edit, all of the role checkboxes completely go away from the form. Obviously this doesn't happen outside of simpletest. It also doesn't happen in the user module's user.test file. I've xdebugged a few hours trying to get this solved, but I apparently don't know where to look...

This patch doesn't apply, Here is the error message.

git apply -v ../role_expire-port_d7-915334-61.patch 
Checking patch role_expire.info...
error: while searching for:
name = Role Expire
description = "Enables user roles to expire on given time and day."
core = 7.x
; Information added by drupal.org packaging script on 2011-07-28
version = "7.x-1.x-dev"
core = "7.x"

error: patch failed: role_expire.info:1
error: role_expire.info: patch does not apply
Checking patch role_expire.js...
Checking patch role_expire.module..

It looks like you are coding against the dev release rather than checking out from git.

This one does go in though. I just changed your .info lines. You didn't actually include the .test file in the patch however. Can you roll a patch with the test file in please?

I've committed the patch from #63 so please pull from git before you reroll

Did a git clone (git clone --branch 7.x-1.x http://git.drupal.org/project/role_expire.git) before rolling this patch.

Patch now contains ONLY the following changes:

• role_expire.info contains a 'files[]' line for the test file plus some cruft you may want to edit.
• tests/role_expire.test is in

I noticed that LICENSE.txt is different from the one I had before. This patch assumes your license file is newer/better than mine, so patch does not contain any modifications to the license file.

And don't forget the simpletest failure comment I mentioned at #61.

Thanks. The packaging script puts the license file in I think. I'll take a look

For the next commit to 7.x-1.x-dev, is it possible to also update the notes (http://drupal.org/node/1209456) ? It still says the user edit form doesn't work. This could prevent some users from trying it.
Thanks.

I've done that. Is that ok?

Yes. Sounds good. Thanks.
Shouldn't we go for a 7.x-1.0-alpha release ?

Thanks for the update on the notes. You said "None of the simpletests work yet." But the only case that fails (for me) is when you remove a role from a user. I'd prefer something like "The Simpletests are a work in progress." ;)

I will make that edit when we commit the tests. They aren't in there yet. I guess I should have said it has no simpletests.

Hey everyone,

I did some testing of the latest 7.x-1.x code today and it's working very well. The main module functionality is working without a hitch and I also got the Views integration working by including the "Role expiration time" field in some user views I created.

What wasn't working for me at all was the module's Rules integration. Should I create a separate issue for fixing Rules integration or is it enough just post to this thread?

Thanks,
Ben

I would probably continue to patch in this thread to get rules integration working.

I've done a bit of work to enhance the Views integration in role_expire. The following patch is pretty much identical to the previous one, with major work in the .views.inc file.

I've done the following which I offer for your review and approval:

1. Renamed the field from "Role expiration time" to "Role expiration date/time" for clarity.
2. In the help instructions on "Role expiration date/time", I added a note to refer the user to also see "Role expiration role".
3. Added the field "Role expiration role" so the view can list the name of the role that corresponds to the expiration date.
4. Added the filter "Role expiration role" so the view can be filtered on roles that have expiration dates. Note that the results of a view using "Role expiration role" is different from the results when you use "User: Role". The difference is that if you use "Role expiration role" your view will contain only that role. But if you use "User:Role", your view will contain all users that have the specified role, which will result in a row for every role that user has, not just the Role expiration role.
5. Added the argument (in D7, now known as "Contextual Filter") for "Role expiration role". When present, you can pass in the role id of the role you want to see in the view.
6. Created the class "role_expiration_handler_field_rid" that handles the display of role IDs as role names.

Sample test instructions:

1. Create a few users with multiple roles and various expiration dates.
2. Create a new "User" view. [I.e., go to admin/structure/views/add. Then "Show *Users* sorted by {whatever}."]
3. Add the following fields to the view: User: Role expiration date/time and User: Role expiration role.
4. Add the following filter to the view: User: Role expiration role.
5. Make sure the filter is exposed.
6. Test various configurations and values for the exposed filter.
7. Add the following "Contextual filter" to the view: User: Role expiration role.
8. Test various values for the "Contextual filter" (on the URL).

This was my first foray in creating Views handlers. I would love for a Views genius to check my work.

Drat! I just discovered that role_expire throws an undefined index Notice if you block/unblock a user at /admin/people. The attached patch covers everything in #74 as well as silencing that Notice.

Suggest you run the sample tests described in the several posts above (#37, #74, and the Simple Tests).

Shall we drop role expire beta1 now as it is Drupalcon and then work on migrate and rules integration for beta2/rc1?

Stew

I say, "Yes!" Let's do it.

Since we've now released a beta version, I'd like to call this one 'fixed' and simply add new issues for Migrate and Rules (see #72: Old Drupal support forum). The reason is, this issue is so all-encompassing, the thread could grow inordinately long. It seems it would be easier to manage each of the other two issues if they were separated. Sound OK?

I'm going ahead and marking this one as fixed. If anyone disagrees, weigh in and change it.

In the meantime, try the beta version that stewsnooze just posted.