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

Comments

infojunkie’s picture

StatusFileSize
new1.79 KB

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

alex_b’s picture

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

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

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

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

Status:Needs work» Needs review
StatusFileSize
new5.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

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new5.98 KB

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

alex_b’s picture

Status:Reviewed & tested by the community» Fixed

This is committed.

alex_b’s picture

alex_b’s picture

#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

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

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

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

Subscribing, greetings, Martijn

joetsuihk’s picture

StatusFileSize
new1.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.