Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Add support for locale key injection#82

Closed
ghost wants to merge 6 commits intodevelopfrom
issue/78-version-specific-locale-key
Closed

Add support for locale key injection#82
ghost wants to merge 6 commits intodevelopfrom
issue/78-version-specific-locale-key

Conversation

@ghost
Copy link

@ghost ghost commented Jan 31, 2019

Description

Fixes #78, which requires that _locale be passed as the key to v2 endpoints, while preserving the original locale key for prior versions.

This required special consideration, as ServiceRemoteWordPressComREST is responsible for endpoint versions, while WordPressComRestApi has responsibility for applying locale to requests.

Testing Details

  • Checkout the branch, confirm that it builds, and that both new & existing tests pass.

  • Review the code and documentation for additional context.

  • From WPiOS, checkout the branch try/wpkit-locale-integration and confirm that it compiles & tests pass. Also launch the app and exercise requests to confirm that there are no regressions.

  • Please check here if your pull request includes additional test coverage.

@ghost ghost added the enhancement New feature or request label Jan 31, 2019
@ghost ghost self-assigned this Jan 31, 2019
@ghost ghost requested review from SergioEstevao, ctarda and frosty January 31, 2019 22:02
@ghost
Copy link
Author

ghost commented Jan 31, 2019

@frosty @SergioEstevao given that this work continues what began in #80, would you two be willing to take a look at this? I'd be especially interested to hear if you have any thoughts on make the change more "strict", or if you think the documentation of this change can be improved in some way.

I've also added @ctarda, as this is potentially relevant to the ongoing work with Site Creation.

Finally, the CircleCI error is due to the fact that this repository is not correctly configured at the moment.

@SergioEstevao
Copy link
Contributor

@stevebaranski While you approach in this PR looks ok, I think we are getting on a path that in the future may introduce too much complexity.

I was considering the following alternative, instead of just one WordPressComRestAPI instance ( object not class) what about having two instances:

  • One for the Rest API v1.x
  • Another for the Rest API 2.x

This way we could just set the locale parameter on the constructor of the API object, and we don't need to go around changing depending on the service. If the call is the REST API v2, we get the instance for v2 and make the request.

This way in the future if need to do more changes like this we could even have a different class for API v2.

@ghost ghost added the [Status] DO NOT MERGE label Feb 1, 2019
@ghost
Copy link
Author

ghost commented Feb 1, 2019

@SergioEstevao thank you for taking time to review this, and for providing such great feedback.

I share your concerns, and your suggestion on improving it is valid. I've affixed the DO NOT MERGE to the PR to underscore that I'll rework this - either revising this PR, or creating a new & improved one.

@ghost
Copy link
Author

ghost commented Feb 4, 2019

I opted to continue our discussion in the root issue. I'd welcome your thoughts.

@ghost
Copy link
Author

ghost commented Feb 5, 2019

Closing this PR as discussed. I'll open a new one shortly.

@ghost ghost closed this Feb 5, 2019
@ghost ghost deleted the issue/78-version-specific-locale-key branch February 5, 2019 17:31
@ghost ghost mentioned this pull request Feb 5, 2019
1 task
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v2 endpoints require modified locale key

2 participants