Propel storage bridge

[jkabat]

No description provided.

[makasim]

I'd prefer to move it to constructor, even better if you allow a developer to pass it as a second argument. query could be passed same,

They could optional and user do not provide them then you can build it in constructor.

[makasim]

Looking forward to merge it, Tests and docs are left....

[makasim]

by the way is it Propel 1 or 2?

[jkabat]

@makasim I'm working with 1.7 propel branch, hopefully it will get tested by 2.0 branch as well.

[makasim]

Does 2.0 propel backward compatible?

TODO: Add a storage factory to symfony bundle,just a note for me anybody else who want to do it.

[jkabat]

@makasim I'm not sure, I've never had chance to use 2.0 in any project. As soon as it will be stable, I'l probably switch it in projects.

[lunika]

Hi,

nice job.

Unfortunately this PR will not work with Propel 2. Peer classes have been removed for exemple and with this PR more BC break will be introduced

[jkabat]

I've created a few tests based on @liamsorsby file and have a few questions.

1. should PropelBundle be in dependencies for test suite?
2. because propel is not using any object manager, it is almost impossible to mock classes instantiated inside storage methods, following options are available:
   • zend extension for overloading: https://github.com/krakjoe/uopz
   • testmodel files generated by propel generator in fixtures folder
   • individual testmodel class containing necessary methods invoked by storage

[jkabat]

@lunika thx for info.
@makasim can we have separate storages for propel v1 and v2?

[makasim]

@makasim can we have separate storages for propel v1 and v2?

yes, sure, I am just thinking about the names. Maybe it is better to call this storage as Propel1Storage. wdyt?

[makasim]

should PropelBundle be in dependencies for test suite?

I do not think you need a bundle, the propel lib would be enough.

[makasim]

individual testmodel class containing necessary methods invoked by storage

This approach looks good to me.

[jkabat]

@makasim we have to talk about storage usage in payum bundle.

As I said here #144 (comment) I'm having trouble to configure it in working environment. Both models (token + order) writes to DB correctly, but request ends up with an error "Request Capture{model: Order} is not supported."

I'm sure there is a problem in configuration as shown in gists, but I have no clue how to test.

[jkabat]

Ok, I just found I'm missing interface for Order model (BaseOrder is generated by propel1).

class Order extends BaseOrder implements OrderInterface

Now I'm getting "Request GetHumanStatus{model: NULL} is not supported." exception. Seems like I'm still missing one piece.

Stack trace:

DEBUG - time: 0.000 sec | mem: 13.2 MB | connection: default | SELECT `hash`, `details`, `afterUrl`, `targetUrl`, `paymentName` FROM `Token` WHERE `hash` = '4nb-ggNeHR4yEYSykbrr6r26NB4tBkJy5kVISuTpJOo' 
DEBUG - [Payum] 1# Payum\Core\Action\ExecuteSameRequestWithModelDetailsAction::execute(Capture{model: Token}) 
DEBUG - time: 0.000 sec | mem: 16.0 MB | connection: default | SELECT `number`, `description`, `clientEmail`, `clientId`, `totalAmount`, `currencyCode`, `currencyDigitsAfterDecimalPoint`, `details`, `id` FROM `Order` WHERE `id` = 14 
DEBUG - [Payum] 2# Payum\Core\Action\CaptureOrderAction::execute(Capture{model: Order}) 
DEBUG - [Payum] 3# Payum\Core\Action\ExecuteSameRequestWithModelDetailsAction::execute(GetHumanStatus{model: Order}) 
DEBUG - [Payum] 4# Payment::execute(GetHumanStatus{model: NULL}) throws exception RequestNotSupportedException 
DEBUG - [Payum] 3# ExecuteSameRequestWithModelDetailsAction::execute(GetHumanStatus{model: NULL}) throws exception RequestNotSupportedException 
DEBUG - [Payum] 2# CaptureOrderAction::execute(Capture{model: Order}) throws exception RequestNotSupportedException 
DEBUG - [Payum] 1# ExecuteSameRequestWithModelDetailsAction::execute(Capture{model: Order}) throws exception RequestNotSupportedException 
CRITICAL - Uncaught PHP Exception Payum\Core\Exception\RequestNotSupportedException: "Request GetHumanStatus{model: NULL} is not supported." at \Payum\Core\Exception\RequestNotSupportedException.php line 29 

[makasim]

Seems like order->getDetails() returns NULL but it must be an array

[jkabat]

Yes, that is right. Order details are not saved. Token details are saved successfully.

[liamsorsby]

What is being used in the app/config file for storage as I keep getting Unrecognized option "propel" under "payum.security.token_storage.Acme\DemoBundle\Model\Token" or have I jumped a few steps?

[makasim]

@liamsorsby I believe you mess things up. The storage we are discussing here is not related to one in the bundle config. The config one has to be address separately after we are done with this storage

One we have this storage we can create a storage factory in bundle to simplify its creation. Somethin like doctrine: https://github.com/Payum/PayumBundle/blob/master/DependencyInjection/Factory/Storage/DoctrineStorageFactory.php

[liamsorsby]

@makasim So I'm right in thinking I've skipped the step that you are working on and Instead of working on the Payum bundle for symfony, I should be working on the Payum bundle and getting that to work without any framework? Is that correct?

[makasim]

I should be working on the Payum bundle and getting that to work without any framework? Is that correct?

This PR is about a storage that could be reused in plain php or with any other frameworks. In PayumBundle you can use with custom storage factory. like this:

payum:
    security:
        token_storage:
            AppBundle\Model\Token:
                custom: appbundle.payum.storage.propel.token

    storages:
        AppBundle\Model\Order:
            custom: appbundle.payum.storage.propel.order

    payments:
        offline:
            offline: ~

services:
    appbundle.payum.storage.propel.order:
        class: Payum\Core\Bridge\Propel\Storage\Propel1Storage
        arguments: ["AppBundle\\Model\\Order"]

    appbundle.payum.storage.propel.token:
        class: Payum\Core\Bridge\Propel\Storage\Propel1Storage
        arguments: ["AppBundle\\Model\\Token"]

Later we may add a specific factory, like doctrine one.

[makasim]

is it really not supported? It is hard to implement such method for filesystem storage but should be possible for propel.

(we can do it as separate PR)

[liamsorsby]

@makasim Criteria can be done but tends to be done using the Peer class which @lunika stated was removed in propel2.

[liamsorsby]

@jkabat I wil check it out tomorrow, the bug he has pointed out may have been the issue that I was encountering when testing a payment. However, with the current it was the token I was struggling to set. Also, is the storage going to be different for propel 2? As the current storage uses peer

[jkabat]

There will be separate Storage for Propel2. This one is named Propel1Storage.

[jkabat]

It's part of the propel1 library.

[makasim]

@jkabat thanks.

[makasim]

@jkabat Could you please add a short usage guide here (like one for Doctrine) https://github.com/Payum/Payum/blob/master/src/Payum/Core/Resources/docs/storages.md#doctrine-orm

[lionelbzv]

I'me just discovering this bundle and glad to see that a Propel version has just been merged!

@jkabat @liamsorsby could you confirm that the propel version is ok - because you seem to talk about a bug?

[jkabat]

@makasim Ill do that as soon as Ill get some spare time.
@StudioEcho Propel version is OK, currently only v1.7 is supported, but maybe someone who is using 2.0 branch could do standalone integration.

[liamsorsby]

@jkabat @makasim I've just got onto using the bundle now. However, other than the doc's is it okay to make a pull request on the PayumBundle for symfony to add Propel into the Bundle or is there some more work needed to be done on the core bundle first?

[makasim]

The storages (for Propel1\2 were already added). You just have to add a bundle specific factory to make it work in the bundle.

[makasim]

Like this https://github.com/Payum/PayumBundle/tree/master/DependencyInjection/Factory/Storage

[liamsorsby]

@makasim Should I make a pull request and add the propel specific storage onto the bundle?

[makasim]

Would be great If you have time.

[liamsorsby]

@makasim I'll get onto doing it probably tomorrow now but I'll get it done by the end of the week (hopefully)

[liamsorsby]

@makasim Sorry for the delay! Just working on this now, does it need two storage implementations for propel1 and propel2? Or can it use a similar method to doctrine with the dynamic driver in the addConfigurations?

[makasim]

Yes, it is. It should be two separate factories.

Or can it use a similar method to doctrine with the dynamic driver in the addConfigurations?

sorry dont get this question, Are you asking about ability to extend abstract storage, if it helps you can do it.

[liamsorsby]

@makasim Sorry, my question wasn't overly clear. Thats all I needed with regards to clarification on being two seperate factories. I've got to a point now where I've implemented the model and the extension but I seem to be getting this error:

Unrecognized options "storage_dir", id_property" under "payum.security.token_storage.Acme\DemoBundle\Model\Token.propel1" any thoughts on what this could be just to push me in the right direction.

[makasim]

Hm storage_dir is a config option for filesystem storage. It could be possible that configs were merged or you leaved those options somewhere in your yaml file.

[liamsorsby]

So to clarify these should not be needed at all as the 1st parameter should be the modelClass. Would that be correct?

[makasim]

yeap, and maybe some options which propel may require

[liamsorsby]

@makasim Last question, I've pretty much got everything together, however I can't seem to work this last bit out, if you wouldn't mind pointing me in the right direction.

I've added two seperate configs one for propel1 and one for propel2.

The config would look like this:

Payum:
    Security:
        Path\To\Your\Token\Model: { propel: propel1 }
    Storages:
        Path\To\Your\Payment\Model: { propel: propel1 }
    gateways:
        offline:
            offline: ~

I've used a similar configuration builder to Doctrine but it doesn't seem to pass through the correct file name.

    public function addConfiguration(ArrayNodeDefinition $builder)
    {
        parent::addConfiguration($builder);
        $builder
            ->beforeNormalization()->ifString()->then(function($v) {

                return array('driver' => $v);
            })->end()
            ->children()
                ->scalarNode('driver')->isRequired()->cannotBeEmpty()->end()
            ->end();        
    }

As with Doctrine it should pass through orm or Mongodb, but it seems to be passing through the class rather than the actual parameter.

Have you got any thoughts on how to tweek this as this should be the last section to finish this off.

[makasim]

Hm, I this it should be done the other way. The config must be like this:

Payum:
    Security:
        Path\To\Your\Token\Model: { propel1: ~ }
    Storages:
        Path\To\Your\Payment\Model: { propel2: ~  } 
    gateways:
        offline:
            offline: ~

So you dont need any magic with parameters.

[liamsorsby]

@makasim @jkabat Just looking at the implementatpion and was thinking. Shouldn't Propel also have the GatewayConfig and ArrayObject mappings as Doctrine does? Not sure if it would be possible but it looks like it may be needed as the following will not work without it.
http://payum.org/doc/1.0/PayumBundle/custom_purchase_examples/paypal_express_checkout#configure-context

Example:

$gateway = "paypal_express_gateway_name";

$storage = $this->get('payum')->getStorage('Acme\PaymentBundle\Entity\PaymentDetails');

$details = $storage->create();

$details['PAYMENTREQUEST_0_CURRENCYCODE'] = "USD";

$storage->update($details);

$captureToken = $this->get('payum.security.token_factory')->createCaptureToken(
            $gatewayName,
            $details,
            'acme_payment_done'
        );

return $this->redirect($captureToken->getTargetUrl());

Throws the following error:

Error: Cannot use object of type Acme\PaymentBundle\Model\Payment as array 

[makasim]

It would be good to provide basic mappings for the models, but as far as I know it is a bit hard to do for Propel.

You can always implement ArrayAccess interface to your model, the example will works in this case.

[liamsorsby]

I'll give it a go and if it works then I'll add this to the current PR.

[liamsorsby]

Doesn't seem to like it:

Request GetCurrency is not supported. 

[makasim]

Doesn't seem to like it:
Request GetCurrency is not supported.

Please Open separate issue for it with some more info like, the payum version, what gateway are you using and some other that you think may help to debug it.