Each time I run cron, only a single ticket is closed at a time because project_comment_save() will only save a change if $edit->cid is not already set. However, the loop performed by project_issue_cron() over projects that are ready to be automatically closed does not reset $edit->cid, so after the first iteration, no changes are made.

CommentFileSizeAuthor
#3 cron-more-than-one-comment.patch719 byteszostay

Comments

hunmonk’s picture

hunmonk commented
Status: Active » Postponed (maintainer needs more info)

wha? $edit is not a static variable -- it's reset each time that function is called. if you're experiencing this problem, that's not why. please investigate further and provide a walkthrough of how exactly to reproduce it. FYI, you might be experiencing some side effects from http://drupal.org/node/173657

zostay’s picture

zostay commented

I think you're right, my bad. For some reason I thought $edit was passed by reference. I'm still having the problem after the patch to the other ticket, I will continue to investigate. I've got a lot of other modules installed so it might be some interaction with one of them too.

zostay’s picture

zostay commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new cron-more-than-one-comment.patch719 bytes

Nope, my original post was completely correct, the behavior appears to be consistent with my original post. It doesn't matter that $edit is not static, it's an object that isn't reset each time. Therefore, a change to $edit->cid, carries back out to $comment->cid, which is never reinitialized. I can provide additional evidence of the problem if you want it.

I've attached a patch that should cleanly resolve the problem (and make the code somewhat safer) in any case.

aclight’s picture

aclight commented
Version: 5.x-1.0 » 5.x-1.1
Status: Needs review » Reviewed & tested by the community

I ran into this same problem on my site as well. On the site all project_issue nodes are visible only to a few users. I applied the patch in http://drupal.org/node/173657 first but each time cron ran only one issue was changed from fixed --> closed. The patch in #3 fixes the problem.

hunmonk’s picture

hunmonk commented
Status: Reviewed & tested by the community » Needs review

i won't commit this fix yet. i ran cron.php on my local test install, and as expected, it auto-closed all fixed issues older than two weeks in one run. we should understand what is causing this issue before we provide a fix. as far as i can tell, $edit->cid is empty on each successive call (as it should be, because the variable is not passed by reference).

zostay’s picture

zostay commented

That's a good call. Since I do much of my work not in PHP, I'm not familiar with all the minutia of how objects are passed. I'm used to Java or Perl where an object is always passed by reference and reading through php.net, I couldn't determine if this is true or not. Since you aren't seeing the behavior I am and probably have a better handle on PHP, I leave it to you. If you want a copy of my php.ini or anything like that I can pass it along.

I'd recommend anyone having the same problem as myself and aclight consider the patch until this is worked out.

hunmonk’s picture

hunmonk commented
Status: Needs review » Postponed (maintainer needs more info)

@zostay: out of curiosity, is your php.ini setting allow_call_time_pass_reference set to On? it doesn't even seem like this would really be an issue, but it's the only thing i have to go on right now...

aclight’s picture

aclight commented
Status: Postponed (maintainer needs more info) » Active

I know I'm not zostay, but I was seeing this same thing on my site. From phpinfo() on my site:

                                             Local Value                     Master Value
allow_call_time_pass_reference               On                              On
dww’s picture

dww
we/he/they
commented
Status: Active » Fixed

zostay's patch in #3 is clearly a good move in terms of defensive programming in the code, and aclight confirms it solves the problem. I don't see any reason to invest more time in this. I slightly modified the comment, but otherwise, committed this to HEAD, DRUPAL-5, DRUPAL-4-7--2, and DRUPAL-4-7. Thanks, all!

hunmonk’s picture

hunmonk commented

@aclight: FWIW, http://www.php.net/ini.core explains what that ini parameter does. as i understand it, just because it's turned on doesn't mean that the variable is passed by reference. if you do:

$blah = myfunction($foo);

the variable should still be passed by value.

as opposed to:

$blah = myfunction(&$foo);

which does pass the variable by reference, without warning, if allow_call_time_pass_reference is set to on.

or the function definition itself can declare that the var is passed by reference:

myfunction(&$foo) {
  ...stuff here...
}

we have no reference stuff going on at all in the code in question, which is why i find this issue troubling. perhaps a PHP guru could weigh in and enlighten us as to wtf is going on here... ;)

aclight’s picture

aclight commented

@hunmonk: I wasn't saying that the variable is being passed by reference, just that my setting is on. I agree that it shouldn't be passed by reference, but then again I'm not a PHP guru. I'm just glad that zostay was able to come up with a way to fix the problem. I understand not wanting to apply a fix when you aren't sure why the problem exists in the first place, but given how minor the fix was I think it's fine to apply it here, as dww has. Thanks for looking into this however, and I'm glad the problem seems to be fixed.

Anonymous’s picture

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