When usernode is enabled it tries to update all users -- deleting users that don't exist and adding new users. If there are a lot of existing users, usernode can easily take too long to run and php can abnormally abort.

The attached patch limits the number of users to add to 100 per run -- starting the process when the module is enabled, but uses cron to add subsequent users.

CommentFileSizeAuthor
#6 126917_0.patch1.76 KBdouggreen
#4 126917.patch1.61 KBdouggreen
limit.patch1.14 KBdouggreen

Comments

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Active » Needs work

hm, doing it that way would be really inefficient. Consider sites that use cron really often, it will have to do a lot of user checking... I think we need a way to determine in the cron hook in a efficient way if there is work to do, perhaps through setting a $conf variable.

douggreen’s picture

douggreen commented

Once the users are updated, it doesn't do any additional work. So, the only real work in cron is a single SELECT statement. If this really bothers you, we can add a variable_get/variable_set.

Personally, I don't like this solution because it essentially stores the same information in two places, leaving the possibility that the cached version will be out of date with the real version. As there's not much of a performance gain (saving a single SELECT on every cron), I opt for simplicity and maintainability.

fago’s picture

fago
Primary language German
Location Vienna
commented

no, it will do two not really simple queries. And that every time although there is no need to it, so yes I worry about it. I like the idea, however it needs to be done in a efficient way.

So a simple variable_set/get would just solve it. The variable could be set on hook_enable and deleted after the work has been completed.

douggreen’s picture

douggreen commented
StatusFileSize
new 126917.patch1.61 KB

Patch attached as requested...

fago’s picture

fago
Primary language German
Location Vienna
commented

thanks for your work!
Please return TRUE and FALSE instead of 0,1.. Also use boolean values as default values for the variable_get() not 0.
Furthermore I've overlooked a small thing that was already in your last patch: To use the db-abstraction layer correctly use http://api.drupal.org/api/5/function/db_query_range instead of manually adding LIMIT to the query.

Otherwise patch looks fine and will be a great addition to usernode!

douggreen’s picture

douggreen commented
StatusFileSize
new 126917_0.patch1.76 KB

New patch attached. Good catch on the db_query_range(), I obviously forgot about that. I also changed the return 0/1 to FALSE/TRUE because you asked.

But I do believe that this is bad practice by the Drupal community, and I'm trying to get some steam around the discussion. You can follow the thread here,

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Fixed

thanks! I've further replaced two other 0/1 with FALSE/TRUE and added a comment.
Committed to 5.x and HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)