Add credit card icons that are supported

thats a good question. i don't know of a good way to do a patch that includes binary files. maybe just post a code-only patch, and then post the files separately along with paths where they are supposed to go, and i can get them in to the repo for you.

also please note the module in strict bug-fixes only mode at the moment, so this will have to go in to the 2.0 branch.

Thank you for working on this!

Can you post a screenshot of what your patch does? The image archive won't open for me, and the patch doesn't seem to change anything on my interface.

I'm using test stripe API keys, does it matter?

I only have Paypal and Stripe enabled, but still the Stripe option is labeled "Credit card" instead of "Stripe" as set in the Rule.

The patch kinda works for me for 7.x-1.x-dev

Path for the images is path/to/modules/commerce_stripe/images.

Don't forget to clear your cache.

One thing that i found though is that $icons = commerce_stripe_icons('CAD'); is hard coded. So it will only show icons for Visa, Amex and MasterCard. If it happens you are using the module for US you must change that in code to have icons for JCB, Discover and Diners Club. Also icons for Diners Club & JCB are missing.

I don't know if stripe often changes which cards are accepted in which country but having to update the code to match all if they do seems a bit messy.

But having this feature would be great.

Sounds like it might need work. I'll see if I can get to this soon, as having card icons is a nice feature to have.

The patch from #3 didn't apply anymore so I rebased on the latest dev, did some fixes like missing images and cleaned the code a bit. Images are also type svg now.

Added missing union pay

+function commerce_stripe_icons($country = '') {

Would it be better to set $country as NULL

Looks good otherwise.

For what it's worth, I don't mind getting icons into the module, but having them show up unexpectedly after a minor version bump could break existing checkout form theming. Can we add a setting that governs whether or not icons are included? It can even default to be enabled for new installations, but existing installations should have it disabled until it is explicitly enabled.

@rszrama thanks for the review, makes sense to me.

Added stting in stripe actions:

The setting will be enabled by default for new installs. For old installs there is an update hook to disable the setting.

Here are some screenshots with and wihout the icons enabled:

Here is the patch

The patch in #16 looks good. Tested in local and works great.

Reviewing the patch, the main issue I see is that the update function assumes the only payment method configuration a site might have that uses the Stripe action is the default one. This isn't a safe assumption, because any number of payment method rules could be defined that enable an instance of the gateway.

I didn't look into it further to say this is definitely possible, but I'm curious to know if you could do something besides updating Rules configurations here. For example, can we just leave it defaulted to TRUE but code the module to treat the absence of a value set for enable_credit_card_icons in the action configuration as FALSE?

The alternative would be to default it to FALSE.

Makes sense. I removed the config update hook. The problem is that if we set the default value to TRUE, the value is always present. And I can not check if this is a first time setup or editing an old config.

To follow the rule of least surprise I set the default to be FALSE. If i don't do that and someone edits the gateway without unchecking the checkbox the credit card branding will show up (unintentionally)

Like Ryan said, I think calling _commerce_stripe_load_setting() is wrong, since it'll always get the value from the default rules configuration.

You actually have access to the payment method Rule from commerce_stripe_form_commerce_checkout_form_alter(), you should be using that instead.

As soon as this is fixed, it should be good to go. If you try with multiple payment method instances, you'll notice only the default instance settings will be checked.

@jsacksick Thanks, found the setting available in the $form. Did some testing and it works as expected

Here are some screenshots with multiple PG setup

Commited.