With #840626: Support using same mapping target multiple times we've introduced resetting target fields before mapping to them. Unfortunately we reset all possible targets. When we should only reset the ones that we are mapping to.

CommentFileSizeAuthor
#7 853194-7_reset_only_used_targets.patch5.03 KBalex_b
#1 853194-1_reset_only_used_targets.patch1.96 KBalex_b

Comments

alex_b’s picture

alex_b commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new 853194-1_reset_only_used_targets.patch1.96 KB

Patch adjusts reset so that only targets that are actually used in mappings are reset.

This patch also adds uid as target, which would break existing tests as map() resets all mapping targets an the test checks whether a change in author survives an update.

andrewlevine’s picture

andrewlevine commented

Taking a quick look this should be correct. I am going to test this patch on my application when I am done writing some unit tests for it.

andrewlevine’s picture

andrewlevine commented
Status: Needs review » Reviewed & tested by the community

Took a closer look and it looks good. Applied the patch and ran it through the custom unit tests for the site I'm working on (tests if some content using mappings was imported correctly) and that worked as well. RTBC

On a side note, if I updated Feeds to HEAD some 200+ unit tests fail...not sure why

Anonymous’s picture

Anonymous (not verified) commented

Thanks, I was worried about losing the "timestamp", this patch fixed it.

alex_b’s picture

alex_b commented

Thank you so much for the review. I will commit this on the weekend. FYI, I think we found another regression from #840626: #855222: Beta2 node processor overrides 'published' flag default

andrewlevine’s picture

andrewlevine commented

Alex, it is likely the #855222: Beta2 node processor overrides 'published' flag default regression will be solved by this patch, no? As long as the mapping is not set up in the feed.

alex_b’s picture

alex_b commented
StatusFileSize
new 853194-7_reset_only_used_targets.patch5.03 KB

- Adds a test case for the bug reported in #855222.
- Running tests now, if all are passing, I will commit.

On a side note, if I updated Feeds to HEAD some 200+ unit tests fail...not sure why

My bad, I committed a presumably minor fix to the settings UI (remove default setting 'page' for content type in basic settings) which broke a score of tests. This is fixed now.

alex_b’s picture

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

Committed, thank you.

http://drupal.org/cvs?commit=394464

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.