When using the CSV importer on a feed and triggering the import via Drush instead of the UI, I've noticed that the source configuration isn't used properly.

I've debugged my way around the problem and it seems to stem from the following in FeedsCSVParser:

  public function parse(FeedsSource $source, FeedsFetcherResult $fetcher_result) {
    $source_config = $source->getConfigFor($this);

I added print_r($source_config); right after that and it shows the following:

Array
(
    [delimiter] => TAB
    [encoding] => UTF-8
    [no_headers] => 1
)

However, if I change the first line to this:

    $source_config = $this->getConfig();

.. it shows the options I actually set in the configuration:

Array
(
    [delimiter] => ,
    [encoding] => UTF-8
    [no_headers] => 0
)
CommentFileSizeAuthor
#2 feeds-n3004499-2.patch532 bytesdamienmckenna

Comments

DamienMcKenna created an issue. See original summary.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new feeds-n3004499-2.patch532 bytes

This changes the line to $this->getConfig(); as suggested.

Status: Needs review » Needs work

The last submitted patch, 2: feeds-n3004499-2.patch, failed testing. View results

megachriz’s picture

megachriz
Primary language Dutch
commented

Thanks for the patch, but I think this change is not right. The delimiter setting can be overridden on the source form and with the proposed change that override no longer works. I think the issue is that the Drush command gives unexpected results when the FeedsSource object is created by the Drush command (instead of loaded from the feeds_source table). FeedsCSVParser::sourceDefaults() does default to the importer's config though, so not sure how that happened.

The other possible cause is that the FeedsSource did actually come from the feeds_source table and that the source config was in fact already overidden. Changing the importer config does not update the source config.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Title: CSV importer uses wrong config for delimiter » Drush feeds-import command ignores stored config for CSV parser

That's a fair point.

So it seems there might be multiple items at fault here.

You are correct in the hypothesis that the config was overridden, in this use case I'm working from an exported configuration which I overwrote to test different import options. I'll try re-exporting it to see if that makes a difference.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Ah! The configuration I'm overwriting comes from commerce_feeds. I'll try manually exporting it to a new feature.

megachriz’s picture

megachriz
Primary language Dutch
commented

Note that the source's config is not exported to features. This config is saved in the feeds_source table. A FeedsSource config usually contains the url to the file to import (HTTP fetcher) or a reference to an uploaded file (File fetcher). But in case of the CSV parser also the delimiter. I think the reason behind is that if you allow your content editors to import content, the editors are allowed to specify the format of his/her CSV.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Regardless of what someone does at /import/FEEDNAME, there's still a disconnect because config changes on /admin/structure/feeds/FEEDNAME/settings/FeedsCSVParser are not loaded when you run the importer via Drush, which should load the current configuration.

It sounds like the configuration loading architecture is slightly wonky - the internal API should always properly load the configuration, and then the feed runner should be responsible for layering additional overrides on top of that. It seems like instead that the CSV importer assumes the configuration is always being loaded from the /import/FEEDNAME UI so doesn't properly load the full configuration.

megachriz’s picture

megachriz
Primary language Dutch
commented

If there is no config on the FeedsSource, the CSV parser takes the config from the importer:

/**
 * Define defaults.
 */
public function sourceDefaults() {
  return array(
    'delimiter' => $this->config['delimiter'],
    'encoding' => $this->config['encoding'],
    'no_headers' => $this->config['no_headers'],
  );
}

$this->config should exist when the parser is configured on the importer and contain the importer's parser config. When looking at the code, I see that FeedsConfigurable (where FeedsCSVParser derives from) fills $this->config with the config defaults, but FeedsPlugin (which extends FeedsConfigurable and where FeedsCSVParser also derives from) does not call its parent constructor. The docs to that method say that calling the parent constructor resulted into "circular dependencies" (I think it meant to say "an endless loop"). So what happens if you don't configure the CSV parser at the importer level at all and then run the drush command "feeds-import". Then there would be an empty $this->config, right? Is that the issue you are facing?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Given there are drush commands, you shouldn't have configure anything at the /import/FEEDNAME pages at all, it should work from the exported configuration.

I reverted back to the latest -dev release, cloned the existing importer with a new name, exported it with Features, reverted it so it shows as a default config, then ran the importer via Drush - it works as intended. So it seems the problem is that the overridden config values through /admin/structure/feeds/FEEDNAME/settings/FeedsCSVParser are ignored when you use the Drush commands.

megachriz’s picture

megachriz
Primary language Dutch
commented

No, the importer's config for the CSV parser is ignored when there is config for the CSV parser on the FeedsSource instance. In other words: the CSV config on the importer is only used when creating new Feeds sources, not when reusing existing ones.

I think the issue is that when using the drush "feeds-import" command (and the importer is not attached to a content type), you don't know if there is already a Feeds source or not (there's however an other drush command to check that). When using the '--file' option, a source is created automatically if one doesn't exist already. In case one exist, that Feeds source is used, together with all config that is stored on that object, which - in case of the CSV parser - always takes precedence on the importer's config.

bluegeek9’s picture

bluegeek9 commented
Status: Needs work » Closed (outdated)

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.