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.48k stars 9.29k forks source link

Why is assignData(..) in Adapter only accessible through observer ? #5587

Closed rikterbeek closed 8 years ago

rikterbeek commented 8 years ago

The new way of using a payment methods is using the Adapter Facade class. Why do you need to add an observer to assignData instead of using Dependency Injection to do this like all other calls in this class ?

joni-jones commented 8 years ago

Hi, @rikterbeek, can you clarify your question? All payment methods, which extends payment Adapter can replace or extend the assignData method. The order management system "directly" calls this method to set payment data. How do you want to use DI to call this assignData method?

rikterbeek commented 8 years ago

@joni-jones,

In the new way you define the payment methods in the di.xml file. I would expect that I can define my assignData logic class in this di.xml file instead of creating a events.xml file. In this file I need to define an observer to set my variables in the additionalData of the payment method received from the front-end. It is much easier to keep the flow that is used for all other methods of the adapter class so you can just define it in one place.

Regards, Rik Adyen

joni-jones commented 8 years ago

The current behaviour for assignData method is based on Magento events, as you can see this method uses event manager and that's why you need to describe observers in events.xml file. This allows extending payments methods by some specific data and base payment module doesn't know about any specific implementations. As I understand, your purpose to move assignData method into separated class or service, which can be configured via di.xml for each payment implementation. Actually, now I can't imagine any use cases, which will get some benefits, but if your integration extends payment method adapter, you can change assignData method to your custom usage.

rikterbeek commented 8 years ago

@joni-jones,

Observers have a downside that you don't have a clear view who is observing this call. I don't understand why assignData() is using observers and all other methods in this class are using dependency injection to declare the behaviour. I found it much more logical if it uses the executeCommand instead. I can extend the Adapter class and change this function but rather be consistent with the core code.

Regards, Rik Adyen

dkvashninbay commented 8 years ago

@rikterbeek Hi,

Introduction

When we were implementing current behaviour for assignData() method, the main idea was to move away from assignData at all, as method implementation depends on a concrete client(Checkout/Quote) implementation and data structure used to perform a transaction.

So the easiest way to substitute assignData() call (which actually means assignDataFromWebCheckoutQuote()) with an event which gets triggered in a concrete client, and has event handlers (if applicable) in payment method implementation.

Why is this needed right now?

MethodInterface (and so Adapter implementation) contains a huge amount of helper methods, which are not required for most of the payments or do no not cover the integration abilities.

Example:

The next step

Define Payment Usage Context infrastructure which will be used by clients to run operations on Payment Methods.

Example:

Summary

rikterbeek commented 8 years ago

Hi @dkvashninbay,

Thanks for this detailed information. I hope it can be moved soon to the DI file so you have a clear overview what files the payment method is using during the checkout process.

Regards, Rik Adyen

joni-jones commented 8 years ago

@rikterbeek, I'm thinking Dmitry posted an exhaustive answer to your question, so I'm close this issue. If you have, any questions, please, fill free to reopen it. UPD you can use Magento2 PHPStorm plugin, which allows navigating for all event observers.