Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
I have a products catalog based on taxonomy vocabulary, as advised the term reference field is on the product content type.
Products imports are working fine, but when I try to import product displays the mapping for term reference does not appear in the target list.
I'm using Feeds 7.x-2.0-alpha8
Thanks in advance
Comment | File | Size | Author |
---|---|---|---|
#24 | use-gethooktargets-1998222-24.patch | 1.97 KB | agn507 |
#22 | 1998222-use-gethooktargets-for-commerce-feeds-22.patch | 1.82 KB | id.tarzanych |
Comments
Comment #1
korzh-nick CreditAttribution: korzh-nick commentedthe same problem
Comment #1.0
korzh-nick CreditAttribution: korzh-nick commentedTypo
Comment #2
yakoub CreditAttribution: yakoub commentedthis 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
Comment #3
yakoub CreditAttribution: yakoub commentedi make priority major because this is a crucial feature of drupal field api and have been ignored for two years already .
Comment #4
twistor CreditAttribution: twistor as a volunteer commentedThe 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.
Comment #5
yakoub CreditAttribution: yakoub commentedno, 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 :
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 .
Comment #6
yakoub CreditAttribution: yakoub commentedplease read my last comment .
Comment #7
yakoub CreditAttribution: yakoub commentedi also don't understand what you mean by feeds_alter problem and how it is fixed in dev version .
Comment #8
twistor CreditAttribution: twistor as a volunteer commentedhmmm... 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.
Comment #9
yakoub CreditAttribution: yakoub commentedi 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 :
Comment #10
twistor CreditAttribution: twistor as a volunteer commentedThat 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().
Comment #11
yakoub CreditAttribution: yakoub commentedit 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 .
Comment #12
twistor CreditAttribution: twistor as a volunteer commentedI'm going to try to explain this once again.
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.
How is writing a hack simpler than using code with a fix already?
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.
Comment #13
droddis CreditAttribution: droddis commented@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
Comment #14
AlanAT CreditAttribution: AlanAT commentedI 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.
Comment #15
calebyoder CreditAttribution: calebyoder commentedHi @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
Comment #16
MariannevdS CreditAttribution: MariannevdS commentedHello,
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)
Comment #17
yakoub CreditAttribution: yakoub commentedquote "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 .
Comment #18
Berlaus CreditAttribution: Berlaus commentedWorked for me just after replacing recomended of version the Commerce Feeds module by dev version
Comment #19
yakoub CreditAttribution: yakoub commentedwhen you are using a distribution you can get into trouble when you upgrade some module individually without upgrading the whole distribution .
Comment #20
MariannevdS CreditAttribution: MariannevdS commented@yakoub I certainly should have read that... totally missed it.
Comment #21
akosipax CreditAttribution: akosipax commentedTested it with the dev version of commerce_feeds and the missing targets are back.
Comment #22
id.tarzanych CreditAttribution: id.tarzanych commentedPlease check patch for that
Comment #23
id.tarzanych CreditAttribution: id.tarzanych commentedComment #24
agn507 CreditAttribution: agn507 commentedWe'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
and
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.
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.
Comment #25
czigor CreditAttribution: czigor commentedWe've been using #24 in production for 3 months now. The patch is straightforward and very well explained in #24.
Comment #27
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCommitted #24.