A more versatile solution to field naming

[zanderwar]

Field renaming support :)

Let me know what you think

[bummzack]

Looks good, but we'd need some way to get the fields mapped to their respective Omnipay parameters. The Omnipay parameters are named number, expiryYear etc. and it won't accept parameters that are named differently…

Example: The shop module just uses the GatewayFieldsFactory to create the form fields and passes the form data directly to the gateway (for initialization). So the shop module would have to be altered as well to first normalize the incoming-form data field-names. This will also affect RequiredFields validators, that rely on the existing field-names. Example: https://github.com/silvershop/silvershop-core/blob/12fb6cf7e4f67e4adf8d6ac411729095d7819418/code/account/OrderActionsForm.php#L227

[zanderwar]

Any suggestions? :)

[bummzack]

The validator field names could also be wrapped in getFieldName calls, although this is rather cumbersome. Something like a batch rename would be good, eg:

$fieldFactory = GatewayFieldsFactory::create();
RequiredFields::create($fieldFactory->getFieldNames([
    'type', 'name', 'number'
]));

Then the required-fields instance would be initialized with the correct field names…

To normalize data for a gateway, maybe a method GatewayFieldsFactory::normalizeFormData could be implemented. It should take the raw form data as parameter and return an array with the fields renamed to their Omnipay parameter name. Maybe even some whitelisting could be implemented that would strip all unwanted fields from the return value.

[zanderwar]

@bummzack Where would said GatewayFieldsFactory::normalizeFormData need to be called? :)

[bummzack]

I don't see too much problems with altering silvershop, since these are internals and don't affect the public API. Also, the default configuration should not change any form-fields, so unless a user configures special field-names for omnipay, everything will behave as it did previously.

Candidates for normalizeFormData are form handlers that pass their data into the OrderProcessor::makePayment method.
Eg. https://github.com/silvershop/silvershop-core/blob/12fb6cf7e4f67e4adf8d6ac411729095d7819418/code/account/OrderActionsForm.php#L127 and https://github.com/silvershop/silvershop-core/blob/12fb6cf7e4f67e4adf8d6ac411729095d7819418/code/checkout/PaymentForm.php#L117

[zanderwar]

Excellent, will submit a new PR soon / tomorrow

[zanderwar]

You can comment out the entire rename.yml file if you want, I just made it as obvious as possible what to do for the "noobie"

[bummzack]

Thanks for all the work you're putting into this. I really don't agree with having test-data in the actual methods though… you can just use the config API in your tests to test different configurations, eg. in your test, do this:

Config::inst()->update('GatewayFieldsFactory', 'rename', [
    'prefix' => 'prefix_',
    'name' => 'testName',
    'number' => 'testNumber',
    'expiryMonth' => 'testExpiryMonth',
    'expiryYear' => 'testExpiryYear'
]);

[zanderwar]

Happy to help :) and I did feel weird putting it in there to be honest haha. (I only started with SilverStripe a couple months ago)

[bummzack]

Thanks!

[bummzack]

It seems, that rules are generally not merged. So if I have:

GatewayFieldsFactory:
  rename:
    prefix: 'global_'
    name: 'globalName'
    number: 'globalNumber'
    Stripe:
      prefix: 'stripe_'
      cvv: 'stripeCVV'

And I use the GatewayFieldsFactory without a gateway parameter, I might get:

global_globalName
global_globalNumber
global_cvv

And with Stripe as the gateway, I'll get:

stripe_name
stripe_number
stripe_stripeCVV

Not sure, if this is what a developer might expect? My first assumption was, that the global config would be global and could be overridden with specific configurations. So if there was a field that's not defined for the gateway, it would fall back to the global one:

stripe_globalName
stripe_globalNumber
stripe_stripeCVV

What's really confusing here is the prefix and name mixing though…
Was there any specific reason you added a prefix? Do you need this in a project of yours? I guess it makes it easier to configure if you can just set a prefix instead of renaming all the fields in the config.

So, to bring this one to a conclusion: Do we need both, a prefix and individual renaming options? It might be sufficient to just have a prefix, maybe with the possibility to override the prefix on a per-gateway basis?

[bummzack]

I noticed another problem with converting the field-values back to their correct names. I've updated your testNormalizeFormData to test for this case and the test currently fails.

I've pushed the changed test to my fork of the module:
https://github.com/bummzack/silverstripe-omnipay/blob/fix-field-renames/tests/GatewayFieldsFactoryTest.php#L249

So, normalizeFormData needs to be fixed, but it also depends on whether we want to reduce some of the functionality (as outlined in my previous comment).

[bummzack]

When looking through the current implementation I was also thinking about ways to improve readability and performance.

I think getFieldName could be simplified a lot, if prefix and a rename-map would be pre-evaluated (for example in the setGateway method). So whenever the gateway is being set, determine the field-prefix and create a rename-map and store it as class-members.

Then field renaming would be as simple as looking up the field-name in the rename-map (if it exists) and prefix the name with the prefix…

Reverting fields back should also be simpler than the current implementation, what do you think?

[zanderwar]

Not sure, if this is what a developer might expect? My first assumption was, that the global config would be global and could be overridden with specific configurations. So if there was a field that's not defined for the gateway, it would fall back to the global one

This is indeed the intention and gateways should definitely be falling back to their global parents so will get this resolved. Though I didn't prepare for gateways themselves to have prefix, noted this is definitely a mistake and will get that implemented.

Was there any specific reason you added a prefix? Do you need this in a project of yours? I guess it makes it easier to configure if you can just set a prefix instead of renaming all the fields in the config.

My reasoning was simply versatility, where one would be interested in just prefixing all fields, others may be interested in renaming specific fields, though this does seem a little convoluted and you have my vote for reducing functionality to purely prefix where the gateways prefix is prioritised

When looking through the current implementation I was also thinking about ways to improve readability and performance.

I'll take this into consideration during my edits to resolve the above