The "Style Settings for Stripe Card Element" admin UI field does not work with styles that contain valid CSS with commas—except when the comma separates individual style pairs. For example, this valid CSS breaks:
fontFamily: "MyFont, arial, helvetica, sans-serif", fontSize: 15px, color: "#333333", iconColor: "blue", '::placeholder': {color: '#333333',}
This is because function _uc_stripe_get_element_styles_array on line 1169 of uc_stripe.module uses a slightly too-simple approach of explode(',') to separate the string into key-value pairs as it creates the $styles array.
I can think of two ways to solve it. First one is to create a simple regular expression-based parser to correctly separate this string into key-value pairs. A second approach would be not to parse this at all, but to pass the entry as-is, after some safety validation, without trying to separate it into key-value pairs. After all, this entry is passed as a whole into uc_stripe.js and there seems no need for the key-value array, see lines 20 and 67 of uc_stripe.js.
On another note, this field is too short for realistic use. It is a textfield at the moment, and it would be more appropriate to make it into a textarea or at least a longer textfield.
I would be happy to provide a patch, but I would like to understand which of the two approaches is more appropriate. Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | uc_stripe-3126638-9-style-settings-break-when-commas.patch | 6.2 KB | Rafal Lukawiecki |
| #3 | uc_stripe-3126638-1-style-settings-break-when-commas.patch | 2.26 KB | Rafal Lukawiecki |
Comments
Comment #2
Rafal LukawieckiComment #3
Rafal LukawieckiI attach a patch with a possible solution. The style setting field has been increased in size to a textarea to allow for more realistic styling options. As it does not make sense to write a full-on CSS parser in this module just to pass the styling to uc_stripe.js, this patch checks if the value provided in this field has already been encoded as valid JSON or not. If it has, it passes the JSON as-is to uc_stripe.js. If the string is not valid JSON, it falls back to the way this module has worked before, that is, it will use the helper function
_uc_stripe_get_element_styles_arrayto convert a simple, comma-delimited list of style:value pairs into an array, before encoding it as JSON. I have updated the admin UI descriptions, too.In other words, with this patch applied this module works the same way for existing installations that use the simpler styling strings, while adding the option of more complex formatting if desired.
On another note, it might be a good idea to change the default style from using 1em font size, as that generates a Stripe warning, since Stripe now recommend using absolute font sizes in px, rather than relative. I would be happy to add that change to the patch if @AndraeRay agrees.
Comment #4
andraeray commentedHey Rafal,
Thanks for identifying this and making a fix.
I like your approach of allowing it to work “as is” but also allowing a full json string.
Is it possible to make a new textarea instead of changing the old textfield?
I would prefer to see that and a toggle where the user can select basic or advanced.
Basic would work just like before (with perhaps better error checking).
Advanced field will take in a full JSON string which can validate and throw an error if the JSON is invalid.
I think the advantage is that this approach allows a non tech savy user to just put in basic settings.
And the advanced user can get a validation error if they enter a badly formed json string.
Comment #5
andraeray commentedComment #6
Rafal LukawieckiGood idea, @AndraeRay. Do you mean a new text area and to remove the existing one, instead of changing the existing one? If we do that, it will break sites that have used Features to export the existing one, as the name of the variable will change. That is the reason why I have repurposed the existing one.
Adding a checkbox or a toggle of some other kind to let the user decide between "simple" and JSON-encoded makes sense. It would allow us to do a simple validation of the JSON if that is the intention of the user, catching simple errors that are hard to spot like incorrectly pasted quotation marks—don't ask why. :) The toggle would need to default to the simple style so as not to break existing sites esp. if they use Features. What do you think?
Comment #7
andraeray commentedHey Rafal,
I think it's best to leave the existing textfield, and then add a new textarea for advanced. Then the toggle can show/hide each one.
And it's exactly for the reasons you mentioned.
Comment #8
Rafal LukawieckiI understand. I will need to dig up how to create a toggling admin form field. Give me some time. :)
Comment #9
Rafal LukawieckiI attach a new patch which creates a new field and a variable for the JSON-encoded styling. I added form validation that checks if it is valid JSON. Also, I made a change to uc_stripe.js so that the new styling becomes the value of the "style" element, rather than its "base" child. This allows for richer styling, for example including complete, empty, and invalid variants, which was not possible before. At the same time, there is no change to how simple styling works because after the simple string gets JSON-encoded it gets surrounded with the "base" element, unless it is empty.
I have also updated the help text's link to the Stripe site because the old one was no longer pointing to the styling section.
Finally, I have changed the default styling to use fontSize: 16px instead of 1em as per the new Stripe recommendations—using relative metrics, like 1em, now throws a warning in the browser debug console.
Comment #10
Rafal LukawieckiOn another note, @AndraeRay, my Drupal coding standards editor plugin tells me that there are minor indentation and whitespace issues in the current version of uc_stripe.module and uc_stripe.js. If you would like me to, I can regenerate this patch with the indentation styling consistent with Drupal standards. Your decision.
Comment #11
andraeray commentedThanks @Rafal, sounds good. I'm planning to test out the patch this week.
Thanks for setting the default to 16px.
You can leave the whitespace and indentation errors for now. Version 3.5 will be a version dedicated to fixing those issues and php code styling issues. I don't want to include them in the next version 3.4 because there are a lot!
Comment #12
Rafal LukawieckiThank you for your help, @AndraeRay.
Comment #13
andraeray commentedHey Rafal,
Thanks for working on this and sorry for the long delay.
Your work looks good. I made minor changes mainly to add php constants, delete vars on uninstall.
Comment #14
andraeray commentedComment #16
andraeray commentedComment #17
Rafal LukawieckiMany thanks, @AndraeRay, for working on this issue and for the commit. :)