-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't store settings as JSON #13
Comments
@guillaumemolter can you confirm that WP-CLI's This amounts to a difference in opinion. We either manually run Plus the Postmark plugin's UI is javascript based, and uses the unmodified JSON settings (makes thing a lot simpler). |
Yeah no you right...thinking about it twice...I don't see why it would be an issue.
Well until you need to do some data validation, escaping etc... see https://github.com/wildbit/postmark-wordpress/blob/master/postmark.php#L95 where the json is decoded only to check that it's valid... and then if you want to sanitize the data properly, on need to decode and re-encode. Once again there is nothing wrong with JSON ( I work with JSON on all my JS projects) and PHP serialized strings are a pain to work with, but my opinion or yours don't really matter here, it's the WordPress way of doing things. And in my experience, when you work with complex CMS/Frameworks, if you don't have a really good reason not to,it's always a good idea to embrace the philosophy of the project. If anything else it would make the code easier to read and understand for WordPress devs so they can more easily contribute to a plugin that wildbit clearly doesn't have time to update/improve.
Yes, as you will see I have refactored this, to be able to properly escape the data before displaying it. Once again security good practice never trust the data (especially when this data hasn't been validated and sanitized properly on the back end. Also IMO easier to understand. Some extra resources to justify all this: |
I am biased - being a WordPress dev - but I am support the opinion to go the WordPress way.
Especially not using the helpers provided by WP core is bad. WP is already bad when it comes to dependencies and reusability so we shouldn't make it worse by ignoring working concepts for code reuse. ALSO: a change here will not break anything anyone could have extended upon the plugin. No API/hooks will be affected. |
update_option
accepts arrays as parameter and will handle auto-serialization.get_option
will also auto-deserialize into an array.I know JSON is great and pretty popular but WordPress uses PHP serialized data. Not using it requires some extra processing that would normally be automatically handled by WordPress.
It also breaks compatibility with some widely use WordPress tools (ie: search-replace of the WP-CLI).The text was updated successfully, but these errors were encountered: