If, when getting a list of issues, projects, etc. available for use when creating comments, any of these happen to contain no values, code execution will not be stopped and eventually a fatal error will occur when drupal_execute() is called later on with bad data.
This error does not currently cause a problem, but because it leads to a fatal error I'm marking this as critical.
The attached patch checks to make sure all of these arrays have at least one value that can be used when creating comments, and if not the function returns and an error is presented to the user.
Comments
Comment #1
dwwGood catch, thanks. My only concerns are:
A) As an API function, I'm not sure a drupal_set_message() is the most appropriate error propagation method. However, I'm not sure what the alternative is, either. ;) But, it's worth at least a little consideration before we go this route. Ideally, the function would return an error value, and the caller can decide if/how to propagate the error depending on what's appropriate given the call site, but that might be too much of a pain.
B) The way this logic works, if you have multiple variables that are empty, you lose that info and the last one you inspect is the one reported. Should we use an array to keep track, instead? Something like the attached (untested) patch.
C) Isn't this potentially a problem with project_issue_generate_issues(), too? I haven't looked closely, but I noticed when I was skimming surrounding code.
Let me know what you think. This might in fact be RTBC, but I'm leaving it CNR.
Thanks,
-Derek
Comment #2
aclight commentedLet's at least go with a patch that doesn't have a syntax error :)
Still untested.
Comment #3
aclight commentedHere's one without CRs. The patch doesn't cause any problems under conditions where the fatal errors wouldn't have been generated in the first place. I still need to test this under conditions where, without the patch, the fatal errors would be present.
Comment #4
aclight commentedRegarding your comments:
A) This function is called by both the testing profile and a form submit function. The project_issue_generate_issue_comments() function itself already has a call to drupal_set_message() at the end indicating that comments were created, so we already have a (arguably) non-ideal API function. However, I don't see a use case for calling this function any where else, and if we did have this API function just return an error code of some sort then the next step would be to just put that into drupal_set_message() from whatever function is calling project_issue_generate_issue_comments(). In short, I don't think this is a big deal.
C) Yep, good catch. That attached patch adds in the error checking there as well.
Comment #5
hunmonk commentedattached patch simply changes from using count() to empty(), which i think is a bit cleaner.
i don't think it's worth wringing our hands over A), since we don't have a use case for it yet.
tested by running issue generation from the admin menu, and nothing is broken. i think this patch is simple enough that no further testing is necessary.
Comment #6
dwwThat works. I'm fine dropping (A). aclight, please commit to HEAD and DRUPAL-5--2 at your earliest convenience. ;) Thanks.
Comment #7
aclight commentedCommitted patch in #5 to HEAD and DRUPAL-5--2.
http://drupal.org/cvs?commit=148486
http://drupal.org/cvs?commit=148485
Comment #8
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.