Add REST API #8
Add REST API #8
Conversation
|
|
pierlon
force-pushed
the
enhancement/28-unsplash-proxy
branch
from
62ab71d
to
1b8d1a0
Compare
|
Can you make sure that travis test pass before I review. |
|
Based on my feedback here I have created a PR see #10 |
php/RestAPI/RestController.php
Outdated
| * @param \Exception $e Exception. | ||
| */ | ||
| private function log_error( \Exception $e ) { | ||
| $message = sprintf( |
There was a problem hiding this comment.
We could even intergrate with stream.
There was a problem hiding this comment.
I'm not a fan of that function. Any better/simpler solutions are appreciated.
|
@derekherman The |
|
|
pierlon
force-pushed
the
enhancement/28-unsplash-proxy
branch
from
9fdd854
to
0325121
Compare
pierlon
force-pushed
the
enhancement/28-unsplash-proxy
branch
2 times, most recently
from
3aca14f
to
241c390
Compare
| * @param string $path URL path. | ||
| * @return string Route path. | ||
| */ | ||
| private function get_route( $path = '' ) { |
There was a problem hiding this comment.
Why would this need to be private?
There was a problem hiding this comment.
Is there a get_route method in the RestController class that this is mimicking?
There was a problem hiding this comment.
No, its just a helper method for the test class. No specific reason as to why its private.
There was a problem hiding this comment.
Typically if we need to duplicate something by creating helper functions then it's probably better to create a method in the class not the test class.
| @@ -0,0 +1,34 @@ | |||
| <?php | |||
There was a problem hiding this comment.
@pierlon I'd assume that the tests would be integration tests not unit. What is the reasoning behind mocking instead of creating actual tests that load WP?
There was a problem hiding this comment.
No reason in particular. Just wanted to add a test to ensure the actions in init() were being called. Writing tests for the other functions would be in vain since they will most likely change very soon.
There was a problem hiding this comment.
I could add an integration test as well (and/or remove the unit test) if needed.
|
@pierlon I've added a bit more to the settings class to encrypt and decrypt the stored values. Could you please save them in the settings page and test all is working. |
php/Settings.php
Outdated
| } | ||
| } | ||
| return $settings; | ||
| return $sanitized_settings; |
There was a problem hiding this comment.
We could be adding additional keys to the array, so this is overlooking that use case.
There was a problem hiding this comment.
I'd assume we'd want to validate those key-value pairs as well, and not just pass in the settings we received from the user.
There was a problem hiding this comment.
We would add that in when we know more. The logic was broken anyhow as it would clear out values on re-save.
There was a problem hiding this comment.
Oh yea you're right 😅 , that's on me
| * | ||
| * @covers ::access_key_render() | ||
| */ | ||
| public function test_access_key_render() { |
There was a problem hiding this comment.
Methods like this are testing output and not functionality which is less useful. We can leave it but I suspect these fields will be changing anyhow.
This PR aims to provide a foundation for the WP REST API and the extensibility of Unsplash's API.
To have the API working correctly, copy the
.env.exampleto.envand update it with the specified Unsplash credentials.Routes:
Todo:
Routerclass