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

CommentFileSizeAuthor
#14 feeds-error-handler-955236-14.patch2.46 KBtwistor
#8 feeds-error_handler.patch3.94 KBfranz
#4 955236-4_error_handling.patch4.88 KBalex_b
#3 955236-3_error_handling.patch10.26 KBalex_b
#2 955236-2_error_handling.patch4.82 KBalex_b
#1 955236-1_error_handling.patch2.27 KBalex_b

Comments

alex_b’s picture

alex_b commented
Status: Active » Needs review
StatusFileSize
new 955236-1_error_handling.patch2.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

alex_b commented
StatusFileSize
new 955236-2_error_handling.patch4.82 KB

- 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

alex_b commented
StatusFileSize
new 955236-3_error_handling.patch10.26 KB

Avoid a notice.

alex_b’s picture

alex_b commented
StatusFileSize
new 955236-4_error_handling.patch4.88 KB

Accidentally had included link patch.

johnbarclay’s picture

johnbarclay commented
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

franz
any
Location Montréal
commented

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

franz
any
Location Montréal
commented
Priority: Normal » Major
franz’s picture

franz
any
Location Montréal
commented
StatusFileSize
new feeds-error_handler.patch3.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.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

@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

twistor commented

Cross referencing #1029102: Importing Large CSV document (downloaded and processed in the background).

ari-meetai’s picture

ari-meetai commented

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

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

Putting this on my list.

twistor’s picture

twistor commented

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

twistor commented
StatusFileSize
new feeds-error-handler-955236-14.patch2.46 KB

I suppose the patch would help.

On to the tests.

megachriz’s picture

megachriz
Primary language Dutch
commented

This is probably a related issue: #1953008: PHP Fatal error: Nesting level too deep - recursive dependency? in FeedsProcessor.inc on line 199

twistor’s picture

twistor commented
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

twistor commented
Related issues: +#1029102: Importing Large CSV document (downloaded and processed in the background)
megachriz’s picture

megachriz
Primary language Dutch
commented

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"?

bluegeek9’s picture

bluegeek9 commented
Assigned: twistor » Unassigned
Status: Needs review » Closed (outdated)
Issue tags: -Needs tests, -D7 stable release blocker

Drupal 7 reached end of life and the D7 version of Feeds is no longer being developed. To keep the issue queue focused on supported versions, we’re closing older D7 issues.

If you still have questions about using Feeds on Drupal 7, feel free to ask. While we won’t fix D7 bugs anymore, we’re happy to offer guidance to help you move forward. You can do so by opening (or reopening) a D7 issue, or by reaching out in the #feeds channel on Drupal Slack.

If this issue is still relevant for Drupal 10+, please open a follow-up issue or merge request with proposed changes. Contributions are always welcome!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.