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.

Sign up for GitHub

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

Jump to bottom

Site Settings: Add support for the language setting to the API #8568

Merged
merged 8 commits into from
Jan 22, 2018
Merged

Site Settings: Add support for the language setting to the API #8568

merged 8 commits into from
Jan 22, 2018

Conversation

jblz
Copy link
Member

@jblz jblz commented Jan 19, 2018 •
edited
Loading

Provide API support for users to manage site / blog language from their WordPress.com Site Settings page.

See: Automattic/wp-calypso#11316

Changes proposed in this Pull Request:

  • Declare support for lang_id
  • Prevent modification & return an error code if the client cannot change the setting
  • Download language packs if needed & available
  • Broker access to the WPLANG option

Testing instructions:

Proposed changelog entry for your changes:

Added support for changing site language via WordPress.com

@jblz jblz self-assigned this Jan 19, 2018
@jblz jblz requested a review from frontdevde January 19, 2018 17:09
@jblz jblz requested a review from a team as a code owner January 19, 2018 17:09
@jblz
Stop checking that WPLANG is non-empty
fd4ece5 
PHP 5.3 errors when trying to call `empty( SOME_CONSTANT_NAME )`. If it's set at all, just return the error.
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] i18n Internationalization / i18n, adaptation to different languages [Feature] WP REST API labels Jan 19, 2018
@frontdevde frontdevde mentioned this pull request Jan 19, 2018
Merged
@jblz jblz added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 19, 2018
@jblz jblz requested review from tyxla, zinigor and dereksmart and removed request for frontdevde January 19, 2018 20:03
@jblz jblz added this to the 5.8 milestone Jan 19, 2018
tyxla
tyxla approved these changes Jan 19, 2018
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave this a spin in various cases (with/without constant, with/without the install_languages capability, with a custom language downloaded and specified and with the default en_US language.

Works like a charm 👍 Code also looks great! ❤️

_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php
}

$value = get_option( 'WPLANG' );
$response[ $setting ] = empty( $value ) ? 'en_US' : $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this, we'll return en_US as a default language, but according to the definition of the setting in get_updateable_data_list(), it's be expected that it would return an empty string. Perhaps we should use the same in both locations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly set it to en_US in the declaration in 2a408ec. Thanks!

jblz
jblz commented Jan 20, 2018
View reviewed changes
_inc/lib/core-api/class.jetpack-core-api-module-endpoints.php
}

// `wp_download_language_pack` only tries to download packs if they're not already available
$language = wp_download_language_pack( $value );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo: make sure wp_download_language_pack doesn't return false before setting the option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2a408ec

zinigor
zinigor approved these changes Jan 22, 2018
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested this, everything looks good! After you have addressed both your own and @tyxla 's comment, feel free to merge.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 22, 2018
@jblz
Explicitly declare US English as the default &
2a408ec 
Guard against `wp_download_language_pack` failure
@jblz jblz merged commit f88694f into master Jan 22, 2018
@jblz jblz deleted the add/blog-lang-setting branch January 22, 2018 20:13
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 22, 2018
frontdevde added a commit that referenced this pull request Jan 29, 2018
@frontdevde
Changelog: add #8568
e435094
@frontdevde frontdevde mentioned this pull request Jan 29, 2018
Merged
jeherve pushed a commit that referenced this pull request Jan 29, 2018
@frontdevde @jeherve
Changelog: add #8568 (#8646)
b92bc4f
zinigor pushed a commit that referenced this pull request Jan 30, 2018
@oskosk @zinigor
Changelog 5.8: running changelog (#8442)
1ee2c03 
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
@jblz jblz mentioned this pull request Feb 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyxla tyxla tyxla approved these changes

@zinigor zinigor zinigor approved these changes

@dereksmart dereksmart Awaiting requested review from dereksmart

Assignees

@frontdevde frontdevde

@jblz jblz

Labels
[Feature] WP REST API [Focus] i18n Internationalization / i18n, adaptation to different languages [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
6 participants
@jblz @zinigor @tyxla @jeherve @frontdevde @matticbot