Modified locale handling to inspect request parameters

[ghost]

Description

Fixes #72, which observed that the existing framework behavior inspected a request's path for locale, and by default, appended it if missing. This change ensures that request parameters are also inspected for the presence of the parameter.

From the standpoint of semantic versioning, I view this as a breaking change.

Testing Details

• Checkout the branch and confirm that existing tests pass.

• Review the code and documentation for additional context.

• From WordPressAuthenticator, checkout the branch try/wordpresskit-72-validation and confirm that it compiles & tests pass.

• From WPiOS, checkout the branch try/wordpresskit-72-validation 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.

[SergioEstevao]

From the standpoint of semantic versioning, I view this as a breaking change

If I'm correct the API for external users of the library didn't change, so we could live with an update only of the minor version number? 1.9?

[SergioEstevao]

Do we no longer need to use this method on ObjC in the main app?

[ghost]

I searched for consumers of this in the try/wordpresskit-72-validation branch I created, and could not find any.

[frosty]

Thanks for tackling this!

I built and ran tests in all the projects you specified, and all passed ✅

I left a couple of code level comments, but nothing major. Also, I'm not sure if I missed something, but do these changes provide a way for a different localeKey to be provided to WordPressComRestApi? How will v2 endpoints specify the different key? Do they just specify it as part of their parameters? If so, how does applyLocaleIfNeeded know not to add locale in addition to _locale? I'm not hugely familiar with the code so I might just not understand how it fits together.

[ghost]

@frosty thanks for the review, and for your comments. I have pushed two commits that address the items you pointed out. Regarding:

I'm not sure if I missed something, but do these changes provide a way for a different localeKey to be provided to WordPressComRestApi

Not just yet. The majority of that work is slated for #78, but the change to WordPressComRestApi.buildRequestURLFor(path:parameters:localeKey:) paves the way for it.

For that change, we will likely amend the signatures of these methods to inject the applicable locale key.

• GET(_:parameters:success:failure:)
• POST(_:parameters:success:failure:)
• multipartPOST(_:parameters:fileParts:requestEnqueued:success:failure:)

[ghost]

While tests pass locally, I should note that CircleCI is currently reporting a build error. I inquired about that Friday when others were observed, and was advised that the CircleCI transition is in progress for this repo, and CircleCI doesn’t have a config file for it just yet.

[ghost]

Hey @SergioEstevao - I really appreciate you taking the time to review this!

If I'm correct the API for external users of the library didn't change, so we could live with an update only of the minor version number? 1.9?

This is something I considered as well. My reasoning for updating the major version was two-fold:

1. As you observed, the pathByAppendingPreferredLanguageLocale method was marked public. While I couldn't find any uses of this in WPiOS, the method was being used in our tests. Taking this away could break things for other API consumers.
2. I also intend to resolve v2 endpoints require modified locale key #78 in the current sprint (i.e., the same beta cycle), and it will most certainly be a breaking change. So I felt like I could either update the version now or in that PR.

[frosty]

Thanks for your responses – that all makes sense :)

Fixes look good, too! Let's merge even though CircleCI is currently not working, as it builds / tests pass locally.

[SergioEstevao]

Great work! Please ping me on the follow-up PRs!