Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StorageExtension is not safe with multiple Gateways #528

Open
davidfuhr opened this issue Apr 19, 2016 · 6 comments · May be fixed by #540
Open

StorageExtension is not safe with multiple Gateways #528

davidfuhr opened this issue Apr 19, 2016 · 6 comments · May be fixed by #540

Comments

@davidfuhr
Copy link
Contributor

There is only one StorageExtension instance per Model which is used in all Gateway instances. The StorageExtension keeps the Action Stack Level internally but this only works reliable when used with only one Gateway Instance. This leads to very unexpected behaviour.

Example

<?php
//example.php

require __DIR__ . '/vendor/autoload.php';

use Payum\Core\PayumBuilder;
use Payum\Core\Payum;
use Payum\Core\Model\Payment;

$paymentClass = Payment::class;

/** @var Payum $payum */
$payum = (new PayumBuilder())
    ->addDefaultStorages()
    ->addGateway('aGateway', [
        'factory' => 'offline',
    ])
    ->addGateway('bGateway', [
        'factory' => 'offline',
    ])
    ->getPayum()
;

header('Content-Type: text/plain');

function getStorageExtension($gateway)
{
    $r = new ReflectionObject($gateway);
    $p = $r->getProperty('extensions');
    $p->setAccessible(true);
    foreach ($p->getValue($gateway) as $extension) {
        if ($extension instanceof \Payum\Core\Extension\StorageExtension) {
            return $extension;
        }
    }
}

$aGateway = $payum->getGateway('aGateway');
$aStorageExtension = getStorageExtension($aGateway);

$bGateway = $payum->getGateway('bGateway');
$bStorageExtension = getStorageExtension($bGateway);

var_dump($aStorageExtension === $bStorageExtension);
// true
@makasim
Copy link
Member

makasim commented Apr 19, 2016

I wish the storage extension was stateless.

@makasim
Copy link
Member

makasim commented Apr 19, 2016

As an idea: we can make use of context to store state

@davidfuhr
Copy link
Contributor Author

Or one StorageExtension instance per Gateway instance

@makasim
Copy link
Member

makasim commented Apr 20, 2016

Or one StorageExtension instance per Gateway instance

This is the way to go, but I personally consider it as workaround as the service still has a state which is bad imho

@makasim makasim linked a pull request Apr 30, 2016 that will close this issue
@makasim
Copy link
Member

makasim commented Apr 30, 2016

Hm, giving it one more thought I do not think there must not be any problems.

The storage extension is only keeping the array of models scheduled for update. And the array is emptied on the last postExecute.

@davidfuhr
Copy link
Contributor Author

For my use case it was be solved with #529 because I had two instances of the same gateway name.

But it might still be an issue if you trigger bGateway in the postExecute of aGateway. Then after the execution of bGateway the storage extension will update all scheduled moduels and changes from aGateway will not be persisted anymore because all scheduled models already got persisted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants