Posted by pcambra on June 15, 2011 at 8:43am
11 followers
| Project: | Commerce Feeds |
| Version: | 7.x-1.x-dev |
| Component: | Miscellaneous |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
With normal mappers like the price one, we have no issues in replacing values, but the product reference one is slightly different because it relates the node (display) with the product id from commerce_product in a sepparate field.
If you want to load product references, the first time is just fine, but for updates you need to send all the product references again for adding a new one, which in some cases could be a pain.
Maybe it would make sense to have two mappers, one that is quicker and straightforward, replaces the field contents and that's it, another one would be "acumulative", and would not delete product ids but add them to the reference field.
I have a proof of concept working, if it makes sense I'll post the code here.
Comments
#1
I don't quite understand this. Can you give an example of how it works?
#2
Subscribing, greetings, Martijn
#3
Example with code:
<?php
$info = field_info_field($target);
list($field_name, $sub_field) = explode(':', $target);
// We get the previous values from the field.
$field = $entity->{$field_name};
$i = isset($field[LANGUAGE_NONE]) ? count($field[LANGUAGE_NONE]) : 0;
// We only check if the number of elements is > 0 because the process is slower.
$check = TRUE;
if ($i == 0) {
$check = FALSE;
}
foreach ($value as $v) {
if (!is_array($v) && !is_object($v)) {
if (strstr($target, 'product_id')) {
$product_id = $v;
}
elseif (strstr($target, 'sku')) {
if ($product = commerce_product_load_by_sku($v)) {
$product_id = $product->product_id;
}
else {
drupal_set_message(t('A product with SKU %sku could not be found. Please check that the product exists or import it first.', array('%sku' => $v)), 'error');
}
}
if ($product_id) {
$delta = $i;
// If there are already some elements in the field, we search for it to reuse them.
if ($check) {
foreach($field[LANGUAGE_NONE] as $key=>$id) {
if ($id['product_id'] == $product_id) {
$delta = $key;
break;
}
}
}
$field[LANGUAGE_NONE][$delta]['product_id'] = $product_id;
}
}
if ($info['cardinality'] == 1) {
break;
}
$i++;
}
$entity->{$field_name} = $field;
?>
#4
I think an accumulative product reference is a really great idea... However, it makes a mess of normal updates, where the update might be coming from an external source and might be done over and over. Would we be closing the door on iterative updates?
#5
You're right, we shouldn't assume that users want to always do update. But I'm also worried about the likely confusion we could create having 2 different mappings for apparently the same thing.
Maybe we could keep one mapper and check the import mode, if we're creating/replacing we run the current mapping code, but when we're updating we run the acumulative one.
<?phpif ($this->config['update_existing'] == FEEDS_UPDATE_EXISTING) {
// Acumulative routines.
}
else {
// REPLACE/CREATE
// Normal way
}
?>
#6
Hey Pedro -
I get an error on my test. I imported products using one Importer, then I created a new Display importer that attempts add several products to a single node (using the *same* product CSV - so one product per line). I used the module you sent me - and I get the following error:
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=9&op=do StatusText: OK ResponseText: ( ! ) Fatal error: Call to undefined function dpm() in /projects/saloncommerce/www/sites/all/modules/commerce_feeds/mappers/commerce_product_reference.inc on line 46 Call Stack #TimeMemoryFunctionLocation 10.0002107288{main}( )../index.php:0 20.385336344280menu_execute_active_handler( )../index.php:21 30.392037682544call_user_func_array ( )../menu.inc:503 40.392137682760system_batch_page( )../menu.inc:0 50.393437682856_batch_page( )../system.admin.inc:2284 60.393737683448_batch_do( )../batch.inc:80 70.393737683448_batch_process( )../batch.inc:161 80.614437830648call_user_func_array ( )../batch.inc:284 90.614537830648feeds_batch( )../batch.inc:0 100.649841581344FeedsSource->import( )../feeds.module:170 110.696241734672FeedsProcessor->process( )../FeedsSource.inc:349 120.720941739600FeedsProcessor->map( )../FeedsProcessor.inc:127 130.737942698392commerce_product_reference_feeds_set_target( )../FeedsProcessor.inc:398Here is the export of my Importer:
$feeds_importer = new stdClass;$feeds_importer->disabled = FALSE; /* Edit this to true to make a default feeds_importer disabled initially */
$feeds_importer->api_version = 1;
$feeds_importer->id = 'pedro_test2_display';
$feeds_importer->config = array(
'name' => 'Pedro test2 Display',
'description' => '',
'fetcher' => array(
'plugin_key' => 'FeedsFileFetcher',
'config' => array(
'allowed_extensions' => 'txt csv tsv xml opml',
'direct' => FALSE,
),
),
'parser' => array(
'plugin_key' => 'FeedsCSVParser',
'config' => array(
'delimiter' => ',',
'no_headers' => 0,
),
),
'processor' => array(
'plugin_key' => 'FeedsNodeProcessor',
'config' => array(
'content_type' => 'product_display',
'expire' => '-1',
'author' => '1',
'mappings' => array(
0 => array(
'source' => 'Model',
'target' => 'guid',
'unique' => 1,
),
1 => array(
'source' => 'Description',
'target' => 'body',
'unique' => FALSE,
),
2 => array(
'source' => 'Model',
'target' => 'title',
'unique' => FALSE,
),
3 => array(
'source' => 'SKU',
'target' => 'field_product:sku',
'unique' => FALSE,
),
),
'update_existing' => '2',
'input_format' => 'plain_text',
),
),
'content_type' => '',
'update' => 0,
'import_period' => '-1',
'expire_period' => 3600,
'import_on_create' => 1,
'process_in_background' => 0,
);
#7
@smokinggoat, he just left a debug (dpm) statement in there. Enable devel module and the error will go away.
#8
Oops sorry about that, thanks Randy!
#9
Yeah! Alright, it works! It properly accumulates the product references!
So Pedro, if you get an updated version with the conditional "accumulate" or "override" function, I'll test both cases. :-)
#10
Ok, here's a patch that considers the update_existing setting for the node processor and if it's set to update it uses the accumulative mapper, and if it's set to create or replace, it uses the usual mapper.
#11
You might want to check this against #996808: Update existing doesn't reset targets that have real_target set.. I think you might depending on behavior that's actually a bug. The $field that's attached to $entity is supposed to have been reset before the mapping starts. The reason for copying it off the entity is to support multiple mappings to the same target.
#12
Ok, reviewed what twistor says (finally) and it seems that our approach for accumulative processor is based in an "unexpected" behavior of Feeds, that seems that will disappear eventually as the majority of use cases are different of this one.
Normally one would expect to pass all the field values all over so you can delete references, i.e.
$node->field_values = array(1,2,3)
$node->field_values = array(1,2,4)
Will end with values: 1, 2 and 4
But we're dealing with an use case where we'll end with 1,2,3 and 4.
Probably we'll want to implement a FeedsProductDisplay plugin on top of FeedsProcessor to add a fourth option: add, replace, update and accumulate which will behave as we expect.
Setting back to needs work.
#13
Couldn't you just set real_target to a non-existent attribute? Feeds would never reset it, and you would get the behavior you're looking for.
#14
Great idea twistor, thanks!
Here's a patch that changes the real target so that it's never reseted and just for the use case of updating (not replacing) it keeps the old records.
#15
Let's commit this!
Thanks to everyone :)
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
Hi, sorry to ask, but is this commiitted?
greetings, Martijn
#18
It was committed to -dev version on Jan 13 as you can see in #15
#19
Hi, Thanks for telling! Greetings, Martijn
#20
Hello, I'm trying to understand whether my 'use case' is the same as the one solved with this patch or what changes I would need to make to have it work for my case.
I am understanding the idea behind accumulating values for the product references which is perfect but does this method of accumulate target this value specifically or effect all possible fields?
For example, if you have a product that comes in Small, Medium, and Large but also have other attributes.. say color is one. Would this method allow to accumulate for the unique value but update for non-unique attributes?
#21
This mapper option is for acumulating the product reference field in the product display, exactly as you can see in #12
This mapper option has anything to do with the product import itself but with the relationship between product display and product.
#22
There is a problem with the accumulate in the case that in the import file some SKU's are removed from the product display.
In this case these product stay referenced.
#23
Please don't revive issues that have been committed, open a new one instead