Add support for hidden fields in general (and 'token' specifically) on the payment form. #12332
Conversation
|
(Standard links)
|
| @@ -682,7 +682,6 @@ public function getPaymentFormFieldsMetadata() { | |||
| 'htmlType' => 'text', | |||
| 'name' => 'credit_card_number', | |||
| 'title' => ts('Card Number'), | |||
| 'cc_field' => TRUE, | |||
There was a problem hiding this comment.
per later removed todo - "// @todo - remove the cc_field check - no longer useful."
CRM/Core/Payment/Form.php
Outdated
| @@ -115,25 +114,23 @@ static protected function setBillingAddressFields(&$form, $processor) { | |||
| */ | |||
| protected static function addCommonFields(&$form, $paymentFields) { | |||
| $requiredPaymentFields = array(); | |||
| foreach ($paymentFields as $name => $field) { | |||
| // @todo - remove the cc_field check - no longer useful. | |||
| if (!empty($field['cc_field'])) { | |||
There was a problem hiding this comment.
removal of IF is the change here
| @@ -39,7 +39,7 @@ | |||
| <div class="label">{$form.$paymentField.label} | |||
| {if $requiredPaymentFields.$name}<span class="crm-marker" title="{ts}This field is required.{/ts}">*</span>{/if} | |||
| </div> | |||
| <div class="content">{$form.$paymentField.html} | |||
| <div class="content">{if $form.$paymentField.html}{$form.$paymentField.html}{else}<input id="{$paymentField}" name="{$paymentField}" type="hidden" />{/if} | |||
There was a problem hiding this comment.
quickform does not add html for hidden fields
7cecd12 to
2fdaf2d
Compare
|
I'm not entirely clear what problem this is solving. For my current coding project, I created a simple text field that was hidden by css, and that worked fine. How is a real hidden field a better choice? Semantics is a reasonable argument, I suppose, but perhaps there's something else I haven't even thought of? The biggest challenge I'm experiencing is actually about error handling, and perhaps setting it as a separate field type would help, though I don't immediately see how. In fact, if we're going to add a field here, I'd love to be able to create a markup field in my billing fields (to stick in an iframe in my case). I'd also note that you're doing some clean up in here which is nice, but it makes for a harder review since I have to figure out what code is solving which problem. But making me do more work is a valid goal as well, especially considering how many cans of worms this comment is opening up. And on a final lighter note, referencing those three hardest things about programming, we really should not call this a token, even though that's what some payment processors call it, since we have a completely separate concept of token in civicrm. A payment token would be better, but a different word would be best I think, though I can't think what it would be. |
|
I mean I guess we can hide by css rather than use a real hidden field - but we do have other hidden fields in CiviCRM so there is precedent and the goal is for it to be simple to add them & consistent. I get your point about clean up - in this case I removed the cc field ref in the same commit because it didn't work if I didn't do that unless I added it to the new field which seemed crazy. regarding field name - I agree token might be too generic. We could go the whole hog & payment_authorization_token ? |
|
I like more specific field name and following established pattern for hidden fields. I don't think this is the case but I just want to ask if these tokens are sensitive data that need better protection. |
|
re: naming - after doing a little research, I see that everyone calls it a token, so there really isn't any good alternative. I'm also happy with just payment_token, that seems unambiguous without being pedantic. re: sensitive data - in theory, no (well, certainly less sensitive than the credit card fields they are replacing, and in theory having them in a log is not a security issue, but for sure you wouldn't want them being posted to facebook in real time, for example ...). Here's a nice detailed answer: https://en.wikipedia.org/wiki/Tokenization_(data_security) |
|
@JoeMurray - the main reason why a processor would want to add a token field (or other hidden field) is because there is javascript on the payment page that substitutes credit card details for a token. Actually I think payment_token should be a bit more special in later steps - ie. there is a specific trickiness when card details are removed from the form in favour of a payment_token and then people go forwards & back and there is a problem since the credit card has been masked. That is not for this round but identifies that payment_token will be a bit special |
|
OH @adixon the other part of this change that might have looked like clean up but wasn't is the stuff to do with the label. The point of that change was to make sure no label was assigned when only hidden fields are in the block |
…payment form. It is common enough for payment processors to store a hidden token on the form that we should specifically support it. In general the processor can add other fields of type hidden and they should show up.
|
@adixon updated to payment_token now Re adding a markup field - I wonder if you set 'htmlType' to 'element' that works? |
|
test this please |
|
@Monish could you look at this field and one or two other hidden fields to ensure that your Accessibility work does not expose them to screen readers inadvertently? |
|
interesting. I will need to alter the omnipay extension to reflect the name change for that to be easy for Monish - but I think this is largely agreed so that is probably worth doing |
|
test this please |
1 similar comment
|
test this please |
|
Merging based on code review and feedback in PR comments. |
|
@eileenmcnaughton @colemanw This went through before I had a chance to comment. It actually breaks any payment processor that uses hidden fields because of this line: https://github.com/civicrm/civicrm-core/pull/12332/files#diff-32cb5243c1ab0fe9952a67713a89603eR42 Hidden fields are now being added twice, once by some magic that I cannot find (as they always were) and another via the The rest of the PR is probably fine, but can we revert that line please? |
|
@mattwire can you give me an example? For omnipay it's only going on once |
|
@eileenmcnaughton Right, so this seems to happen on:
I think it may be caused by the AJAX loading, but the Wordpress frontend contribution page is a bit confusing - I guess it might be being loaded in an AJAXy way. If you let me know how you configured Omnipay I'll see if I can reproduce with that. I've tested with both https://github.com/mattwire/com.drastikbydesign.stripe and https://github.com/mattwire/org.civicrm.smartdebit Both fail since this PR. |
|
@mattwire I have been trying in Drupal fronend (multiple processors, dummy as default) and haven't seen that field load twice. There is a bit of magic loading going on in those extensions you pointed me to - but I couldn't see how the hidden fields were already loaded - will try to ping you online to dig more. This is the part that I'm not seeing "once by some magic that I cannot find (as they always were) " |
|
Ah @mattwire - I found the magic - it's defining 'id' in attributes - will do a follow up |
Overview
Allow payment processors to define 'token' as a hidden field without adding additional metadata. Allow adding other hidden fields with additional metadata
Before
Difficult for payment processors to add a hidden 'token' field, or any hidden field
After
Easy to add the former by overriding getPaymentFields & the latter by ALSO overriding getPaymentFieldsMetadata
Technical Details
It is common enough for payment processors to store a hidden token on the form that we should specifically support it. In fields of type hidden should also be supported on this form.
Comments
@adixon FYI (also @mattwire )