Attached is a patch to add a mapper for the Formatted Number CCK field type. Thanks for your review.

CommentFileSizeAuthor
#15 formatted_number_feeds.zip1.22 KBjoetsuihk
#7 838018-7_formatted_number.patch5.98 KBalex_b
#6 formatted_number.patch5.19 KBinfojunkie
#1 formatted_number.patch1.79 KBinfojunkie
formatted_number.patch1.79 KBinfojunkie

Comments

infojunkie’s picture

infojunkie
Location Vancouver
commented
StatusFileSize
new formatted_number.patch1.79 KB

Just noticed the comment on formatted_number_feeds_set_target was wrong. Re-uploaded.

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

Wouldn't it be easier to just add formatted number into the array of allowed field types in content_feeds_node_processor_targets_alter() ?

I know, not 100 % clean in the spirit of 'on behalf' implementations, but it looks we're copying a lot of code here.

infojunkie’s picture

infojunkie
Location Vancouver
commented

We can't add formatted number handling to mappers/content.inc , since the handling is slightly different. Specifically, we need to call parse_formatted_number when assigning the value, since this is the function that does the magic of converting a formatted number to a decimal.

Maybe it would be good to provide an infrastructure that CCK field mappers can reuse, but that's beyond my scope for now.

alex_b’s picture

alex_b commented
Status: Needs work » Needs review

we need to call parse_formatted_number when assigning the value

Didn't catch that. Will review.

alex_b’s picture

alex_b commented
Title: Support Formatted Number CCK » Mapper for Formatted Number CCK
Status: Needs review » Needs work
Issue tags: +Needs tests

This looks good. Needs minimal test. See tests/feeds_mapper_content.test. You should be able to use content.csv for testing.

infojunkie’s picture

infojunkie
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new formatted_number.patch5.19 KB

Attached is a new version of the patch containing simple tests as well as support for all Formatted Number types, not just decimal.

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 838018-7_formatted_number.patch5.98 KB

Reviewed. All tests passing, this is looking good. Will commit shortly.

alex_b’s picture

alex_b commented
Status: Reviewed & tested by the community » Fixed

This is committed.

alex_b’s picture

alex_b commented

FYI #857928: Breaks installation profiles

alex_b’s picture

alex_b commented

#857928-3: Breaks installation profiles - I'd love if you could way in on this. If the module maintainer does not accept a patch to the broken hook_requirements() implementation in FNCCK, I would rather have this functionality live in FNCCK.

infojunkie’s picture

infojunkie
Location Vancouver
commented

I guess we have the option of:
* Remove this from Feeds and re-submit it to FNCCK
* Remove FNCCK tests

In general, I think it is a big responsibility that Feeds undertakes of including mappers for all 3rd party fields. Another approach might be to open another module, Feeds Mappers, that just provides these mappers. Tests in this module need not be as rigorous as in Feeds core.

However, this should wait until the mapper infrastructure is stabilized, e.g. in reference to the issue of converting them to proper plugins #860748: Config for mappers?.

alex_b’s picture

alex_b commented
Status: Fixed » Needs work

- Remove from Feeds and resubmit to FNCCK

is my preferred way of action.

Tests in this module need not be as rigorous as in Feeds core.

I would not want to maintain that module then :) Srsly: the fact that Feeds integrates with so many modules is the main reason why tests are so incredibly important. The mappers are some of the most test worthy integration points.

alex_b’s picture

alex_b commented

I just removed what I've committed - we are back to #7, sorry :-(

Re: preferred way of action - this is all up to you now. My only restraining factor is that I can't commit the FNCCK mapper to Feeds if it breaks the test profile...

summit’s picture

summit commented

Subscribing, greetings, Martijn

joetsuihk’s picture

joetsuihk commented
StatusFileSize
new formatted_number_feeds.zip1.22 KB

I have moved the code into a module, just like location_feeds module is doing. Feel free to use and test and do anything you want.

As there is not huge interest, i think i will not create a separate d.o project for this tiny module.

Please let me know if there are bugs, thx.

  • alex_b committed 759e58f on 7.x-1.x 
    #838018-12 Remove Formatted Number CCK mapper, cannot be properly tested...
  • alex_b committed dc2655c on 7.x-1.x
    #838018 infojunkie: Mapper for Formatted Number CCK field.
twistor’s picture

twistor commented
Issue summary: View changes
Status: Needs work » Closed (outdated)