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

Add apiURL parameter for changing WP.com API URL #731

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Add apiURL parameter for changing WP.com API URL #731

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31798ce
Add `apiURL` parameter to changing WP.com API URL
crazytonyli Feb 20, 2024
7c01ab3
Add an unit test for changing API url
crazytonyli Feb 20, 2024
dbce6ab
Remove unnecessary labels
crazytonyli Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions WordPressKit/WordPressOrgRestApi.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public final class WordPressOrgRestApi: NSObject {
}

enum Site {
case dotCom(siteID: UInt64, bearerToken: String)
case dotCom(siteID: UInt64, bearerToken: String, apiURL: URL)
case selfHosted(apiURL: URL, credential: SelfHostedSiteCredential)
Comment on lines -43 to 44
Copy link
Contributor

@mokagio mokagio Feb 20, 2024

Choose a reason for hiding this comment

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

Nitpick: What do you think of making apiURL the first associated value, so that it's in line with the selfHosted case?

One argument against doing so would be that siteID is more important when it comes to identifying a .com site, so it belongs in first position. I'd be fine with that. This is just a style nitpick / question that I'm not sure on.

Copy link
Contributor Author

@crazytonyli crazytonyli Feb 21, 2024

Choose a reason for hiding this comment

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

I'd agree with that argument, considering 'apiURL' is an optional parameter in the initialiser. 😄

}

Expand All @@ -49,8 +49,8 @@ public final class WordPressOrgRestApi: NSObject {

var selfHostedSiteNonce: String?

public convenience init(dotComSiteID: UInt64, bearerToken: String, userAgent: String? = nil) {
self.init(site: .dotCom(siteID: dotComSiteID, bearerToken: bearerToken), userAgent: userAgent)
public convenience init(dotComSiteID: UInt64, bearerToken: String, userAgent: String? = nil, apiURL: URL = WordPressComRestApi.apiBaseURL) {
self.init(site: .dotCom(siteID: dotComSiteID, bearerToken: bearerToken, apiURL: apiURL), userAgent: userAgent)
}

public convenience init(selfHostedSiteWPJSONURL apiURL: URL, credential: SelfHostedSiteCredential, userAgent: String? = nil) {
Expand All @@ -68,7 +68,7 @@ public final class WordPressOrgRestApi: NSObject {
if let userAgent {
additionalHeaders["User-Agent"] = userAgent
}
if case let Site.dotCom(siteID: _, bearerToken: token) = site {
if case let Site.dotCom(siteID: _, bearerToken: token, _) = site {
Copy link
Contributor

@mokagio mokagio Feb 20, 2024

Choose a reason for hiding this comment

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

Nitpick: The new _ entry made me realize that the siteID: label is unnecessary

Suggested change
if case let Site.dotCom(siteID: _, bearerToken: token, _) = site {
if case let Site.dotCom(_, bearerToken: token, _) = site {

Site note: I looked for a SwiftLint rule for this but did not find one that would have picked that up. At first, I browsed the rules docs. Then, I tried with

cat WordPressKit/WordPressOrgRestApi.swift \
  | ./Pods/SwiftLint/swiftlint lint --quiet --enable-all-rules --use-stdin \
  | sort -v

additionalHeaders["Authorization"] = "Bearer \(token)"
}

Expand Down Expand Up @@ -203,8 +203,8 @@ public final class WordPressOrgRestApi: NSObject {
private extension WordPressOrgRestApi {
func apiBaseURL() -> URL {
switch site {
case .dotCom:
return URL(string: "https://public-api.wordpress.com")!
case let .dotCom(_, _, apiURL):
return apiURL
Comment on lines -206 to +207
Copy link
Contributor

@mokagio mokagio Feb 20, 2024

Choose a reason for hiding this comment

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

A nice side effect is that now we are one step closer to having a single source of truth for what this URL should be.

image

case let .selfHosted(apiURL, _):
return apiURL
}
Expand Down Expand Up @@ -252,7 +252,7 @@ private extension HTTPRequestBuilder {
}

switch site {
case let .dotCom(siteID, _):
case let .dotCom(siteID, _, _):
// Currently only the following namespaces are supported. When adding more supported namespaces, remember to
// update the "path adapter" code below for the REST API in WP.COM.
assert(route.hasPrefix("/wp/v2") || route.hasPrefix("/wp-block-editor/v1"), "Unsupported .org REST API route: \(route)")
Expand Down
18 changes: 18 additions & 0 deletions WordPressKitTests/WordPressOrgRestApiTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,18 @@ class WordPressOrgRestApiTests: XCTestCase {
let api = WordPressOrgRestApi(site: .dotCom(siteID: 1001, bearerToken: "fakeToken"))
let _ = try await api.get(path: "/wp-block-editor/v1/settings", type: AnyResponse.self).get()
}

func testSettingWPComAPIURL() async {
var request: URLRequest?
stub(condition: { _ in true }, response: {
request = $0
return HTTPStubsResponse(error: URLError(.networkConnectionLost))
})

let api = WordPressOrgRestApi(dotComSiteID: 1001, bearerToken: "token", apiURL: URL(string: "http://localhost:8000")!)
let _ = await api.get(path: "/wp/v2/hello", type: AnyResponse.self)
XCTAssertEqual(request?.url?.absoluteString, "http://localhost:8000/wp/v2/sites/1001/hello")
}
}

extension WordPressOrgRestApi {
Expand All @@ -114,4 +126,10 @@ extension WordPressOrgRestApi {
}
}

extension WordPressOrgRestApi.Site {
static func dotCom(siteID: UInt64, bearerToken: String) -> Self {
.dotCom(siteID: siteID, bearerToken: bearerToken, apiURL: WordPressComRestApi.apiBaseURL)
}
}
Copy link
Contributor

@mokagio mokagio Feb 20, 2024

Choose a reason for hiding this comment

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

Note for future readers: This method looks unused in the diff, but it's actually a way to bridge the new implementation, which expects an apiURL parameter, with the existing code that did not have it.

See for example line 103 in this file.


private struct AnyResponse: Decodable {}