Skip to content
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

Open
guillaumemolter opened this issue Aug 6, 2016 · 3 comments
Open

Don't store settings as JSON #13

guillaumemolter opened this issue Aug 6, 2016 · 3 comments

Comments

@guillaumemolter
Copy link
Contributor

guillaumemolter commented Aug 6, 2016

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).

@mgibbs189
Copy link
Contributor

@guillaumemolter can you confirm that WP-CLI's search-replace doesn't support JSON? I don't see why it wouldn't...

This amounts to a difference in opinion. We either manually run json_decode, or WP automatically runs unserialize. The same amount of processing is done regardless.

Plus the Postmark plugin's UI is javascript based, and uses the unmodified JSON settings (makes thing a lot simpler).

@guillaumemolter
Copy link
Contributor Author

guillaumemolter commented Aug 6, 2016

@mgibbs189

can you confirm that WP-CLI's search-replace doesn't support JSON? I don't see why it wouldn't...

Yeah no you right...thinking about it twice...I don't see why it would be an issue.

This amounts to a difference in opinion. We either manually run json_decode, or WP automatically runs unserialize. The same amount of processing is done regardless.

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.

Plus the Postmark plugin's UI is javascript based, and uses the unmodified JSON settings (makes thing a lot simpler).

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:

@cfoellmann
Copy link

I am biased - being a WordPress dev - but I am support the opinion to go the WordPress way.
Main points by @guillaumemolter I support are

  • reusing data validation and escaping already proven in Core and
  • lowering the threshold for contributors from the WP space by doing it the WP 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants