I ran into a couple of issues with the code in existing_entity_id() that made feeds pull duplicates rather than it returning an existing entity id.

The condition for 'id' was $source->id, but this is there no more. There is $source->importer->id();
The test for deciding to return $entity_id was isset(). However DatabaseStatementBase::fetchField() returns FALSE so $entity_id was set.
With this fixed the prepared $query was being reused for the second test. However, the previous condition('guid', $value) was also present.

Patch to fix follows.

CommentFileSizeAuthor
#14 2328605-14-existing_entity_id.patch1.51 KBtwistor
#12 2328605-12-existing_entity_id.patch1.52 KBtwistor
#10 2328605-10-existing_entity_id.patch1.41 KBekes
#6 2328605-06-existingEntityId.patch1.42 KBekes
#1 2328605-01-existingEntityId.patch1.49 KBekes

Comments

ekes’s picture

ekes commented
Status: Active » Needs review
StatusFileSize
new 2328605-01-existingEntityId.patch1.49 KB

Fixes three issues above.

Status: Needs review » Needs work

The last submitted patch, 1: 2328605-01-existingEntityId.patch, failed testing.

ekes’s picture

ekes commented

0 fails, and that patch didn't create one of the 82 exceptions.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Postponed (maintainer needs more info)

Is this still an issue now that a fix for #661606: Support unique targets in mappers has been committed? Either way, the patch provided in #1 doesn't apply anymore.

ekes’s picture

ekes commented

Ah 4 hours is a long time in the feeds queue :-)

I'll test the new version.

ekes’s picture

ekes commented
StatusFileSize
new 2328605-06-existingEntityId.patch1.42 KB

#661606: Support unique targets in mappers fixed the is_null/false logic error.

But the query is still reused despite having conditions added already earlier in the foreach, and it's accessing $source->id rather than $source->importer()->id.

Re-rolled.

ekes’s picture

ekes commented
Status: Postponed (maintainer needs more info) » Needs review
twistor’s picture

twistor commented
Status: Needs review » Needs work

$source->id is still there.

Re-using the query might not be technically correct, but it seems to be working according to the tests. At least, we should clone it rather than rebuilding it.

Can you write a test to demonstrate the problem? I cannot reproduce it.

twistor’s picture

twistor commented

I spoke too soon. I see the problem now with re-using the query.

Re-building the query is fine, since there should be that many unique fields anyway.

Either way, $source->id is still there.

ekes’s picture

ekes commented
StatusFileSize
new 2328605-10-existing_entity_id.patch1.41 KB

> Either way, $source->id is still there.

Quite so, not sure what happened there.

twistor’s picture

twistor commented
Status: Needs work » Needs review
twistor’s picture

twistor commented
StatusFileSize
new 2328605-12-existing_entity_id.patch1.52 KB

How about we move the whole thing down?

Status: Needs review » Needs work

The last submitted patch, 12: 2328605-12-existing_entity_id.patch, failed testing.

twistor’s picture

twistor commented
Status: Needs work » Needs review
StatusFileSize
new 2328605-14-existing_entity_id.patch1.51 KB

blarg.

  • twistor committed 1065324 on 7.x-2.x authored by ekes 
    Issue #2328605 by ekes, twistor: Fixed Unique item checking:...
twistor’s picture

twistor commented
Title: Unique item checking: FeedsProcessor::existing_entity_id() » Unique item checking: FeedsProcessor::existingEntityId()
Status: Needs review » Fixed

Thanks ekes!

I don't think this needed a test since it comes from *very* old code that probably didn't understand how the new db API works.

Status: Fixed » Closed (fixed)

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