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

Adds JSON extension as a Composer requirement #460

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Adds JSON extension as a Composer requirement #460

merged 2 commits into from
Feb 19, 2020

Conversation

moebrowne
Copy link
Contributor

Description of the Change

Adds JSON PHP extension as a Composer requirement as the extension is being used

Alternate Designs

N/A

Benefits

Ensures that supporting modules are installed before the code is run

Possible Drawbacks

N/A

Verification Process

No code changes were made

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

N/A

@jeffpaul jeffpaul added this to the 2.0.0 milestone Oct 8, 2019
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Oct 8, 2019
@adamsilverstein
Copy link

Hi @moebrowne - thanks for the PR! Overall this looks good especially if it resolves an issue.

Can you clarify where you see "the extension is being used" and what code causes problems before this change?

What issue you were seeing that led you to opening this PR?

@adamsilverstein
Copy link

Note tests are failing as well, not sure why.

@moebrowne
Copy link
Contributor Author

Hi, The JSON extension must be installed else the calls we make to the json_* functions will error. You can see examples of where they're used here:

I'm not sure why the tests are failing as I haven't changed any code?

@adamsilverstein
Copy link

Ah, thanks for clarifying @moebrowne, that makes sense!

I wonder why we aren't using wp_json_encode in these instances which should work in all contexts and not require the JSON extension. In general I'd prefer we stick to this built in WordPress function rather than add an additional dependency.

Can you see any reason not to go that direction instead?

@jeffpaul jeffpaul requested review from dkotter and removed request for adamsilverstein January 27, 2020 21:49
@dinhtungdu
Copy link
Contributor

I agree that we should use wp_json_encode too.

We have this issue because some PHP installs don't include PHP JSON extension. Prior to 5.3, WordPress uses some workarounds/polyfills to ensure the functionality json_* functions. From WordPress 5.3, PHP Native JSON Extension is required to run WP, see: https://make.wordpress.org/core/2019/10/15/php-native-json-extension-now-required/

@jeffpaul @dkotter I suggest we close this PR and create a new one to replace json_* functions by the wp_ versions.

@dinhtungdu
Copy link
Contributor

@jeffpaul, after digging into the codebase of Distributor, I want to hold my suggestion above, json_encode and json_decode are using in our classes, tests, and the plugin-update-checker library that we ship with Distributor. So I think the most reliable approach now is @moebrowne solution.

@jeffpaul
Copy link
Member

@dkotter any disagreements with @dinhtungdu's latest approach noted above?

@dkotter
Copy link
Collaborator

dkotter commented Jan 31, 2020

@jeffpaul @dinhtungdu Nope, this sounds good to me.

@jeffpaul jeffpaul merged commit a4a2840 into 10up:develop Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants