fix basic auth; fix listing lists; error handling #92

diablodale · 2017-02-12T05:27:53Z

fix basic auth
api index and therefore the routes wouldn't load because code didn't pass auth as needed at that stage

fix lists
when using the command "list" the code was not handling some lists being returned from WP 4.7.2. For example:

wp-cli --http=https://user:password@server.domain.com rest settings list
wp-cli --http=https://user:password@server.domain.com rest status list

error handling of outputs
Added error handling of outputs. E.g., if an error occurs, then --porcelain does not output data.

Dev tested against WP 4.7.2 on Ubuntu 16.04 with PHP 7.0.13

- updated two strings in the CI test due to the deprecation of the REST plugin and the new core WP 4.7x functionality. With it, two strings changed.

danielbachhuber

Thanks for the pull request!

Some minor issues to fix up. I'd also generally feel much better about this pull request if there were additional tests being added.

danielbachhuber · 2017-02-14T01:29:05Z

inc/RestCommand.php

@@ -61,9 +61,15 @@ public function set_auth( $auth ) {
 	public function create_item( $args, $assoc_args ) {
 		list( $status, $body ) = $this->do_request( 'POST', $this->get_base_route(), $assoc_args );
 		if ( Utils\get_flag_value( $assoc_args, 'porcelain' ) ) {
-			WP_CLI::line( $body['id'] );
+			if ( $status < 400 ) {


Can you add a test for this change?

It'd probably be good to explicitly check for $body['id'] being set too

Caveat, I am learning Travis on the fly. Might see several commits as I figure out how to use it.
I found examples where the REST api returns list results with no "id". I wasn't able to reverse-out the spec on what to do in those situations. So in this case, what is the spec/rule? Here's the scenario

user types command line to create an item

code calls a REST api to create item. code expects an id to be returned

code receives an http result code < 400 yet no id
What is the output for the case with and without porcelain?

It is possible for REST to return http=200, a body, and no id. So that means the server did...something. I intentionally left the code in a manner that errors will occur so that the admin can see something unexpected happened. Think of porcelain, in that usage I would output nothing. A user then knows the create failed yet has no diagnostics yet the server did something. I didn't like that scenario so I was allowing the code to fault so it could provide diagnostics.
But...what do you think. What is the approach/spec on this? For both porcelain and without?

Would you please add apache to the travis config? It would likely much churn for me to get that working. The docs to do it are at https://docs.travis-ci.com/user/languages/php/#Apache-%2B-PHP
Why? None of the test cases use the --http code path. They are all calling locally. The code paths in restful are significantly different between them. And the results coming back from REST are sometimes different. I encountered this during my testing and handled the differences in code.
With apache in place, I can add test cases which exercise the new PRs I'm making. I'll want to use localhost similar to the following wp-cli --http=username:password@localhost rest

Caveat, I am learning Travis on the fly. Might see several commits as I figure out how to use it.

You should be running the tests locally. Here's the appropriate documentation: https://make.wordpress.org/cli/handbook/pull-requests/

But...what do you think. What is the approach/spec on this? For both porcelain and without?

Porcelain is meant to only ever output the result of the operation that can be piped to another command. I think we should continue with this behavior. If RESTful WP-CLI sometimes doesn't return output, the exit code should indicate whether it was an error or not.

Would you please add apache to the travis config? It would likely much churn for me to get that working.

You're welcome to work on it in a separate pull request. I don't have the bandwidth to handle this immediately.

Related #10

error codes now set in exit code and/or error text. Put error code at end of line without trailing period to make it easier for parsing.

danielbachhuber · 2017-02-14T01:29:31Z

inc/RestCommand.php

 		} else {
-			WP_CLI::success( "Created {$this->name} {$body['id']}." );
+			if ( $status < 400 ) {
+				WP_CLI::success( "Created {$this->name} {$body['id']}." );


Let's also check that $body['id'] is set before attempting to display it.

danielbachhuber · 2017-02-14T01:30:27Z

inc/RestCommand.php

+			if ( $status < 400 ) {
+				WP_CLI::success( "Created {$this->name} {$body['id']}." );
+			} else {
+				WP_CLI::error( 'Could not complete request.' );


Let's include the HTTP code in this error message:

WP_CLI::error( "Could not complete request (HTTP code {$status})" );

error codes now set in exit code and/or error text. Put error code at end of line without trailing period to make it easier for parsing.

danielbachhuber · 2017-02-14T01:31:39Z

inc/RestCommand.php

 				}
+				WP_CLI::error( 'Could not complete request.' );


It'd be good to include the http code here too (similar to above).

error codes now set in exit code and/or error text. Put error code at end of line without trailing period to make it easier for parsing.

danielbachhuber · 2017-02-14T01:32:07Z

inc/RestCommand.php

 			} else {
-				WP_CLI::success( "Deleted {$this->name} {$body['id']}." );
+				WP_CLI::error( 'Could not complete request.' );


Same as above.

error codes now set in exit code and/or error text. Put error code at end of line without trailing period to make it easier for parsing.

danielbachhuber · 2017-02-14T01:32:51Z

inc/RestCommand.php

@@ -145,12 +165,30 @@ public function get_item( $args, $assoc_args ) {
 				'api_url'     => $this->api_url,
 			) );
 		} else {
+			if ( empty( $body ) ) return;


Coding standards: return should be on a new line, and surrounded by braces.

danielbachhuber · 2017-02-14T01:33:17Z

inc/RestCommand.php

+			if ( $status < 400 ) {
+				WP_CLI::success( "Updated {$this->name} {$body['id']}." );
+			} else {
+				WP_CLI::error( 'Could not complete request.' );


Same as above.

danielbachhuber · 2017-02-14T01:33:42Z

inc/RestCommand.php

@@ -428,7 +478,7 @@ private function do_request( $method, $route, $assoc_args ) {
 			$body = json_decode( $response->body, true );
 			if ( $response->status_code >= 400 ) {
 				if ( ! empty( $body['message'] ) ) {
-					WP_CLI::error( $body['message'] . ' ' . json_encode( array( 'status' => $response->status_code ) ) );


Why'd you remove this?

I didn't. It is still there. There was only a change to no longer put the status at the end so that it matched the output of your test cases.
If you want to change the spec on output, then the code and the test cases need to change. I would recommend that a consistent manner of reporting http status be chosen. In the old codebase, it literally shows the JSON. In your PR feedback above, you want it be not show JSON and be formatted. That would cause inconsistencies in http error code output which I would discourage.

danielbachhuber · 2017-02-14T01:34:09Z

inc/Runner.php

-			return false;
-		}
+		if ( $response->status_code >= 400 ) return false;
+		if ( empty( $response->headers['link'] ) ) return false;


Coding standards

danielbachhuber · 2017-02-14T01:34:31Z

inc/Runner.php

 		}
+		$response = Utils\http_request( 'GET', $api_url, null, $headers );
+		if ( $response->status_code >= 400 ) return false;
+		if ( empty( $response->body ) ) return false;


Coding standards

danielbachhuber · 2017-02-14T01:37:41Z

@diablodale Out of curiosity, how are you using this package? Testing it out, or for a project in the real world? And, what do you think about its utility generally?

diablodale · 2017-02-14T12:42:42Z

I'm using wp-cli in AWS. Specifically, in a CloudFormation template used to create my website/eStore infrastructure. I was using just wp-cli but found a scenario where I couldn't do the work "local" due to process access/ownership of my cache. I needed to call to the webserver so that normal handling of input would correctly update the cache. When I saw the restful package, I started testing its usage and found many list scenarios were not working. Which led to this PR.
With WP core committed to REST, I do think a command line tool to manipulate that API is needed. There is/will be people that want to automate WP activities at the command-line. I do not know the number of people.

…dling - on http error results (400+) put it in the exit code and in error text - check for id field before trying to display it in output - handle result objects that do/not have a context array field no field = show field in output empty array field = skip showing field array with values = show field if a value matches requested contexts - minor formatting changes in source code

- if rest route leads to a collect and has a DELETE method on it, the code now supports a command to purgeall on that - /me is returned by WP core on users; the code does not support parameters already within a route, therefore a workaround was made to ignore routes ending in /me

danielbachhuber · 2017-02-16T23:41:18Z

inc/RestCommand.php

@@ -188,8 +254,14 @@ public function list_items( $args, $assoc_args ) {
 				'api_url'     => $this->api_url,
 			) );
 		} else {
+			if ( empty( $items ) ) return;


This should be:

if ( empty( $items ) ) { return; }

- aligning more error messages to have error codes and exit codes - small refactor in register_route_commands() where id is forced change prevents duplicate id parameters while also only forcing id in cases where id is a parameter via the schema - updated two test cases for the new error message format and exit codes

- if a route has named path variables, then those names are marked required and positional within the set of arguments - removed use of 'id' as a hardcoded key field in register_route_commands() since that does not work with all routes (e.g. taxonomy). Now the code respects the required arguments including the above path vars - removed hardcoded edit command params to use above code - insert debug warning that routes with 2 or more path vars will not yet function. This work is TBD.

- support multiple positional arguments via route named path variables, e.g. post-revisions and page-revisions - updated test cases to exercise new code above - updated behat "save STDOUT as..." for nicer layout with RegEx - added behat version string comparison, used to check WP version - removed install of the deprecated wp-rest plugin; test cases now check that it is at least WP 4.7 - optional environ var RESTFUL_TEST_DBHOST can be used to specify a host for the db

danielbachhuber · 2017-02-21T21:07:43Z

features/comment.feature

@@ -2,7 +2,8 @@ Feature: Manage WordPress comments through the REST API

  Background:
    Given a WP install
-    And I run `wp plugin install rest-api --activate`
+    When I run `wp core version`
+	Then STDOUT should be a version string >= 4.7


Minor nit: it looks like your indentation is off here.

Bigger question: Why is testing the version important?

Because the REST api is not part of the core WP codebase until version 4.7. I want to ensure that we have WP 4.7 or greater. These test cases will fail otherwise.

Because the REST api is not part of the core WP codebase until version 4.7. I want to ensure that we have WP 4.7 or greater.

I think this is an assumption we don't need to test for. The test suite downloads the latest version of WordPress behind the scenes. We can assume we're on the latest version of WordPress (and that we have a working database, etc.).

I disagree. The test cases are separate from travis. And separate from WP installs. The cases a behat and run separately (like I've done forever since I started using these test tools).
Behat docs suggest that the background givens ensure that the environment is setup as expected. It is not guaranteed (since these behat cases are separate) that WP is any specific version. Therefore, the test for the specific version.
Since this only makes the test cases stronger, I can't understand why you would not allow this to remain. It is not enough to just have a WP install. It must be a WP 4.7+ install.

Since this only makes the test cases stronger, I can't understand why you would not allow this to remain. It is not enough to just have a WP install. It must be a WP 4.7+ install.

This specific comment isn't a strongly held opinion. However, I must point you to my previous comment on this pull request:

At this point, I'm not confident in accepting the entire pull request. Some changes make sense, and others need more consideration. I'd like for it to be broken into logical subparts, so that each piece can be evaluated on its own.

Of course. The pull request is for your benefit and those that use restful at large. I will continue to use my restful fork until some future where you reach my functionality level or we merge. I have a well working fork and my needs as such that I won't wait or lessen functionality. That's one of the benefits of forks. :-)

Ok, that's fine. I'll pull code out of your pull request when I have some bandwidth for it.

danielbachhuber · 2017-02-21T21:13:32Z

features/comment.feature

      """
+    Then the return code should be 145


I'm still not sure how I feel about using these elevated return codes (WP-CLI still uses 0 vs 1). It doesn't need to block the PR though; we can discuss separately #96

comments in #96

danielbachhuber · 2017-02-21T21:13:44Z

features/load-rest.feature

@@ -2,7 +2,8 @@ Feature: Manage WordPress through endpoints locally

  Background:
    Given a WP install
-    And I run `wp plugin install rest-api --activate`
+	When I run `wp core version`
+	Then STDOUT should be a version string >= 4.7


Indentation.

I see no issue all tests pass. Perhaps this is a text/tab oddity

I see no issue all tests pass. Perhaps this is a text/tab oddity

Behat .feature files require spaces, not tabs. We don't have a linter in place right now.

behat doesn't require spaces. It only requires that related indentation be the same number of chars. If you are suggest this is a coding-style rule, then the coding style rules need to be updated to indicate that tabs are disallowed in xxx, xxx, xxx set of files. The php rules at https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation declare tabs should be used. Since this is the primary language used for wp-cli developers, it seems odd to me to change the indentation rule which then creates an additional burden on devs and reviewers both. Consistency helps efficiency and decreases error rates.

If you are suggest this is a coding-style rule, then the coding style rules need to be updated to indicate that tabs are disallowed in xxx, xxx, xxx set of files.

See https://github.com/wp-cli/restful/blob/master/.editorconfig#L16

The php rules at https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation declare tabs should be used.

.feature != .php

I've filed an issue to include a linter in the future wp-cli/scaffold-package-command#82

sounds good. tools and automation to help devs is usually a great idea. Uuusually... ;-)

A caution: this .editorconfig file is useless to me. It is also a hidden requirement and functionality as it is not documented in the coding standards. My editors (e.g. VS Code, notepad++, and vi) do not use such a file. On searching now, it is possible to install a 3rd party extension in VS Code for it to work. However, if that is needed, then such needs to be documented in the coding standards.

danielbachhuber · 2017-02-21T21:16:24Z

features/steps/given.php

@@ -102,7 +102,7 @@ function ( $world, TableNode $table ) {
 	}
 );

-$steps->Given( '/^save (STDOUT|STDERR) ([\'].+[^\'])?as \{(\w+)\}$/',
+$steps->Given( '/^save (STDOUT|STDERR) ([\'].+[^\'])?\s?as \{(\w+)\}$/',


This source of this code is WP-CLI's test suite. Want to submit a PR there too? https://github.com/wp-cli/wp-cli/blob/master/features/steps/given.php#L112

Related wp-cli/wp-cli#3826

danielbachhuber · 2017-02-21T21:16:56Z

features/steps/then.php

@@ -145,6 +145,16 @@ function ( $world, $stream ) {
 	}
 );

+$steps->Then( '/^(STDOUT|STDERR) should be a version string (<|<=|>|>=|==|=|!=|<>) ([+\w\.-]+)$/',


This will eventually want to end up in WP-CLI's test suite, if we deem it a necessary test to keep.

issue opened, fixed, and soon PR

danielbachhuber · 2017-02-21T21:18:06Z

inc/RestCommand.php

+	 *
+	 * @subcommand delete
+	 */
+	public function purgeall_items( $args, $assoc_args ) {


Did you mean to include this?

Yes, the text comment should read purgeall instead. I have updated it.

danielbachhuber · 2017-02-21T21:28:33Z

@diablodale First of all, I sincerely appreciate the effort you've put into this pull request. Please don't take my comments as disrespect to the time you've spent.

It's getting harder to approve the pull request because of the amount of code that's changed. While this isn't necessarily a bad thing at this stage (the package probably isn't in production use many places), I'm concerned about inheriting decisions, bugs, etc. that we have to live with indefinitely.

To make it easier to approve the changes going in, would you be willing to separate the pull request into logical subparts? We could start with the changes required to get the tests passing again, then basic auth, and then the new features you want to add separately.

diablodale · 2017-02-21T22:09:48Z

All tests pass as is. No failures.
I recommend taking the current set of commits as a whole. They have bug fixes throughout as I made needed corrections to the scaffolding. What you and others wrote was good; it provided a base from which I could get it working. As I continued my testing, I made the fixes.

The "contract" is the output. I have not taken away features. Instead, I fixed existing ones and enabled intended ones (like 2+ path param endpoints). So...by looking at the changes to the test cases, you can see where things would change for users on the internet. Other things I implemented they couldn't be using...because they didn't work before this set of commits.

click over on the tab and you can see what changed in the 3 test files. In short, it is a change in the capitalization of "x-wp-totalpages" and a change to the error text. Everything else is the same.

danielbachhuber · 2017-02-21T22:18:57Z

The "contract" is the output. I have not taken away features. Instead, I fixed existing ones and enabled intended ones (like 2+ path param endpoints).

I understand this. However, the policy is to open a new issue to discuss features and, generally, to keep to one feature per pull request.

At this point, I'm not confident in accepting the entire pull request. Some changes make sense, and others need more consideration. I'd like for it to be broken into logical subparts, so that each piece can be evaluated on its own.

diablodale · 2017-02-21T22:20:43Z

Oh, and there are bugs still in the codebase areas I touched. None that affect the functionality that I'm using and the basic functionality that I enabled. Some need discussion, like colliding param names. Some are cosmetic and will live to another day when someone has time to address them.

If you find bugs which affect the direct functionality that I fixed or enabled, then YES I want to isolate and fix them. That's my responsibility as I'm the source of them.

I am now writing the custom endpoint which is then exposed with the restful functionality that I enabled. It is a good breaking point, a way to segment that work (which I've already started) from the work in this PR and batch of commits.

diablodale · 2017-02-21T22:22:08Z

Of course your choice as it is your repro. I'll continue in my fork and when you want to collaborate, please loop me in.

…l#92

Dale Phurrough added 4 commits February 12, 2017 04:41

fix basic auth; fix listing lists

0673219

fix dup lists due to missing else; fix simple list case regression

3803911

error handling command output; url protocol fixes for --http

6425bbc

update CI test for new WP 4.7x text

f089f17

- updated two strings in the CI test due to the deprecation of the REST plugin and the new core WP 4.7x functionality. With it, two strings changed.

diablodale changed the title ~~fix basic auth; fix listing lists~~ fix basic auth; fix listing lists; error handling Feb 12, 2017

handle no result case with lists

17455d4

danielbachhuber requested changes Feb 14, 2017

View reviewed changes

Dale Phurrough added 2 commits February 14, 2017 19:34

danielbachhuber reviewed Feb 16, 2017

View reviewed changes

danielbachhuber requested a review from schlessera February 16, 2017 23:42

Dale Phurrough added 7 commits February 17, 2017 01:45

code layout tweak

6c7cb56

corrected past coding standards to use 'elseif' not 'else if'

b2b02f2

one line tweak for PHP 5.3 compatibility; added PHP 7 to travis

e376bf0

tweak for php 5.3: array() instead of []

f1279b3

danielbachhuber mentioned this pull request Feb 21, 2017

Strongly consider the implications of using HTTP status headers as return codes. #96

Closed

danielbachhuber requested changes Feb 21, 2017

View reviewed changes

danielbachhuber removed the request for review from schlessera February 21, 2017 22:19

danielbachhuber mentioned this pull request Feb 22, 2017

Add a linter for Behat .feature files wp-cli/scaffold-package-command#82

Closed

tweak comment

358bb3a

Dale Phurrough added 2 commits February 22, 2017 21:48

update db host testing env var to be same as that in wp-cli repro

966a31f

updated license information; might broaden it later

c22011e

danielbachhuber closed this Feb 23, 2017

diablodale added a commit to diablodale/restful that referenced this pull request Feb 23, 2017

Merge branch 'develop' which is all code submitted into wp-cli/restfu…

701c1bb

…l#92

fix basic auth; fix listing lists; error handling #92

fix basic auth; fix listing lists; error handling #92

Conversation

diablodale commented Feb 12, 2017 • edited

danielbachhuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diablodale Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Feb 14, 2017

diablodale commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Feb 21, 2017

diablodale commented Feb 21, 2017

danielbachhuber commented Feb 21, 2017

diablodale commented Feb 21, 2017

diablodale commented Feb 21, 2017

diablodale commented Feb 12, 2017 •

edited

diablodale Feb 14, 2017 •

edited