Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As it stands right now it looks like if no runs exist, $runs isn't initiated to an array before being passed to the final return statement which calls array_reverse, and thus expects an array.
Added a patch which moves $runs = array() outside of the is_dir() if statement.
Comment | File | Size | Author |
---|---|---|---|
#11 | XHProf-check_for_valid_tmp_dir-1377792-11.patch | 2.58 KB | bdone |
#11 | interdiff-6-11.txt | 2.05 KB | bdone |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedHmm, it appears that this would be an issue if the directory didn't exist, not if there weren't any runs. I agree that this isn't necessarily handled properly, but I don't think this patch fixes it.
Comment #2
grantlucas CreditAttribution: grantlucas commentedWhat changes would you suggest? I don't know too much about the code done in the module. Just came across this error when first enabling the module.
Right now, where the results of scanXHProfDir() are used, an array is expected and passing an empty array back if the directory doesn't exist seems to resolve the warning without having to alter too much elsewhere in the module.
Comment #3
davidwatson CreditAttribution: davidwatson commentedWas bitten by this as well when the directory was missing. As #1 mentioned, creating the directory fixes the problem, though the error handling could stand to be more graceful.
As I understand it, we should attempt to create the directory if it doesn't exist. If we can't, throw an exception and warn. If this sounds like a reasonable approach, I'll go ahead and whip up a patch.
Comment #4
msonnabaum CreditAttribution: msonnabaum commentedYou should have already got a watchdog message for the directory not being there, but it should probably throw an exception.
Either way, I'm confused about how this scenario comes about. Are you setting xhprof.output_dir in php.ini to a direcotry that doesn't exit? Do you not have a /tmp directory? Maybe a better solution would be to use file_directory_temp(). The hardcoded /tmp is left over from facebook's code.
Comment #5
davidwatson CreditAttribution: davidwatson commentedThe former: I had set the xhprof.output_dir in php.ini, and I must've been pulled away to handle another issue before I actually created it the first time around. Scratched my head for a moment at the error, realized the directory didn't exist, felt silly, created it, and all was well.
Granted, not a common scenario, but it could (did) happen.
Comment #6
erikwebb CreditAttribution: erikwebb commentedI've taken a slightly different approach - check if the default XHProf directory exists; if not, use the Drupal temp directory (then if that doesn't exist, you have bigger problems!).
Also, I removed the shorthand ternary operator because it assumes PHP 5.3.
Comment #7
davidwatson CreditAttribution: davidwatson commentedGood call on the shorthand. But shouldn't we be honoring XHProf configuration settings where we can? It would work, yes, but I could see it causing confusion later on down the line when people are trying to figure out where their runs ran off to (no pun intended).
Comment #8
erikwebb CreditAttribution: erikwebb commentedThis is using the default XHProf configuration unless it's unavailable. Only then will it use a different value (the Drupal temp directory).
Comment #9
JohnAlbinI've reviewed the patch in #6 and it works fine. I think it should be included in the final patch for this issue.
However, it still doesn't solve the problem of how to inform the user about the underlying problem.
I happened to have a misconfigured server because I copied existing configuration files to a new machine, but the directories on the new machine weren't set up properly. So both the xhprof output_dir and Drupal's temp directory were unaccessible to Apache. yayz!
Comment #10
erikwebb CreditAttribution: erikwebb commented@JohnAlbin - do you think a dsm is appropriate on the configuration page or a full status report entry?
Comment #11
bdone CreditAttribution: bdone commentedit seems like a dsm() would be helpful here. there's already some watchdog notices appearing via XHProfRunsInterface get_run().
here's a patch that provides a message. it also casts empty arrays, to avoid a few warnings when the directory isn't there.
Comment #12
gappleMarked #2286747: No error given if xhprof.output_dir doesn't exist or not writeable as a duplicate
Comment #13
dchatryPatch in #11 applies nicely, the warnings are now gone and the error message is much more helpful
Comment #14
osopolar#11 works for me too, thanks.
Comment #16
andypost