Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geek-merlin’s picture

Title: 'Blank source Foo' documentation and token bug » 'Blank source Foo' broken
Priority: Normal » Critical
Status: Active » Needs review
FileSize
1010 bytes

also such items fail to import completely, so setting critical.

here is a simple patch that fixes all said issues for me.

twistor’s picture

Hey, can you try this one?
We REALLY need a better way to handle the friggin' CSV parser.

GaborTorok’s picture

This is not only a problem with Rewrite plugin, it's a problem with FeedsCSVParser+all plugins (eg.: Rewrite, Copy source value, etc.). As I see element keys are converted lowercase for FeedsCSVParser, except for 'Blank source Foo', except for some other place, where even 'Blank source Foo' should be lowercase. This is madness for me, as I'm new to Feeds and Feeds Tamper internals.
Couldn't it be handled centrally? Even when all Feeds Tamper plugins handle the case correctly, my values are not written to my target entity, because FeedsCSVParser has this override:

  /**
   * Override parent::getSourceElement() to use only lower keys.
   */
  public function getSourceElement(FeedsSource $source, FeedsParserResult $result, $element_key) {
    return parent::getSourceElement($source, $result, drupal_strtolower($element_key));
  }

$element_key is 'Blank source Foo', $source has it as 'Blank source Foo', but FeedsCSVParser converts it to lowercase, so my source value is lost. What is the correct solution? Should Feeds be aware of Feeds Tamper 'Blank source Foo', or should the whole thing be handled somehow in feeds_tamper_feeds_after_parse()? With the second solution (if it is possible), other plugins wouldn't need to handle FeedsCSVParser as an exception.

If I can help, I will try, but I will have to find and go through places that affect $element_key case handling in a code mostly unknown for me.

GaborTorok’s picture

I removed the solution in #2 from the current 7.x-1.x-dev, patched it with #1 and my problem was solved. Before I looked at patch in #1, I also came to that solution myself.
However, this solution doesn't remove the need for all plugins to handle element keys differently for the CSV parser, but they don't need to have an exception for blank sources from the CSV special handling.

vadym.kononenko’s picture

I've reinvent the weel :) in another issue http://drupal.org/node/1938940 and had fixed there this issue too.

#1 works for me.

Anybody’s picture

Status: Needs review » Reviewed & tested by the community
osopolar’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.14 KB

I combined patch from #1 and #2 to just one patch and did a little code formatting. Please review again.

twistor’s picture

Can someone try this?
Trying to clean up some hacks. This should fix quite a few bugs.

twistor’s picture

Initialize every source value.

Status: Needs review » Needs work

The last submitted patch, feeds_tamper-blank-source-1515316-9.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
4.5 KB

Well, that's easily fixable.

Status: Needs review » Needs work

The last submitted patch, feeds_tamper-blank-source-1515316-11.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
John Bickar’s picture

Trying to test this so that we can get a point release, but I can't make heads or tails of the issue summary.

Here are a couple of screenshots from simplytest.me with the patch added; I can't see "Blank source Foo" anywhere.

twistor’s picture

The bug is solved, I just needed someone to test existing installs to make sure the fix doesn't have any unexpected side effects.

I've added more test coverage. Assuming this comes back green, I will commit it and roll the release on Monday.

twistor’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)
byrond’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Issue summary: View changes
Status: Patch (to be ported) » Needs work

After updating to this version, one of our mappings broke. We have an RSS category mapped to a Tags field on the Article content type. We used tamper plugins to Implode the array into a string (separated by newlines), Rewrite the string to add a term, do several Find/replaces on some terms, and finally Explode the string back into an array. After updating to 1.0-beta5, rewriting and find/replace no longer work, and the data gets imported as-is from the source. The Find/replace plugin now generates the following error:

Warning: Invalid argument supplied for foreach() in feeds_tamper_feeds_after_parse()

Drupal core 7.26
Feeds 2.0-beta8
Feeds Tamper 1.0-beta5

The commit immediately before the one that applies the patch for this issue works fine.

twistor’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Priority: Critical » Major
Status: Needs work » Patch (to be ported)

#17, There's been lots of fixes. If you're still having this problem, please create a new issue.

twistor’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed

This patch did cause some problems. I'm not going to port it to 6.x.

Nobody is using 6.x anyway.

Status: Fixed » Closed (fixed)

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