PHP Fatal error: Nesting level too deep - recursive dependency? in FeedsProcessor.inc on line 199

I encountered the exact same problem when working on a processor plugin for Ubercart Addresses. The entity contains a cross reference, resulting in the "Nesting level too deep" error. The easiest way I see to fix this is to move generating the error to a separate method, so other classes have a chance to override the method. Of course, any processor plugin that deals with classes that can have cross references should override this method too.

The attached patch moves the code for creating the error message to a new method called createLogMessage().

Oops, I had set the parameters in the wrong error when calling the createLogMessage() method:
+ $message = $this->createLogMessage($e, $item, $entity);

Should have been:

+ $message = $this->createLogMessage($e, $entity, $item);

New patch.

I've gone ahead and committed this to 7.x-2.x. Anything that makes the process() method shorter is good to me. I also switched to drupal_var_export() because it makes nicer exports. Not sure if it will help though.

Thanks for committing the patch!

Other solutions?

Meanwhile, I had been looking for better solutions for this issue, though I have not found one yet. I was looking for a way to move the whole contents of try into a method called processItem(). This way classes could override this method, call the parent and decide if they want to catch the Exception themselves (see pseudo code below for an example). The caveat is that a lot of variables defined before the try statement are used inside the try, thus that would mean a lot of parameters to pass for the processItem() method. Maybe something like an $options parameter would do the trick, though that would be a bit dirty as the variables are not really options.

Pseudo code:

class MyProcessor extends FeedsProcessor {
  /**
   * Overrides FeedsProcessor::processItem().
   */
  protected function processItem() {
    try {
      parent::processItem();
    }
    catch (FeedsValidationException $e) {
      // Example: add extra information and let FeedsProcessor handle the rest.
      $e->my_var = 'value';
      throw $e;
    }
    catch (FeedsAccessException $e) {
      // Example: log the error, but proceed anyway.
      $source->log('import', 'no access', array(), WATCHDOG_WARNING);
    }
  }
}

abstract class FeedsProcessor extends FeedsPlugin {
  // (...)
  public function process(FeedsSource $source, FeedsParserResult $parser_result) {
    // (...)
    try {
      // @todo pass parameters.
      $this->processItem();
    }
    // Something bad happened, log it.
    catch (Exception $e) {
      $state->failed++;
      drupal_set_message($e->getMessage(), 'warning');
      $message = $this->createLogMessage($e, $entity, $item);
      $source->log('import', $message, array(), WATCHDOG_ERROR);
    }
    // (...)
  }

  /**
   * Processes a single item.
   *
   * @todo define parameters.
   */
  protected function processItem() {
    // Load an existing entity.
    if ($entity_id) {
      $entity = $this->entityLoad($source, $entity_id);

      // The feeds_item table is always updated with the info for the most
      // recently processed entity. The only carryover is the entity_id.
      $this->newItemInfo($entity, $source->feed_nid, $hash);
      $entity->feeds_item->entity_id = $entity_id;
    }
    // (...)
  }
}

drupal_var_export()

I also switched to drupal_var_export() because it makes nicer exports. Not sure if it will help though.

drupal_var_export() does a better job for stdClass instances, but not for instances of other classes: it won't be a direct solution for objects with cross references.

Backport not needed?

It looks like that there is no need for a backport to 6.x-1.x, at least not yet, because the process() method in the 6.x-1.x version is abstract, so basically there is nothing from this issue to backport at this point.

I'm bumping this back up. I think we can kill a whole class of bug reports if we just fix this.

This makes the log much more usable, plus it escapes the output, which we should be doing already.

This issue is related. The nesting level issue can cause Drupal to not release the lock, so trying to import again breaks.

The problem with this approach is that the variables are still concatenated directly into the message string. That message string can become quite large and pollutes the locales_source table since it is run through t() on display. So you get lots of source translation variants in your translation system, which is bad. The message should be a static string with placeholders, I propose that in #2502419: Log messages XSS attack vector.

I agree with klausi that we shouldn't put the whole message, including exception and var export in one string to log. The patch in #10 should be merged with the patch from #2502419: Log messages XSS attack vector. This will require an API change, since the return signature for the createLogMessage() will change from string to array (see the patch from the other issue). We should take in account though that modules that override that method may still return a string (Ubercart Addresses does for example, though since I control that module I can easily adjust that once this issue is fixed).

There are also other places where an exception message is directly logged. Fixing these is beyond the scope of this issue, we should handle that in #2364103: Feeds error log crashes when log messages are too long.

I think we should create a new method that has the correct return variables and deprecate createLogMessage(). A security release that breaks things will make it harder to adopt. A polluted locales_source table is minor compared to someone avoiding the update because it breaks their processor. Custom implemented or otherwise.

Yep, we could do that, patch attached.

We need sanitizing in 6.x as well.

Err, other issue.