Feeds should handle fatal errors occurring when creating entities. The goal being to not let any problems in the wider entity creation stack interrupt large imports, but rather log them and flag them to the Feeds administrator as a log message.

http://php.net/manual/en/function.set-error-handler.php

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alex_b’s picture

Status: Active » Needs review
FileSize
2.27 KB

First shot. Issues:

- Invokes hooks even though error may be fatal (this may be OK as if an error on invoke occurs, that error is escalated to PHPs error handler).
- Treats every error as fatal and aborts import/clear.

alex_b’s picture

- Only abort if error is fatal.
- Log where error was caused.

To do:

- There is still a possibility that sources are left in a limbo state if a fatal error occurs. Could be avoided by resetting source state to empty before dispatching import / clear.
- Long error messages are hard to read - break them out into a separate error page that shows details? Would require schema changes.

alex_b’s picture

Avoid a notice.

alex_b’s picture

Accidentally had included link patch.

johnbarclay’s picture

Title: Handle fatal errors » Handle fatal errors for entity create with better error handler function

I like this approach to handling entity errors. With the backtrace it gives some good error messaging into a complex part of drupal.

Can the maintainer say if this approach is desireable or the standard try/catch/watchdog approach to error catching. There are a few places in feeds where better error catching could be implemented.

franz’s picture

I didn't compare line by line, but why is it necessary to have a new function to display last caller instead of Drupal's?

franz’s picture

Priority: Normal » Major
franz’s picture

FileSize
3.94 KB

I understand the difference now, but I don't think we need duplicate code to address it. I replaced the duplicated code with _drupal_get_last_caller() which keeps consistency. Also re-rolled.

I haven't tested this, if anyone can review it.

axel.rutz’s picture

@franz #8:
this patch looks totally reasonable. has to be tested though.
(anyone any idea how to mock those fatal entity create errors for automated tests?)

why do we still have identical error handler methods importError() and clearError()?

twistor’s picture

ari-meetai’s picture

I think it would be good to add an option on the import settings for the import to stop on errors. In my case importing more than 100,000 combinations.

twistor’s picture

Assigned: Unassigned » twistor
Issue summary: View changes
Issue tags: +Needs tests

Putting this on my list.

twistor’s picture

Re-trying this. Simplified franz's approach a bit. I think the feeds_get_last_caller() was a bit overkill. Also, I just added the error handler to acquireLock() and releaseLock(). There's a nice symmetry there.

I have an idea for testing this.

twistor’s picture

FileSize
2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 5,304 pass(es). View

I suppose the patch would help.

On to the tests.

MegaChriz’s picture

twistor’s picture

Category: Feature request » Task
Issue tags: +D7 stable release blocker

Gotta resolve this.

I want to explore the deleting on load a bit, but we need to address this in some form.

For the record, I came up with a test for this, but it fails because any kind of fatal error triggers a test failure. Perhaps we can restrict it to run locally only.

twistor’s picture

MegaChriz’s picture

Perhaps you can override DrupalTestCase::errorHandler() in the test class. I think this is the method that triggers a test failure upon a fatal error. Or does the test fail to complete, because the error is "fatal"?