Use FeedsProcessor::getHookTargets() if available.

the same problem

Typo

this is a bug .

the plugin implementation is missing a call to : $this->getHookTargets($targets);

bug is here :
http://cgit.drupalcode.org/commerce_feeds/tree/plugins/FeedsCommerceProd...

example from feeds Node plugin :
http://cgit.drupalcode.org/feeds/tree/plugins/FeedsNodeProcessor.inc#n342

i make priority major because this is a crucial feature of drupal field api and have been ignored for two years already .

The problem is not actually with the missing getHookTargets() call. That method was added so that it's backwards compatible with older versions. The real problem is with the feeds_alter() usage, which is fixed in dev.

no, you are wrong .
current code doesn't invoke hook_feeds_processor_targets .
please open the actual code of that method in feeds/plugins/FeedsProcessor.inc and see the difference :

  $targets += module_invoke_all('feeds_processor_targets', $entity_type, $bundle);

you think by calling self::loadMappers() you are invoking all other mappers, but you are not .

moreover open feeds/plugins/FeedsNodeProcessor.inc and see how it is properly implemented using getHookTargets and not by directly calling self::loadMappers .

please read my last comment .

i also don't understand what you mean by feeds_alter problem and how it is fixed in dev version .

hmmm... Seeing as how I wrote all of the code that you're talking about, I'm well aware of how it works.

You are correct, Feeds is using getHookTargets(), as this module should *eventually*. But, there is a specific backwards compatible shim in place so that this upgrade isn't necessary yet. I want to make sure that it's working, before trying to patch this module.

see _feeds_feeds_processor_targets_alter() and feeds_feeds_processor_targets().

Please actually test the dev release and report your findings, then we can patch things.

i tried understanding the problem but it seemed complicated,
maybe when i have more time i will look at it again .

can you just tell me how to deal with this in my current project ?
currently i fixed it using this hook :

function hook_feeds_processor_targets_alter(&$targets, $entity_type, $bundle) {
  if ($entity_type == 'commerce_product') {
    $targets += module_invoke_all('feeds_processor_targets', $entity_type, $bundle);
  }
}

That will work, but it should be necessary since that's what Feeds is doing already.

Have you tried the dev version of this module? The main problem I see here is that the current release is using feeds_alter() instead of drupal_alter().

it is not about what feeds is doing already, but about FeedsCommerceProductProcessor not calling the method which allows feeds to do what it does :)

bottom line without the alter i implemented above, then all field api attachments are not presented as targets under importer processor mapping .
and i prefer to use this simple alter than having to upgrade the whole module to an unstable dev version .

anyway, i still haven't looked further into the reason you hold back on using getHookTargets() .
you are preventing a very critical feature which is field api support from all your module's users .

I'm going to try to explain this once again.

it is not about what feeds is doing already, but about FeedsCommerceProductProcessor not calling the method which allows feeds to do what it does :)

This is where your mistaken. Feeds is doing everything already. Modules shouldn't have to change for this to work. If they do, then there is a bug in Feeds that needs to be fixed.

bottom line without the alter i implemented above, then all field api attachments are not presented as targets under importer processor mapping .
and i prefer to use this simple alter than having to upgrade the whole module to an unstable dev version .

How is writing a hack simpler than using code with a fix already?

anyway, i still haven't looked further into the reason you hold back on using getHookTargets() .
you are preventing a very critical feature which is field api support from all your module's users .

No, I am not holding anything back. You're free to write a patch, if you feel the urge, however, if it gets committed, then you'll still have to use the dev version until a new release is published.

The only thing holding me back from writing a "proper" fix is that nobody will test the dev version and verify if that fixes it.

@twistor,

I was having the same issue of not picking up additional text and term reference fields. I've installed the dev version and it does in fact make all my additional fields available for mapping.

Thanks a ton! Please let me know if there any other specific tests I could run to help out. From what I can tell it solved my immediate problem of not seeing my extra commerce product fields in the mapping target.

Feeds 

I had the same problem with missing fields in the mapping page.
I've installed 7x 1x dev. and all the missing fields are present.
I re-ran my most recent feed with the 'skip hash check' selected (10,000+ lines) and all the missing data was written to the database. Thank you.

Hi @twistor,

I was having issues because the image target was missing and I had added another field to my product and it wasn't showing up. I uninstalled commerce_feeds and installed the dev release and now they are showing up. I don't know if this has anything to do with this issue or not. I just thought I would let you know.

Thanks!

-Caleb

Hello,
I want to use the -dev version to solve this problem as we very much need the feeds importer to work again on our live site.

But to install the -dev version I have to throw away the current version, which I cant, because it is using all our importers. Do I really have to delete all that and rebuild all to try the dev version?

I tried $ drush dl commerce_feeds --dev
it downloads the right version, asks i i want to overwrite, i say yes
But when i look into the backend the version still says: 7x-1.3
and the importer is still not showing the right fields.
What am I doing wrong?

I'm using commerce_kickstart 7X-2.28 all worked fine until the last update :-(

edit:
1 day later: I foud out why it is not working, you need to dis and pmu and manually remove ALL feeds AND commerce feeds folders (and feeds tamper)
then install a --dev version for feeds AND commerce_feeds
then reimport importers and yes now it works.

Does not make me happy to have so much work to do after simple kickstart update (again)

quote "Does not make me happy to have so much work to do after simple kickstart update"

well gee .... maybe you all should have read my initial comment about adding three simple lines of an alter function that solves the problem without need to upgrade to dev version .

Worked for me just after replacing recomended of version the Commerce Feeds module by dev version

when you are using a distribution you can get into trouble when you upgrade some module individually without upgrading the whole distribution .

@yakoub I certainly should have read that... totally missed it.

Tested it with the dev version of commerce_feeds and the missing targets are back.

Please check patch for that

We've just recently had the missing targets show up after updating to the latest dev version of feeds. The fix above provided by id.tarzanych fixes this issue. I took a closer look to figure out exactly what was causing this to suddenly fail and to give you more info to justify patching this issue.

The issue seems to be related to the changes added in this commit on the feeds module which @twistor was the author of.

http://cgit.drupalcode.org/feeds/commit/?id=12bd52b9b0c4badfcb1d53e4b71b...

specifically this change

+  $targets = feeds_importer($form['#importer'])->processor->getMappingTargets();
+  $targets = _feeds_ui_format_options($targets, TRUE);
+
+  $sources = feeds_importer($form['#importer'])->parser->getMappingSources();
+  // Some parsers do not define source options.
+  $sources = $sources ? _feeds_ui_format_options($sources, TRUE) : array();

and

-      // Some parsers do not define source options.
-      $source = isset($form['source']['#options'][$mapping['source']]) ? $form['source']['#options'][$mapping['source']] : $mapping['source'];
-      $target = isset($form['target']['#options'][$mapping['target']]) ? check_plain($form['target']['#options'][$mapping['target']]) : '<em>' . t('Missing') . '</em>';
+      $source = isset($sources[$mapping['source']]) ? check_plain($sources[$mapping['source']]) : check_plain($mapping['source']);
+      $target = isset($targets[$mapping['target']]) ? check_plain($targets[$mapping['target']]) : '<em>' . t('Missing') . '</em>';

The call to getMappingTargets fails to return the additional targets provided by hook_feeds_processor_targets. The module_invoke_all called in _feeds_feeds_processor_targets_alter fails to get called.

/**
 * Implements hook_feeds_processor_targets_alter().
 *
 * @see feeds_feeds_processor_targets()
 * @see feeds_feeds_processor_targets_alter()
 */
function _feeds_feeds_processor_targets_alter(array &$targets, $entity_type, $bundle) {
  // If hook_feeds_processor_targets() hasn't been called, for instance, by
  // older processors, invoke it ourself.
  if (!drupal_static('feeds_feeds_processor_targets', FALSE)) {
    $targets += module_invoke_all('feeds_processor_targets', $entity_type, $bundle);
  }
}

This function is actually called twice during the page request at /admin/structure/feeds/[name]/mapping.

The first call is in feeds_ui_mapping_form and does actually execute the module_invoke_all('feeds_processor_targets', $entity_type, $bundle) line and return all the correct targets.

The second call in theme_feeds_ui_mapping_form does NOT execute the module_invoke_all('feeds_processor_targets', $entity_type, $bundle) line and fails to retrieve the additional targets.

I think this is enough justification to use $this->getHookTargets($targets) in the feeds processors getMappingTargets function which will properly invoke hook_feeds_processor_targets for all modules defining it.

So @twistor is right that feeds should already be handling this but things seem to fall appart when get getMappingTargets is called multiple times on the same page request. The second call never gets the additional targets.

We've been using #24 in production for 3 months now. The patch is straightforward and very well explained in #24.

Committed #24.