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.

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

Revert "Use the new appendURLString function in WordPressComRestApi" #729

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

crazytonyli
Copy link
Contributor

Description

This PR reverts b01de44 due to an strange usage of WordPressComRestApi.get(path:) function.

- (void)fetchPostsFromEndpoint:(NSURL *)endpoint
withParameters:(NSDictionary *)params
success:(void (^)(NSArray<RemoteReaderPost *> *posts, NSString *algorithm))success
failure:(void (^)(NSError *))failure
{
NSString *path = [endpoint absoluteString];
[self.wordPressComRestApi GET:path

As you can see in the snippet above, a full URL (i.e. https://public-api.wordpress.com/rest/v1.2/read/liked) is used as a path argument. IMO, this is an illegal use case (Gio has kindly create a fix for that in #727) which sends an correct HTTP requests.

Considering the commit b01de44 does break an use case in the app, and who knows what other strange "path" argument the app may pass, I'll revert the commit for now (to prevent it from being shipped in next release) and fix the illegal use case properly in separate PRs.

Testing Details

See the added unit tests.


  • Please check here if your pull request includes additional test coverage.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Love the addition of a unit test 💪

Considering the commit b01de44 does break an use case in the app, and who knows what other strange "path" argument the app may pass, I'll revert the commit for now (to prevent it from being shipped in next release) and fix the illegal use case properly in separate PRs.

That seems very wise. I was pleased that the WordPress UI tests caught this issue. But I don't feel confident in the tests coverage to say that all issues related to this have been addressed 😞

@crazytonyli crazytonyli merged commit 07d9070 into trunk Feb 20, 2024
5 checks passed
@crazytonyli crazytonyli deleted the fix-reader-post-api branch February 20, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants