magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.55k stars 9.32k forks source link

Copying custom fields from quote to order table using sales_convert_quote in fieldset.xml #5823

Closed srenon closed 6 years ago

srenon commented 8 years ago

Preconditions

  1. Magento 2.1

    Steps to reproduce

  1. Create a custom module that add a field to quote and sales_order table
  2. Update the field in quote table and then place an order

    Expected result

  1. The field value will get copied to the order table

    Actual result

  1. The field does not get copy over to sales_order table

The issue seem to be cause by : /vendor/magento/framework/Api/DataObjectHelper.php

Line 238

$secondObjectArray = $this->objectProcessor->buildOutputDataArray($secondDataObject, $interfaceName);
$this->_setDataValues($firstDataObject, $secondObjectArray, $interfaceName);
return $this;

vendor/magento/framework/Reflection/DataObjectProcessor.php

Line 75

    public function buildOutputDataArray($dataObject, $dataObjectType)
    {
        $methods = $this->methodsMapProcessor->getMethodsMap($dataObjectType);
        $outputData = [];

        /** @var MethodReflection $method */
        foreach (array_keys($methods) as $methodName) {
            if (!$this->methodsMapProcessor->isMethodValidForDataField($dataObjectType, $methodName)) {
                continue;
            }

Since the method for the newly added db field is not defined in "\Magento\Sales\Api\Data\OrderInterface" it will not get copied in to "$secondObjectArray" therefore it will not get set in "$this->_setDataValues()"

MauroNigrele commented 8 years ago

Same for quote_item > order_item

southerncomputer commented 8 years ago

Does not work for Magento 2 like magento 1 - probably due to the 3 separate databases for EE edition.

http://magento.stackexchange.com/questions/77016/copy-custom-data-from-quote-to-order-and-order-item-once-order-is-placed-in-mage?rq=1

Are a few solutions!

MagePsycho commented 8 years ago

Same case observed in Magento 2.1 for quote_item to sales_order_item conversion:

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:DataObject/etc/fieldset.xsd">
    <scope id="global">
        <fieldset id="quote_convert_item">
            <field name="custom_quote_item_field">
                <aspect name="to_order_item" />
            </field>
        </fieldset>
    </scope>
</config>

So I guess listing to observer or using around plugin should be the alternatives for the time-being?

southerncomputer commented 8 years ago

below is the fix for convert quote to order in magento2 else you need to write event to move all filed value from quote to order table magento2a\app\code\Sugarcode\Test\etc\fieldset.xml

<?xml version="1.0"?>
<!--
/**
 * Copyright © 2015 Magento. All rights reserved.
 * See COPYING.txt for license details.
 */
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="../../../../../lib/internal/Magento/Framework/Object/etc/fieldset.xsd">
    <scope id="global">
        <fieldset id="sales_convert_quote">
            <field name="custom_field">
                <aspect name="to_order" />
            </field>
        </fieldset>        
        <fieldset id="quote_convert_item">
            <field name="custom_sku">
                <aspect name="to_order_item" />
            </field>
        </fieldset>         
        <fieldset id="sales_convert_order">
            <field name="custom_field">
                <aspect name="to_quote" />
                <aspect name="to_invoice" />
                <aspect name="to_shipment" />
                <aspect name="to_cm" />
            </field>
        </fieldset>        
        <fieldset id="sales_convert_order_item">
            <field name="custom_sku">
                <aspect name="to_quote_item" />
                <aspect name="to_invoice_item" />
                <aspect name="to_shipment_item" />
                <aspect name="to_cm_item" />
            </field>            
        </fieldset>
    </scope>
</config>

app\code\Magento\Quote\Model\QuoteManagement.php (line no 428 )

before

$billingAddress = $this->quoteAddressToOrderAddress->convert( $quote->getBillingAddress(), [ 'address_type' => 'billing', 'email' => $quote->getCustomerEmail() ] ); in function submitQuote() add below code

$order=$this->quoteAddressToOrder->convert($quote->getBillingAddress(), $orderData); change in app\code\Magento\Quote\Model\Quote\Item\ToOrderItem.php (lin no around 78)

after

$orderItem = $this->orderItemFactory->create(); in function convert() add below code

$this->objectCopyService->copyFieldsetToTarget('quote_convert_item', 'to_order_item', $item, $orderItem); `

it will work fine but 
make sure as i mention above fieldset.xml should be there in you custom module and also you should write event to set product attribute once product is added to cart as like in magento1.x and also you should set custom filed in quote table then only it will copy from quote and item to order and its item

^^ Works!
ram10raj commented 8 years ago

Same for quote> sales_order and quote_address to sales_order_address

southerncomputer commented 8 years ago

@ram10raj did you try the patch code I posted? with fieldset.xml format I also posted? I know for sure that quote->ordre and Quote_teims->order_items works! Have not tried out quote_address to sales_address!

southerncomputer commented 8 years ago

` ---/vendor/magento/module-quote/Model/QuoteManagement.php 2016-08-25 22:54:32.000000000 -0400 +++ vendor/magento/module-quote/Model/QuoteManagement.php 2016-08-31 13:34:30.249250519 -0400 @@ -456,6 +456,7 @@ $order->setShippingAddress($shippingAddress); $order->setShippingMethod($quote->getShippingAddress()->getShippingMethod()); } +$order=$this->quoteAddressToOrder->convert($quote->getBillingAddress(), $orderData); $billingAddress = $this->quoteAddressToOrderAddress->convert( $quote->getBillingAddress(), [

--- /vendor/magento/module-quote/Model/Quote/Item/ToOrderItem.php 2016-08-25 22:54:32.000000000 -0400 +++ vendor/magento/module-quote/Model/Quote/Item/ToOrderItem.php 2016-08-31 3:30:33.138252780 -0400 @@ -75,6 +75,7 @@ }

$orderItem = $this->orderItemFactory->create();

+$this->objectCopyService->copyFieldsetToTarget('quote_convert_item', 'to_order_item', $item, $orderItem); $this->dataObjectHelper->populateWithArray( $orderItem, array_merge($orderItemData, $data),`

wbyrnetx commented 8 years ago

+1 for this issue. We ran into it today.

ilol commented 8 years ago

Registered as MAGETWO-60356

willstorm commented 8 years ago

+1 for this issue.

giacmir commented 7 years ago

Any chance to see this resolved?

hellraiser2005 commented 7 years ago

March 2017 still not working, why not :(

jcconde commented 7 years ago

Yep, I say the same - March 2017 still not working, why not :(

southerncomputer commented 7 years ago

Been using the patch above for past 3-4 months no sweat with huge custom fieldset.xml , orders to quotes, quotes to orders, orders/quotes to edit, quote_items -> sales_order_items , sales_order_items -> quote_items...

ghost commented 7 years ago

The fieldset copy is not working because of the fact that magento 2 is using the repositories objects now.

In case of a transfer to the order object the order repository is used. The order repository has a fixed set of methods and has no magic getter and setter. It only allows you to transfer values to the extension_attributes.

Possible Solutions

  1. Completely remove the fieldset.xml and copy from Magento2. Create a new best practice where the extension attributes is used. Its conform the philosophy of not writing custom data to magento 2 tables

  2. Make a fieldset.xml option to register extension_attributes and transport them. This won't fix the save problem but only wil fix the problem of transporting the value to the repository object

  3. Create a generic observer which will transport the values that are found in the fieldset.xml and not found as a method on the repository.

Code example option 3

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
namespace Magento\Sales\Observer;

use Magento\Framework\Event\ObserverInterface;

/**
 * Sales Observer Model
 *
 * @author Maikel & Derrick
 */
class SalesEventQuoteSubmitBeforeObserver
{
    protected $fieldsetConfig;

    protected $orderInterface;

    public function __construct(
        \Magento\Framework\DataObject\Copy\Config $fieldsetConfig,
        \Magento\Sales\Api\Data\OrderInterface $orderInterface
    ) {
        $this->fieldsetConfig = $fieldsetConfig;
        $this->orderInterface = $orderInterface;
    }

    public function execute(\Magento\Framework\Event\Observer $observer)
    {

        $source = $observer->getEvent()->getQuote();

        $target = $observer->getEvent()->getOrder();

        $this->copyFieldsetToTarget('sales_convert_quote', 'to_order', 'global', $source, $target);

        return $this;
    }

    public function copyFieldsetToTarget($fieldset, $aspect, $root, $source, $target){

        $fields = $this->fieldsetConfig->getFieldset($fieldset, $root);

        $methods = get_class_methods($this->orderInterface);

        foreach($fields as $code => $node){

            $targetCode = (string)$node[$aspect];
            $targetCode = $targetCode == '*' ? $code : $targetCode;

            if(!in_array($this->getMethodName($targetCode),$methods)){
                $target->setData($targetCode, $source->getData($code));
            }

        }
    }

    public function getMethodName($key){
        return 'get' . str_replace(' ', '', ucwords(str_replace('_', ' ', $key)));
    }

}
ishakhsuvarov commented 7 years ago

@dheesbeen Thank you for research. We will look into the options you proposed.

yohanespradono commented 7 years ago

i am also having problem converting quote address custom attribute to order address. so I have to use observer for that.

magento-engcom-team commented 7 years ago

@srenon, thank you for your report. We've created internal ticket(s) MAGETWO-60356 to track progress on the issue.

7ochem commented 6 years ago

So what we have here is the Magento 1 inherited fieldset copy service and the in Magento 2 newly introduced DataObjectHelper method using the interface as a reference.

If we look at the reason why this was implemented in Magento 2 is to protect the integrity of the data object and therefore the database table, am I right?

So if we are going to use the "patch" or the suggested/referenced pull request, then this creates a workaround causing redundant code, an overhead and illogical code: Copying fields through the fieldset copier, then applying Data Object merging (undoing copied custom fields) and then applying the custom field values again...

I guess extension attributes should be the right way, but this leaves the fieldset there, still in the code, while it doesn't work and thus is redundant?

@magento-engcom-team @okorshenko Any progress (internally?) on this? Not only code-wise, but maybe in architecture thoughts/discussions you could share here?

srenon commented 6 years ago

My vote would be to remove all this code (since it does not work) and push developer to use "extension attributes"

orlangur commented 6 years ago

According to discussion in Slack, entity custom fields should not be created in the first place as extension attributes is a preferred approach to extend entities.

Especially it is not a good idea to introduce some magic like https://github.com/magento/magento2/pull/11808/files#r159629003 which tries to copy everything. It is much better to use explicit mechanisms like fieldset.xml.

Since the method for the newly added db field is not defined in "\Magento\Sales\Api\Data\OrderInterface" it will not get copied in to "$secondObjectArray" therefore it will not get set in "$this->_setDataValues()"

Described behavior is perfectly valid as in Magento 2 we prefer fair interfaces.

Thanks all for your inputs!

scottsb commented 6 years ago

That would be a reasonable response if extension attributes weren't complete crap (as currently implemented), requiring you to implement your own persistence mechanism just to store a simple data field!

Furthermore, so long as this fieldset feature is still in the code base, it seems wrongheaded to say that an objective bug in it is acceptable. Are there plans to remove it entirely? If extension attributes (with all their faults) are going to be the way forward, then I agree with @7ochem and @srenon that this fieldset code should be removed entirely.

orlangur commented 6 years ago

Hello @scottsb, thanks for your honest opinion, I wouldn't call current extension attributes implementation quite elegant as well.

it seems wrongheaded to say that an objective bug in it is acceptable

There is no bug, implementation works as it was intended and to copy custom data you'll need some custom code, like http://devdocs.magento.com/guides/v2.1/ext-best-practices/tutorials/copy-fieldsets.html

that this fieldset code should be removed entirely

I believe it can be done at some point, first it should be deprecated and then removed in next release.

One of the motivations behind new ORM (see https://github.com/magento-engcom/msi/blob/develop/lib/internal/Magento/Framework/EntityManager/EntityManager.php#L15) is reduction of needed boilerplate code needed to manage extension attributes. So current extension attributes implementation is not for ever and ever.

scottsb commented 6 years ago

It's good to hear about upcoming ORM functionality. It looks like that's being built as part of MSI, which I haven't otherwise been following since we haven't had the need for MSI as such.

The "objective" bug I was referring to, though, was not in extension attributes. Those aren't buggy: just a half-baked feature. I was referring to the bug reported here in this issue with fieldsets, which I would consider legitimately a bug because it's not deprecated yet but doesn't work as intended (per its original design as inherited from M1).

acamarasan commented 6 years ago

I've managed to copy the values from custom quote attributes by following the official tutorial from here.

On the first try, the code snippet from Step 3 was place in an Observer that tracks sales_model_service_quote_submit_success event, but that didn't work. After I've replaced previous event with sales_model_service_quote_submit_before everything worked as intended.

But still, from my opinion, this functionality should work regardless where and when we choose to use copyFieldsetToTarget method from Magento\Framework\DataObject\Copy.

kanduvisla commented 6 years ago

I wish I could have found this thread earlier. Just wasted a couple of hours figuring out why fieldset.xml was not working as I would expect, tried following the documentation, added extension attributes, but eventually had to add an events.xml to simply getData() from quote and setData() on order.

So basically I can delete my fieldset.xml and extension_attributes.xml, because it no longer applies. FYI: the implementation was done by adding an extra column instead of extension attributes, but now that I see this thread it's clear for my now that when it comes to manipulating existing (data) models, extension attributes are always the way to go.

A typical case of "just because it works doesn't mean it's good".

LucScu commented 5 years ago

@acamarasan So the code in the tutorial step3 has to be inserted on a observer triggered by the sales_model_service_quote_submit_before event? @orlangur Why you don't put this info in the tutorial?

victortodoran commented 5 years ago

According to discussion in Slack, entity custom fields should not be created in the first place as extension attributes is a preferred approach to extend entities.

@orlangur I'm looking at the Magento Certified Professional Developer Plus Exam Study Guide

Question 4 You are working with an OMS that requires you to add an attribute to the order API. How do you add a field to the existing sales API?

Correct Answer B. You add an extension attribute to Magento\Sales\Api\Data\OrderInterface to include the new field

orlangur commented 5 years ago

@victortodoran so? Correct answer is exactly what I said.

victortodoran commented 5 years ago

@orlangur, sorry I understood the opposite for whatever reason.