v2 endpoints require modified locale key

Expected behavior

As per p77Llu-byF-p2, v2 endpoints must pass _locale rather than locale. Prior versions should still use locale.

From the perspective of semantic versioning, implementing this may be a breaking change.

Actual behavior

In the default case where appendsPreferredLanguageLocale is true, a localization identifier is added to requests with a key of locale. This needs to change based on the endpoint version.

Steps to reproduce the behavior

Inspect a request constructed via WordPressKit and scrutinize the query string. Note the value locale is used for all endpoint versions.

WordPressKit Environment

All devices & versions of WordPressKit, Xcode & iOS.

• WordPressKit Version [e.g. 22] : all versions
• OS: [e.g. iOS 12.1.3 (16D40)] : all versions
• Device: [e.g. iPhone XS] : all devices
• Xcode Version: [e.g. Xcode 10.1 (10B61)] : all versions

Additional Context

N/A

Context

I drafted a naive PR for this issue that has stimulated some interesting discussion about both ServiceRemoteWordPressComREST & WordPressComRestApi.

Quoting @SergioEstevao:

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.

Observations

I considered this input a bit further, and would like to continue the discussion here. For context, WPiOS services currently subclass ServiceRemoteWordPressComREST and initialize with an instance of WordPressComRestApi.

At the time of this writing, the designated initializer takes a single instance of the WP API:
-[ServiceRemoteWordPressComREST initWithWordPressComRestApi:]

That class offers a single instance method:
-[ServiceRemoteWordPressComREST pathForEndpoint:withVersion:], which modifies a path to create a version-specific one.

That method is used in conjunction with the locale-handling behavior in WordPressComRestApi to compose a request.

I wonder if we are thinking about this the wrong way?

It strikes me that we should better separate the concerns of request composition from request execution.

Any change will have broad scope

At the time of this writing, we have

• 89 Objective-C invocations in 15 files : -[ServiceRemoteWordPressComREST pathForEndpoint:withVersion:], all for v2.0 or earlier.
• Swift - 75 results in 25 files: ServiceRemoteWordPressComREST.path(forEndpoint:withVersion:), with a mix of endpoint versions

Recommendations

• In the short-term (i.e., for WPiOS v11.8 , I think we should expose an initializer (on WordPressComRestApi) that allows for the caller to specify a localeKeyto use when composing & executing requests. To accommodate the majority of clients, this will default tolocale. We will modify the callers of v2endpoints to inject_locale` instead.
• Longer-term, we can devise & implement a plan to extricate request composition & execution and propagate those changes to WPiOS.

[SergioEstevao]

@stevebaranski thanks for the in-depth analysis of the issue!

One question for our services classes do they mix inside the same class V1.X and V2.x endpoint calls?

@SergioEstevao you bet!

One question for our services classes do they mix inside the same class V1.X and V2.x endpoint calls?

ActivityServiceRemote appears to call heterogenous major endpoint versions.

I thought it might be possible to separate these, perhaps in a fashion similar to PlansServiceRemote & PlanServiceRemote_ApiVersion1_3?

Thoughts?

[frosty]

Thanks for the analysis! I think your recommendation makes sense – a smaller change upfront that will allow us to move things forward, with an eye to longer-term changes in the future.

[SergioEstevao]

@stevebaranski

I thought it might be possible to separate these, perhaps in a fashion similar to PlansServiceRemote & PlanServiceRemote_ApiVersion1_3?

That looks good, then we can have each service using the API with the correct locale parameter.

I will make it so. I anticipate sharing a PRs today for both WordPressKit & WPiOS. Thank you!