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.
As mappers become more sophisticated and cover more cases, it is becoming necessary to allow per-importer, per-field configuration of each mapper. For example:
* For date mapper, define the expected input format
* For node reference mapper, define the field used for matching
Is such a feature on the roadmap? If not, what would be the best approach to implement it? Initially, I would think that mappers should be rewritten as plugins - does that make sense?
Comment | File | Size | Author |
---|---|---|---|
#46 | feeds-config-for-mappers-860748-46.patch | 17.84 KB | twistor |
#44 | 860748-per-mapper-config-wtests.patch | 17.7 KB | franz |
#32 | 860748-per-mapper-config-32.patch | 13.76 KB | Niklas Fiekas |
#32 | 860748-per-mapper-config-32-interdiff.txt | 2.94 KB | Niklas Fiekas |
#26 | 860748-config-for-mappers-api.patch | 2.91 KB | Niklas Fiekas |
Comments
Comment #1
Hanno CreditAttribution: Hanno commentedAre the field settings not already managed in the node type?
Comment #2
Hanno CreditAttribution: Hanno commentedComment #3
infojunkieI meant the field import settings, as in the examples above.
Comment #4
TonyK CreditAttribution: TonyK commentedIt definitely should be there. One possible use is to have a setting for cck field mappers to add another value to multi-value field rather than rewrite it. Currently you have to write additional mapper for each field type to accomplish it.
Comment #5
Hanno CreditAttribution: Hanno commentedI see, makes sense. But what about the settings in node type fields:
- default values
- allowed values
- input format
- required
Do we want to have two places to define field handling?
Can we extend the manage fields interface in the node type interface?
Or can we copy some relevant configuration values to the Feeds mapper?
@TonyK in that specific usecase ideally the mapper should check which values are set by this mapper itself in a former session and changed in the source. In that case the value should get replaced, otherwise it should get added.
Comment #6
alex_b CreditAttribution: alex_b commentedinfojunkie - I have no plans to implement a 'heavier' mapping API.
This would be quite some work :-) Do you have a need/the time for tackling it?
Comment #7
infojunkie@alex_b: I don't have the immediate need right now. But given that most of my projects involve Feeds, I wouldn't be surprised that one will require it soon.
Comment #8
alex_b CreditAttribution: alex_b commentedLet's talk when the need comes up.
Comment #9
TonyK CreditAttribution: TonyK commented@Hanno, no I didn't mean two sessions of the same mapper. Imagine a feed with two links in each item and a content type with multi-value CCK link field. When I assign two mappers to one field the latter replaces the former (they are executed in the order they are listed on processor settings page so an ability to reorder them would also be nice). I would like to be able to set the second mapper to add another value to the field, not replace it. It will allow users to make more complex logic without writing their own processors.
Comment #10
Hanno CreditAttribution: Hanno commented@TonyK I see, but that should become the actual behavior. I think the date mapper handles this a simular way (not tested though). When the time element is in another field as the date, the mapper combines the date and time for the date field.
In the same way, multiple values should be stacked and returned as an array when there are multiple source fields for a target and the target field accept multiple values. No configuration needed.
Comment #11
alex_b CreditAttribution: alex_b commented#9 - that behavior should be possible with the current API. It's just the mappers that replace values rather than appending to existing values.
Comment #12
twistor CreditAttribution: twistor commentedAdding configuration to mappers is an interesting idea. I think it would definitely be useful, but a large undertaking. I'd be willing to put in some work if we can come up with a design. It would simplify some of my other projects quite a bit I think.
#10, I agree, it should be the default behavior to append fields rather than overwrite them.
Comment #13
eiriksmAny solutions to this?
i have a feed with multiple values i would want in a multiple value field.
ie.
value1 and value2 should both be added to a field named "somefield", which is a multi-value field. as of now only one value is added. for the record, this is a datefield and it is configured so that the feed matches the field format.
any ideas?
EDIT: I just tried to map to a plain text field, and this works perfectly. all values are added to seperate field values of one multifield. so i am wondering if the issue is in the date module? or is it the date handling in feeds?
Comment #14
NealB-1 CreditAttribution: NealB-1 commentedIt seems to me that configuration for mappers is desirable and will be necessary to do a lot of field types properly.
Ex: There is no single canonical way to import an international phone number. You could decode the country code from the digits, you could infer a country based on a default, or you could get the country some other way. This should be configurable.
I have a need for handling of multi-value fields that's different from the one implemented in Feeds. Changing the default behavior is currently difficult.
Some things that currently require Feeds Tamper, such as mapping taxonomy terms that don't perfectly match the local versions, could be part of the mapper config.
Comment #15
eiriksmAlthough i thought my problem (#13) was related i found out this was related to dates as arrays. If you have the same problem: dig in half a day (like me) and code it yourself or simply see this (duh, it was already an issue)
#697842: Support array of values for dates
Comment #16
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSubscribe.
This would be really useful.
Comment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNow, that I'll need it, I'll spend some time working on it.
The attached patch works already (at least for me), but it is WIP. Please review.
Changes to feeds.api.php aren't included in this version. Basically it adds form_callback and summary_callback to the targets array, so that modules can define their setting forms. The settings will be passes as a fifth parameter to the mapping callback. All the changes should be backwards compatible.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMissed a whitespace error.
Comment #19
twistor CreditAttribution: twistor commentedAwesome! Will be checking out shortly.
Comment #20
colin_young CreditAttribution: colin_young commentedI installed the patch last night (against the dev branch from git). Everything looks like it went okay. I replaced the "supported" feeds with the patched version and ran update, which also appears to have been successful. Feeds is still working, but I don't see anything different in the mappings. I'm assuming I need to write a plugin or something? Is there an example to look at?
Thanks.
Colin
Comment #21
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYes, it is mostly for other modules to extend.
You can see one thing, because I replaced the unique flags with settings forms like that. Both as a demo, because it feels right and because there has to be enough room for the setting forms. Screenshots attached.
Comment #22
colin_young CreditAttribution: colin_young commentedThanks. I'll continue over in #1262732: Map to existing media? then.
Comment #23
colin_young CreditAttribution: colin_young commentedDo you have any details available on implementing your own mappers? I've got some custom taxonomies that have additional fields. I'd like to create a mapper that automatically picks the vocabulary based on the one specified by the node target, and allows you to specify which field in that vocabulary you want to use for the mapping. e.g. I have vocab Stone with Name = 'Diamond', Description = '', Code (custom) = '01' and my incoming feed needs to map '01' to that term.
Thanks.
Comment #24
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTo solve this on a mapping level you can implement
hook_feeds_processor_targets_alter
and expose custom mappers. See feeds/mappers/ for examples.My patch adds a form callback and summary callback property to the processor targets, that can be defined.
Despite that you could - of course - use those properties, this is slightly off topic. Where could something like this be discussed best? groups.drupal.org? Forum? Private mail?
Comment #25
colin_young CreditAttribution: colin_young commentedI'm trying to figure out how to implement the form callback so I can provide a form like you've done with the #1262732: Map to existing media?. I was really trying to get some documentation for this patch started, but seeing as the Feeds documentation is a bit of a WIP, expecting to start some documentation for a WIP patch to feeds might be getting a bit ahead of things.
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAh, ok. The attached patch adds an example to feeds.api.php. Probably we should discuss the API anyway, before reviewing the giant patch for the corresponding UI.
Thank you for trying this and please let me know if it works or how the API could be changed to make this more useful.
Comment #27
colin_young CreditAttribution: colin_young commentedJust wanted to check in and let you know I haven't forgotten about this item. I've been prettying up the site and dealing with requirement changes, but I'm back to the feeds now. I'm just doing a quick and dirty implementation with feeds_tamper, but I'm trying to make it modular enough that it will be easier to port later :)
I've been thinking though that there is potentially some overlap between this patch and feeds_tamper. At the moment, the biggest difference seems to be that feeds_tamper easily allows attaching multiple plugins to each mapping, but at the cost of a less integrated workflow. I wonder if it wouldn't be possible to leverage the feeds_tamper plugins and provide some additional UI and API enhancement to allow attaching multiple existing feeds_tamper plugins to each mapping.
In essence, I'm suggesting that per-field mapping configuration be promoted to a first-class member of feeds, and effectively converting feeds_tamper to a collection of useful plugins for modifying individual mappings.
Comment #28
twistor CreditAttribution: twistor commentedReviewed it and love it!
The first use case that comes to mind is taking something like #724536: Mapper for nodereference field in Drupal 6 where you have a target for GUID, URL, title, nid and making those config options instead.
@Niklas, one critique that I have, albeit minor, is that the form shouldn't be optional_unique or custom, but add the custom form to the optional_unique form when mappings are marked as optionally unique.
@Colin, converting feeds_tamper plugins to use the mapping config would be difficult simply on the basis that there's not enough room. Another problem would be that there's no nice way to form_alter all of the custom forms that would exist. I will be thinking about this much more however.
As far as integration goes, the next version of Feeds will allow a tighter-feel thanks to #1235394: Use more Drupal-y paths.. Feeds Tamper will become a tab on the importer configuration page.
I love that it's backwards compatible, provides enhancement, without needing any changes to supporting modules if they don't care.
Here's hoping this issue goes somewhere!
Comment #29
franzThis is a wonderful feature that will aid us in #1107522: Framework for expected behavior when importing empty/blank values + text field fix. With this, all mapper could include a setting for the empty/blank behavior.
Comment #30
madeby CreditAttribution: madeby commented@franz - this could really help a great lot and make the module so much more useful.
What is the status of all this?
Comment #31
franz@madeby, we just need some more reviewing and testing.
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... sorry. I lost track of this. When talking to twistor in October the plan was that I'd implement the feedback in #28 real quick, but I forgot it and it never happend.
Here's a reroll, solving a merge conflict in the API docs and doing that: The optional unique form will now be merged in, instead of overriden by custom mapping config.
Comment #33
madeby CreditAttribution: madeby commentedApplied the patch but I can't seem to find a way to configure each mapping? How to I access the new features?
Comment #34
franzBy default, I think the only possible configuration available is the "unique" one for 'guid' and other targets. Have you tried to use this mapper?
Comment #35
madeby CreditAttribution: madeby commentedBut wasn't that already there? Nothing new to see. I can set GUID to a unique target just as before.
Comment #36
franz@madeby, that setting was just converted to the new system, which provides an API for other settings as well, and a new UI, similar do Field UI display settings.
Comment #37
franzBTW, I think we need tests for the API. Some common form elements and such.
Comment #38
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYeah. Tests would be awesome. Not for fully automated testing on d.o, until the Feeds testsuite passes, but to locally check everything is fine. Media Feeds (a module that I maintain) uses the upcoming API, too, prematurely. That works.
That said: I am not going to write the tests soon. Unassigning, in case someone else wants to try.
Comment #39
madeby CreditAttribution: madeby commentedI really really need the feature were I can control what happens with empty entries. I would love to test that but lack the coding skills to code it myself.
Comment #40
franzI'll work on a test for this.
Comment #41
franzAs I'm working on a API testing for this, we also need to fix the tests that use the 'unique' value. Also, it's not clear to me what will happen to existing importers when Feeds is updated, we need an update hook for this, or maybe something even sturdier to be backward compatible.
Comment #42
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for attacking this. No update hook required, all the changes are backwards compatible. See #28.
But yes: The UI changed, so some existing tests that use the UI might need to be adjusted.
Comment #43
franzSorry, bad attention on it. I already have a feeds_test.module making use of the API, just need to add a test case now.
Comment #44
franzOk, I got some tests. There is an error when using textarea field, not sure if the value is being trimmed or what. Also, I get a lot of undefined index warnings for the "unique" field, this is worth fixing.
Unassiging to get others to review this.
Comment #45
franzMany features are now depending on this, so I'll increase priority.
Comment #46
twistor CreditAttribution: twistor commentedRe-roll.
Comment #47
twistor CreditAttribution: twistor commentedAlright, I committed this beasty.
http://drupalcode.org/project/feeds.git/commit/17269f88adb6f12075eca2bc9...
Please create new issues for followups. Thanks to all.