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

add JSON_NUMERIC_CHECK to json encode options #3288

Merged
merged 14 commits into from
Jul 15, 2020
Merged

add JSON_NUMERIC_CHECK to json encode options #3288

merged 14 commits into from
Jul 15, 2020

Conversation

devorama
Copy link
Contributor

linked to
#3286

This allows encoding to make numeric data more true to its type (not enclose them with "", making the receiving system identify
the types easier.

Note making the options variable configurable might be handy in the future depending on the needs of the users
people they interface with they might need additional options added here

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

linked to 
#3286

This allows encoding to make numeric data more true to its type (not enclose them with "", making the receiving system identify
the types easier.

Note making the options variable configurable might be handy in the future depending on the needs of the users
people they interface with they might need additional options added here
@michalsn
Copy link
Member

Adding a numeric check just like that may potentially break someone's code. Developers may already use code like:

if ($foo->bar === '1')

But I like your idea of making this configurable. Would you like to work on this PR and add something like $formatterOptions array to the Config/Format.php file? We would use it like:

$config = new Filter();
$options = $config->formatterOptions['application/json'] ?? JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES | JSON_PARTIAL_OUTPUT_ON_ERROR

I'm not really sure about naming the keys for this new variable so if anybody has a different idea, I'm all ears.

@devorama
Copy link
Contributor Author

Hi there @michalsn , I will do this asap. Will look at the current names being used and see if I can add something that appropiate.

But I like your idea of making this configurable. Would you like to work on this PR and add something like $formatterOptions array to the Config/Format.php file? We would use it like:

add config option to add to format options
add config options to formatter
removed testing formatter option value
@devorama
Copy link
Contributor Author

@michalsn I added the config value and also the code for XML and JSON Formatter, for XML I added the additional option to be added as per the php docs or default to the phpdocs default.

One thing to note is I did not make it to overwrite the default options but to add to it, the reason for this is if you look at
JSONFormatter the JSON_PARTIAL_OUTPUT_ON_ERROR is actually important for error checking afterwards in the same function, thus allowing a total override of default options might break actual logic for that reason I believed it saver to make it a add to current option only. The default for the settings are all null.

@michalsn
Copy link
Member

The whole sense of making it configurable is that we should have the ability to change default flags. So please add all current flags to the config file as default.

You can check later in the code if JSON_PARTIAL_OUTPUT_ON_ERROR is set via bitwise operator and add it if needed.

if (! ($options & JSON_PARTIAL_OUTPUT_ON_ERROR))
// add it to options

@michalsn
Copy link
Member

Actually I don't think that we have to check it at all (options) - we can just add it and it should work fine (even if it's already present).

@michalsn
Copy link
Member

Just update your code. If I will have any requests for changes I will let you know :)

@devorama
Copy link
Contributor Author

Oops used my company account there :), will do the chagne now, just finished testing it and it seems to work :)

Just update your code. If I will have any requests for changes I will let you know :)

Move non default values to config
Move non default values to config
@devorama
Copy link
Contributor Author

@michalsn K changes made, should work now.

app/Config/Format.php Outdated Show resolved Hide resolved
system/Format/JSONFormatter.php Outdated Show resolved Hide resolved
app/Config/Format.php Outdated Show resolved Hide resolved
system/Format/JSONFormatter.php Outdated Show resolved Hide resolved
@devorama
Copy link
Contributor Author

@michalsn changes made,will try and enforce the coding standard more with next changes.

@michalsn
Copy link
Member

@devorama No worries. Just one last change and I think we're good.

update as per pull request change
@devorama
Copy link
Contributor Author

@michalsn all done :)

update as per change request
update as per change request
update as per change request
@devorama
Copy link
Contributor Author

@michalsn and @paulbalandan Made the changes as requested , sorry for the config slip up.

@lonnieezell lonnieezell merged commit f6be0ab into codeigniter4:develop Jul 15, 2020
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

Successfully merging this pull request may close these issues.

4 participants