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

Fix manual token creation with plain permalinks #6

Conversation

jkmassel
Copy link
Collaborator

@jkmassel jkmassel commented Mar 29, 2019

Uses the built-in get_rest_url API inside of get_rest-uri – this results in permalink-style-aware URI generation.

Fixes #4

To Test:
Before – set a site to use default plain permalinks. Attempt to create a manual token. It should fail with a network error.
After – set a site to use default plain permalinks. Attempt to create a manual token. It should succeed.

Uses the built-in `get_rest_url` API to handle all situations.

Return only the path from `get_rest_uri`
@jkmassel jkmassel marked this pull request as ready for review March 29, 2019 16:00
@jkmassel
Copy link
Collaborator Author

jkmassel commented May 9, 2019

The test is failing here, because the updated get_rest_uri function produces absolute URLs, rather than relative ones:

1) Test_WP_REST_Key_Pair::test_get_rest_uri
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/wp-json/wp/v2/key-pair'
+'http://example.org/wp-json/wp/v2/key-pair'

IMHO using absolute URIs is the way to go (otherwise how would subdomain multi-site installs work?), so I'd like to update the test. However, I'd love an explicit 👍 from a reviewer on that approach.

@jkmassel jkmassel force-pushed the fix/manual-tokens-not-working-for-plain-permalinks branch 4 times, most recently from a744bef to 1d4ae72 Compare May 9, 2019 20:20
@jkmassel jkmassel force-pushed the fix/manual-tokens-not-working-for-plain-permalinks branch from 1d4ae72 to 21edd99 Compare May 9, 2019 20:49
@valendesigns
Copy link
Collaborator

valendesigns commented May 10, 2019

Hi @jkmassel sorry but I don't understand this PR. I currently have default permalinks set on the site I've been developing on, as a matter of fact, I have the entire time I've developed the plugin. I'm not sure I get what the intent is here because what you're saying is broken is working fine for me. I could only assume that there is something I'm either missing in the setup or you have a conflict from another active plugin. Could you explain the steps to reproduce the issue? I have /%postname%/ set as my permalink structure and I'm able to create a token at this endpoint /wp-json/wp/v2/token using postman and an API key/secret without any issue, what am I missing?

edit:
I think what you're after is that the plain permalink is not working when you add a new key-pair in the admin. I'll look at it now.

@@ -107,7 +111,16 @@ public function test_init() {
* @covers ::get_rest_uri()
*/
public function test_get_rest_uri() {
$this->assertEquals( '/wp-json/wp/v2/key-pair', WP_REST_Key_Pair::get_rest_uri() );
$this->assertEquals( get_rest_url( null, '/wp/v2/key-pair' ), WP_REST_Key_Pair::get_rest_uri() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

/wp/v2/key-pair is not a public API, it is meant for the user profile only.

@jkmassel jkmassel force-pushed the fix/manual-tokens-not-working-for-plain-permalinks branch from 5a350d8 to a06359a Compare May 10, 2019 04:37
@jkmassel
Copy link
Collaborator Author

I think what you're after is that the plain permalink is not working when you add a new key-pair in the admin. I'll look at it now.

Yep that's right – sorry about the confusion. Updated the initial PR description to hopefully prevent future confusion.

@jkmassel
Copy link
Collaborator Author

Closing in favour of #12

@jkmassel jkmassel closed this May 10, 2019
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.

Manually issuing tokens doesn't work if using default permalinks
2 participants