Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. #9306

dverkade · 2017-04-19T07:42:53Z

Fixed coding standard violation for an interface which was not ending the name with "Interface". In order to let CI check this file everytime changed the following:

Removed the @codingstandardsIgnoreFile from the head of the file.
Renamed the file and the name of the interface to PaymentTokenFactoryInterface so that it ends with the name "Interface"
Refactored references to this class in other Magento modules so that this name change is reflected there as well.

… the name with "Interface". In order to let CI check this file everytime changed the following: - Removed the @codingstandardsIgnoreFile from the head of the file. - Renamed the file and the name of the interface to PaymentTokenFactoryInterface so that it ends with the name "Interface" - Refactored references to this class in other Magento modules so that this name change is reflected there as well.

dverkade · 2017-04-19T08:08:15Z

@magento-team For this code violation fix I changed the name of a interface. Please look into this, since I think this might introduce a breaking change and tell me how you want to deal with this.

A possible solution I can think of is to keep the old class name and let it extend to the refactored new class name. We can add a "deprecated" message in the old class name, so that it can eventually be removed.

Please let me know your thoughts so I can change this in order to get this accepted.

ishakhsuvarov · 2017-04-19T08:13:31Z

Hi @dverkade

A possible solution I can think of is to keep the old class name and let it extend to the refactored new class name. We can add a "deprecated" message in the old class name, so that it can eventually be removed.

This is pretty much the way we tend to deal with these issues at the moment. Would be nice if you could update your PR in the proposed way and we will look into it.
Thanks!

…s the new interface name.

- Reverted refactored changed class names for the Abstract class, which should not have been changed in the first place.

dverkade · 2017-04-19T09:26:36Z

Hi @ishakhsuvarov,

I checked this again and made some changes (refactored to much code in the first place) and added the deprecated file which I suggested. So this PR is complete from my side. Hope you can have a look at it.

ishakhsuvarov · 2017-04-19T09:28:44Z

@dverkade Thanks for the update. We are in the process of discussion of the proposed changes, since there are some concerns regarding the interface in question. I will provide an update for you when we come up with some ideas.

maghamed · 2017-04-20T17:04:49Z

Hey @dverkade ,

Making naming consistency along the code base is very important for us, but we can't introduce changes for API very often. That’s why I want you to ask to accompany your changes with refactoring of the PaymentTokenFactoryInterface, because this contract is far from ideal, and does not represent ideal state dictated by our Technical Vision. Which I'll describe below.
Because we don’t want to introduce changes to API interfaces often (like change the same interface in each next release). The less changes introduced for API – the less 3rd party devs are affected and need to update their code. That’s why if we change API interface it should be changed to the ideal state dictated by Technical Vision.

The current problems I see are next:

Usage of Inheritance Based API for AbstractPaymentTokenFactory and its successors
Violation of Single Responsibility principle by PaymentTokenFactoryInterface which contains two methods create() and getType(). And getType has service purposes only (aka protected contract), it's not used by business logic from outside, but instead used for setting correct type for Model\PaymentToken during the instantiation

magento2/app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

Line 40 in 99e85cb

$paymentToken->setType($this->getType());
This code

        $paymentToken = $this->objectManager->create(PaymentTokenInterface::class);
        $paymentToken->setType($this->getType());

also has a "smell" , because in our case instance of Model\PaymentToken doesn't have sense without setting type. But we could potentially create Model\PaymentToken object not specifying one. If we will inject dependency on Model\PaymentToken in Constructor DI of some other object.

So, what I propose to do:

Let’s accept $type as a constructor parameter and set it within constructor method of Model\PaymentToken, to make sure that our object is well formed and valid just after instantiation.
Let's introduce new interface

<?php
/**
 * Copyright © 2013-2017 Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
namespace Magento\Vault\Api\Data;

/**
 * Interface PaymentTokenFactoryInterface
 */
interface PaymentTokenFactoryInterface
{ 
    /**
     * Create payment token entity
     * @param $type account|card
     * @return PaymentTokenInterface
     */
    public function create($type);
}

which will have correct factory contract. Without getType() method, and create method which accepts type as parameter.

Old implementation will proxy call to new one like this:

/**
 * Class CreditCardTokenFactory
 * @api
 * @deprecated 
 * @see PaymentTokenFactoryInterface
 */
class CreditCardTokenFactory extends AbstractPaymentTokenFactory
{
    /**
     * @var string
     */
    const TOKEN_TYPE_CREDIT_CARD = 'card';


    public function __construct(
        ObjectManagerInterface $objectManager
        PaymentTokenFactoryInterface $newTokenFactory = null
    ) {
        $this->objectManager = $objectManager;
        $this->tokenFactory = $newTokenFactory ?: \Magento\Framework\App\ObjectManager::getInstance()
            ->get(PaymentTokenFactoryInterface::class);
    }

    /**
     * Create payment token entity
     * @return PaymentTokenInterface
     */
    public function create()
    {
        return $this->tokenFactory->create($this->getType());
    }
    /**
     * @inheritdoc
     */
    public function getType()
    {
        return self::TOKEN_TYPE_CREDIT_CARD;
    }
}

Doing all of the above, we wouldn't introduce Backward Incompatible changes, and will introduce useful refactoring which moves our Vault Token Factory interface to the desired vision. So, all the 3rd party devs will need to make an upgrade just once.

Hope that makes sense for you.

dverkade · 2017-04-20T18:32:05Z

Hi @maghamed,

Thanks for your information and have checked the solution you are suggesting. However I do see a problem with this. In your example you have added a "create" function to the CreditCardTokenFactory class. This class extends the AbstractPaymentTokenFactory class which in its turn implements the PaymentTokenFactoryInterface. The interface has changed and it's now manditory to submit the $type parameter to it, so the proxy create function you are suggestion in the CreditCardTokenFactory will not be compatible with the interface it needs to implement. This will not work and throws an error since the create function is incompatible.

How about we change the behaviour of the AbstractPaymentTokenFactory class to something like this, where we have a fallback on the type argument. It can be null, when it is, it will call the $this->getType() function, otherwise it will pass the argument given to it.

abstract class AbstractPaymentTokenFactory implements PaymentTokenFactoryInterface
{
    /**
     * Create payment token entity
     * @return PaymentTokenInterface
     */
    public function create($type = null)
    {
        /** @var PaymentTokenInterface $paymentToken */
        $paymentToken = $this->objectManager->create(PaymentTokenInterface::class, ($type) ? $type : $this->getType());
        return $paymentToken;
    }
}

maghamed · 2017-04-21T12:14:50Z

@dverkade

In your example you have added a "create" function to the CreditCardTokenFactory class.

No, I didn't add "create" function to CreditCardTokenFactory. Function create already there, it was inherited from AbstractPaymentTokenFactory with next implementation.

    /**
     * Create payment token entity
     * @return PaymentTokenInterface
     */
    public function create()
    {
        /** @var PaymentTokenInterface $paymentToken */
        $paymentToken = $this->objectManager->create(PaymentTokenInterface::class);
        $paymentToken->setType($this->getType());
        return $paymentToken;
    }

So, CreditCardTokenFactory already has a contract of public function create()
All I did is overwritten default implementation, providing new one which will proxy the call to new interface PaymentTokenFactoryInterface::create($type)

So, we not changing existing interface

interface PaymentTokenInterfaceFactory
{
    /**
     * Create payment token entity
     * @return PaymentTokenInterface
     */
    public function create();

    /**
     * Return type of payment token
     * @return string
     */
    public function getType();
}

which we consider as deprecated.
But make its implementation work through the newly created interface.

/**
 * Interface PaymentTokenFactoryInterface
 */
interface PaymentTokenFactoryInterface
{ 
    /**
     * Create payment token entity
     * @param $type account|card
     * @return PaymentTokenInterface
     */
    public function create($type);
}

Regarding changes proposed by you

abstract class AbstractPaymentTokenFactory implements PaymentTokenFactoryInterface
{
    /**
     * Create payment token entity
     * @return PaymentTokenInterface
     */
    public function create($type = null)

we can't change contract of the AbstractPaymentTokenFactory , because that will introduce backward incompatible changes. Which is forbidden.

Does it make sense for you?

maghamed · 2017-04-24T11:03:14Z

Hi @dverkade , will you help us to improve existing APIs of Vault module based on the output above? That would be very valuable for M2 and improve code quality of Magento Service Contracts.

dverkade · 2017-04-24T11:10:24Z

Hi @maghamed

Yes, I would like to update this PR and get is accepted. But i'm a little confused. You are mentioning that the CreditCardTokenFactory already has a create function. But this is not in de develop branch i'm look in. Is this something that is in another branch already?

Hope you can point me in the right direction so we can solve this issue.

maghamed · 2017-04-24T12:42:04Z

In the Develop branch, CreditCardTokenFactory is extended from AbstractPaymentTokenFactory

magento2/app/code/Magento/Vault/Model/CreditCardTokenFactory.php

Line 12 in 99e85cb

class CreditCardTokenFactory extends AbstractPaymentTokenFactory

AbstractPaymentTokenFactory has create() method

magento2/app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

Line 36 in 99e85cb

public function create()

This method also part of the PaymentTokenInterfaceFactory interface
https://github.com/magento/magento2/blob/99e85cbc45223baa3551e4c534b650c0d2c6358b/app/code/Magento/Vault/Api/Data/PaymentTokenInterfaceFactory.php

Thus, CreditCardTokenFactory class already has a method create() which it inherits from the AbstractPaymentTokenFactory.

Does it make sense?

dverkade · 2017-04-25T12:21:04Z

Hi @maghamed,

Thank you for your anwer. I will look into this over the weekend.

dverkade · 2017-05-01T06:55:17Z

Hi @maghamed,

I see you are at the Meet Magento NL contribution day as well. I'll be there, so let's discuss this PR and #9407 at the event, so I can contribute to this.

- Deprecated AbstractPaymentTokenFactory, CreditCardTokenFactory andd AccountPaymentTokenFactory - Created proxy call for creating the PaymentToken truth the new PaymentTokenFactoryInterface and classes.

…ctory

…enInterfaceFactory - Fixed coding standards in AbstractPaymentTokenFactory

maghamed · 2017-05-13T12:22:08Z

app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

@@ -11,7 +11,8 @@

 /**
 * Class AbstractPaymentTokenFactory
- * @api
+ * @deprecated
+ * @see PaymentTokenFactory


@ see PaymentTokenFactoryInterface

maghamed · 2017-05-13T12:22:57Z

app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

     * AccountPaymentTokenFactory constructor.
     * @param ObjectManagerInterface $objectManager
     */
-    public function __construct(ObjectManagerInterface $objectManager)
+    public function __construct(ObjectManagerInterface $objectManager, PaymentTokenFactoryInterface $paymentTokenFactory)


PaymentTokenFactoryInterface $paymentTokenFactory should be optional parameter
PaymentTokenFactoryInterface $paymentTokenFactory = null

to preserve backward compatibility

maghamed · 2017-05-13T12:24:50Z

app/code/Magento/Vault/Model/AccountPaymentTokenFactory.php

@@ -7,7 +7,8 @@

 /**
 * Class AccountPaymentTokenFactory
- * @api
+ * @deprecated
+ * @see PaymentTokenFactory


@ see PaymentTokenFactoryInterface

maghamed · 2017-05-13T12:25:03Z

app/code/Magento/Vault/Model/CreditCardTokenFactory.php

@@ -7,7 +7,8 @@

 /**
 * Class CreditCardTokenFactory
- * @api
+ * @deprecated
+ * @see PaymentTokenFactory


@ see PaymentTokenFactoryInterface

maghamed · 2017-05-13T12:26:36Z

app/code/Magento/Vault/Model/PaymentTokenFactory.php

+use Magento\Vault\Api\Data\PaymentTokenFactoryInterface;
+
+/**
+ * Interface PaymentTokenFactory


please provide a precise description.
as it's not an interface

…e given type is not found.

- Refactored PaymentToken to use data array instead of own variable.

ishakhsuvarov · 2017-06-06T13:45:16Z

Hi @dverkade
Do you wish to continue working on this PR or should we close it?
Thanks

dverkade · 2017-06-06T13:48:53Z

Hi @ishakhsuvarov

I did not get a reply on this issue where I'm mentioning the failing tests:
#9407

When I get an answer I can work on this PR again

…ty reasons.

…eate function.

dverkade · 2017-06-12T21:04:30Z

Hi @ishakhsuvarov @maghamed,

I have finished this PR and tests are now passing with the latest changes made.

ishakhsuvarov · 2017-06-13T06:59:59Z

@dverkade
Thanks. We will proceed with the review shortly

maghamed · 2017-06-14T10:54:41Z

app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

+        ObjectManagerInterface $objectManager,
+        PaymentTokenFactoryInterface $paymentTokenFactory = null
+    ) {
+        if (!$paymentTokenFactory) {


please use === null instead

maghamed · 2017-06-14T10:55:54Z

app/code/Magento/Vault/Model/AbstractPaymentTokenFactory.php

+    /**
+     * @return string
+     */
+    abstract public function getType();


no need to specify abstract public getType method here
we get it from PaymentTokenInterfaceFactory

interface PaymentTokenInterfaceFactory { /** * Create payment token entity * @return PaymentTokenInterface */ public function create(); /** * Return type of payment token * @return string */ public function getType(); }

maghamed · 2017-06-14T11:00:20Z

app/code/Magento/Vault/Api/Data/PaymentTokenFactoryInterface.php

+
+    /**
+     * Create payment token entity
+     * @param $type string


please specify Null as a possible input value

maghamed · 2017-06-14T11:00:51Z

app/code/Magento/Vault/Model/PaymentTokenFactory.php

+     * @param ObjectManagerInterface $objectManager
+     * @param array $tokenTypes
+     */
+    public function __construct(ObjectManagerInterface $objectManager, $tokenTypes = [])


please specify
array for $tokenTypes

I don't know what you mean here. array has been definied in the doc comments and is specified in the param as default value was well. What and how do you want it to be described?

I mean explicitly specify type hint for $tokenTypes
like
array $tokenTypes = []

- Changed check to === null - Added null to doc comments for $type parameter.

maghamed

Good job, well done!
Thanks for contribution

maghamed · 2017-06-14T13:15:34Z

Btw @dverkade you made a PR which we accepted and will deliver to mainline soon.
This PR introduced a valuable refactoring for our Core Payment APIs.
And now Payment APIs are closer to the desired state than they were before your contribution.

Maybe you have some time to write a blog post regarding this Pull Request and share with Magento community the experience you gained. How it's complicated to write APIs in a backward compatible way, and that it's possible to do so, not breaking anybody. Not a lot of contributors introduce changes for Magento APIs and you are one of those.

I believe your experience would be helpful for many Mangeto contributors.

…ce" at the end of the name. #9306

dverkade changed the title ~~Fix PaymentTokenFactory interface to have the "Interface" name at the end of the name.~~ Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. Apr 19, 2017

ishakhsuvarov added develop improvement Progress: needs update labels Apr 19, 2017

dverkade added 3 commits April 19, 2017 10:17

Added the old interface name with a @deprecated message, which extend…

d2ea4c1

…s the new interface name.

- Added @codingStandardsIgnoreFile to the old interface file

71468a3

- Reverted refactored changed class names for the Abstract class, which should not have been changed in the first place.

- Added @codingStandardsIgnoreFile to the old interface file

1ec515c

- Reverted refactored changed class names for the Abstract class, which should not have been changed in the first place.

maghamed self-requested a review April 21, 2017 09:27

maghamed self-assigned this Apr 21, 2017

maghamed mentioned this pull request Apr 26, 2017

Refactor PaymentTokenFactory interface in Vault\API #9407

Closed

dverkade added 4 commits May 13, 2017 13:42

Added new PaymentTokenFactory logic

4f55988

- Reverted PaymentTokenInterfaceFactory to original class.

dfe690c

- Deprecated AbstractPaymentTokenFactory, CreditCardTokenFactory andd AccountPaymentTokenFactory - Created proxy call for creating the PaymentToken truth the new PaymentTokenFactoryInterface and classes.

Changed to use the correct namespace for the PaymentTokenInterfaceaFa…

f0711c2

…ctory

- Reverted changes in other modules so it will use the old PaymentTok…

84e668a

…enInterfaceFactory - Fixed coding standards in AbstractPaymentTokenFactory

maghamed suggested changes May 13, 2017

View reviewed changes

dverkade added 2 commits May 13, 2017 14:44

Added constructor to the PaymentToken class to set the type.

716b7f8

Fixed code review items.

e3f2800

dverkade added 8 commits May 13, 2017 14:56

Fixed coding standards for PaymentToken class

0760010

Added using the PaymentTokenFactoryInterface

f1c42fd

Added checking token types to the factory. Will throw a error when th…

3ef7f92

…e given type is not found.

Changed constructor

94fc33e

Changed constructor

c036be8

Changed variable to correct one in PaymentTokenFactory

7f4dbe0

- Fixed Unit tests

85a6c71

- Refactored PaymentToken to use data array instead of own variable.

Fixed coding standard violations

bea1a3c

dverkade added 3 commits June 9, 2017 14:46

Added code to the PaymentTokenFactory class for backwards compatibili…

e6dea5b

…ty reasons.

When an array is supplied, should go straight to the objectmanager cr…

cdf43f3

…eate function.

Fixed coding standard violation

f5eaf7a

ishakhsuvarov self-assigned this Jun 13, 2017

maghamed reviewed Jun 14, 2017

View reviewed changes

dverkade added 2 commits June 14, 2017 13:14

- Removed abstract method

57c8974

- Changed check to === null - Added null to doc comments for $type parameter.

Added specific type hint for the tokenTypes parameter

eaf3b04

ishakhsuvarov added this to the June 2017 milestone Jun 14, 2017

maghamed approved these changes Jun 14, 2017

View reviewed changes

ishakhsuvarov added Progress: accept and removed Progress: needs update labels Jun 14, 2017

magento-team merged commit eaf3b04 into magento:develop Jun 15, 2017

magento-team pushed a commit that referenced this pull request Jun 15, 2017

MAGETWO-69879: Fix PaymentTokenFactory interface to have the "Interfa…

e120d8a

…ce" at the end of the name. #9306

dverkade deleted the Fix_PaymentTokenFactory_interface_not_ending_class_name_with_Interface branch June 22, 2018 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. #9306

Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. #9306

dverkade commented Apr 19, 2017 •

edited by ishakhsuvarov

Loading

dverkade commented Apr 19, 2017 •

edited

Loading

ishakhsuvarov commented Apr 19, 2017

dverkade commented Apr 19, 2017 •

edited

Loading

ishakhsuvarov commented Apr 19, 2017

maghamed commented Apr 20, 2017 •

edited

Loading

dverkade commented Apr 20, 2017 •

edited

Loading

maghamed commented Apr 21, 2017 •

edited

Loading

maghamed commented Apr 24, 2017

dverkade commented Apr 24, 2017

maghamed commented Apr 24, 2017

dverkade commented Apr 25, 2017

dverkade commented May 1, 2017

maghamed May 13, 2017

maghamed May 13, 2017

maghamed May 13, 2017

maghamed May 13, 2017

maghamed May 13, 2017

ishakhsuvarov commented Jun 6, 2017

dverkade commented Jun 6, 2017

dverkade commented Jun 12, 2017

ishakhsuvarov commented Jun 13, 2017

maghamed Jun 14, 2017

maghamed Jun 14, 2017

maghamed Jun 14, 2017

maghamed Jun 14, 2017

dverkade Jun 14, 2017 •

edited

Loading

maghamed Jun 14, 2017

maghamed left a comment

maghamed commented Jun 14, 2017

Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. #9306

Fix PaymentTokenFactory interface to have the "Interface" at the end of the name. #9306

Conversation

dverkade commented Apr 19, 2017 • edited by ishakhsuvarov Loading

dverkade commented Apr 19, 2017 • edited Loading

ishakhsuvarov commented Apr 19, 2017

dverkade commented Apr 19, 2017 • edited Loading

ishakhsuvarov commented Apr 19, 2017

maghamed commented Apr 20, 2017 • edited Loading

dverkade commented Apr 20, 2017 • edited Loading

maghamed commented Apr 21, 2017 • edited Loading

maghamed commented Apr 24, 2017

dverkade commented Apr 24, 2017

maghamed commented Apr 24, 2017

dverkade commented Apr 25, 2017

dverkade commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishakhsuvarov commented Jun 6, 2017

dverkade commented Jun 6, 2017

dverkade commented Jun 12, 2017

ishakhsuvarov commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dverkade Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maghamed left a comment

Choose a reason for hiding this comment

maghamed commented Jun 14, 2017

dverkade commented Apr 19, 2017 •

edited by ishakhsuvarov

Loading

dverkade commented Apr 19, 2017 •

edited

Loading

dverkade commented Apr 19, 2017 •

edited

Loading

maghamed commented Apr 20, 2017 •

edited

Loading

dverkade commented Apr 20, 2017 •

edited

Loading

maghamed commented Apr 21, 2017 •

edited

Loading

dverkade Jun 14, 2017 •

edited

Loading