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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

Status: Active » Needs work

Hmm, 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.

grantlucas’s picture

What 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.

davidwatson’s picture

Title: $runs array not properly instantiated in scanXHProfDir() » $runs array not properly instantiated in scanXHProfDir()

Was 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.

msonnabaum’s picture

You 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.

davidwatson’s picture

The 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.

erikwebb’s picture

Title: $runs array not properly instantiated in scanXHProfDir() » Runs not saved when xhprof.output_dir set to non-existant directory
Version: 7.x-1.0-beta2 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
535 bytes

I'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.

davidwatson’s picture

Status: Needs review » Needs work

Good 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).

erikwebb’s picture

This is using the default XHProf configuration unless it's unavailable. Only then will it use a different value (the Drupal temp directory).

JohnAlbin’s picture

I'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!

erikwebb’s picture

@JohnAlbin - do you think a dsm is appropriate on the configuration page or a full status report entry?

bdone’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
2.58 KB

it 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.

Warning: Invalid argument supplied for foreach() in xhprof_compute_inclusive_times() (line 1670 of .../XHProf/xhprof.inc).
Warning: Invalid argument supplied for foreach() in xhprof_compute_flat_info() (line 1644 of .../XHProf/xhprof.inc).
Warning: Invalid argument supplied for foreach() in theme_xhprof_overall_summary() (line 408 of .../XHProf/xhprof.module).
gapple’s picture

dchatry’s picture

Patch in #11 applies nicely, the warnings are now gone and the error message is much more helpful

osopolar’s picture

Status: Needs review » Reviewed & tested by the community

#11 works for me too, thanks.

  • andypost committed c6f514f on 7.x-1.x authored by bdone
    Issue #1377792 by bdone, erikwebb, grantlucas: Runs not saved when...
andypost’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.