This is a follow-up to #2202499: Remove error suppression from fopen().

When fopen() fails, Webform should not attempt to write to the non-existent file handle.

Comments

liam morland’s picture

Status: Active » Needs review
StatusFileSize
new3.07 KB

Fix; also removes error suppression from fclose().

liam morland’s picture

liam morland’s picture

StatusFileSize
new4.83 KB

In this improved version, export functions always return something, making error handling easier.

quicksketch’s picture

Thanks Liam. Do you know exactly what happens when a BatchAPI function returns FALSE? Does that gracefully (relatively) abort the Batch operation?

liam morland’s picture

Most of those functions don't return anything right now, so this doesn't complete the project of nice error handling. It should avoid multiple error message for one problem. For example, if the fopen() fails, you get an error message for that. Then you get errors because the FALSE returned from fopen() is being used as if it was a file handle. With the patch, that won't happen.

liam morland’s picture

If you are concerned about what happens when it returns false, it could return nothing. This would have the effect of having each error cause only one message instead of multiple.

danchadwick’s picture

Status: Needs review » Needs work

Seems like this would be a ton easier with an exception handler, no?

liam morland’s picture

That would be more elaborate. The current patch is supposed to just prevent multiple errors for one problem by stopping it from writing to non-existent file handles. It also removes some error suppression so that errors can be seen and fixed.

liam morland’s picture

Status: Needs work » Needs review
StatusFileSize
new2.47 KB

Here is a simpler patch which just avoids the duplicate error messages. Currently if a file can't be opened, you get the error for that plus the error when it tries to write to the non-existent file handle. With this patch, you get the first error then the function returns.

danchadwick’s picture

Version: 7.x-4.x-dev » 8.x-4.x-dev
Status: Needs review » Fixed

I'm fine with this. Clearly we could do better. The error ends up in the Drupal log and the user gets an empty file, which isn't ideal. But better than having a million errors.

Thanks, Liam. Committed to 7.x-4.x and 8.x.

Status: Fixed » Closed (fixed)

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

danchadwick’s picture

Version: 8.x-4.x-dev » 7.x-4.x-dev