update post create/update by thrijith · Pull Request #129 · wp-cli/entity-command

thrijith · 2018-01-14T11:33:47Z

to fix wp-cli/wp-cli#4605
update the condition to set post category if name/slug is given in --post_category
work in progress

add condition to set post category if name/slug is given

update condition to set post category if name/slug is given

schlessera · 2018-01-15T06:59:12Z

@thrijith Can you provide the corresponding tests with the above change?

thrijith · 2018-01-15T10:56:55Z

ok @schlessera

update code and fix tests

gitlost · 2018-01-15T21:35:34Z

features/post.feature

+      Success: Updated post {POST_ID}.
+      """
+
+    When I run `wp post update {POST_ID} --post_category=test --porcelain`


If a command produces a warning or an error, it appears on STDERR, and need to use try instead of run:

When I try `wp post update {POST_ID} --post_category=test` Then STDERR should be: """ Warning: Unable to set test as post category """ And STDOUT should be: """ Success: Updated post {POST_ID}. """

(This is a real gotcha with the behat tests, and we should throw a reminder to use try when it happens as it's so easy to forget to do it...)

fix tests

schlessera · 2018-01-16T07:45:48Z

I'm wondering now whether the following behavior wouldn't be preferable:
If a category is provided as a string:

if a category term query for that string returns 0 IDs, throw an error that the category is unknown;
if a category term query for that string only returns 1 ID, use that ID instead;
if a category term query for that string returns more than 1 ID, throw an error that the category is ambiguous and that an ID is needed instead.

@gitlost Thoughts about the above? Goal would be to get around the WP limitation by pre-querying IDs to replace the strings.

schlessera · 2018-01-16T07:46:54Z

Oh, btw, @thrijith , good job so far on the pull request. It does exactly what the initial goal was. It's my fault that I didn't think sooner about trying to prefetch the IDs instead...

gitlost · 2018-01-16T11:09:11Z

@schlessera yes, agree that would be the preferable behavior.

gitlost · 2018-01-16T23:46:20Z

From @thrijith slack:

not sure i got it completely https://github.com/wp-cli/entity-command/pull/129#issuecomment-357878268
if a category term query for that string returns 0 IDs, throw an error that the category is unknown [done ?]
if a category term query for that string only returns 1 ID, use that ID instead [done ?]
if a category term query for that string returns more than 1 ID, throw an error that the category is ambiguous and that an ID is needed instead. [in what conditions will we get more than 1 ID ]

Yes you're right, category_exists() won't return more than 1 ID, only an int (if the category exists) or a falsey value (0 or null), so the code in the loop would be eg:

$post_category = is_numeric( $post_category ) && (int) $post_category ? (int) $post_category : $post_category;
$category_id = category_exists( $post_category );
if ( ! $category_id ) {
	WP_CLI::error( "No such post category '$post_category'." );
}
$category_ids[] = $category_id;

gitlost · 2018-01-17T00:10:15Z

Thinking about it this is probably better (as keep original passed-in $post_category to report in error):

if ( is_numeric( $post_category ) && (int) $post_category ) {
	$category_id = category_exists( (int) $post_category );
} else {
	$category_id = category_exists( $post_category );
}

etc.

update code according to review and modify tests

gitlost · 2018-01-18T19:05:32Z

features/post.feature

    When I try the previous command again
    Then the return code should be 1

+    When I run `wp term create category "First Category" --porcelain`


I think it'd be better to move the category stuff to its own scenario. Here's an iteration that also checks some BC behaviour (for details, see get_category_ids() return comment below):

Scenario: Setting post categories When I run `wp term create category "First Category" --porcelain` And save STDOUT as {TERM_ID} And I run `wp term create category "Second Category" --porcelain` And save STDOUT as {SECOND_TERM_ID} When I run `wp post create --post_title="Test category" --post_category="First Category" --porcelain` Then STDOUT should be a number And save STDOUT as {POST_ID} When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be: """ {TERM_ID} """ When I run `wp post update {POST_ID} --post_category={SECOND_TERM_ID}` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be: """ {SECOND_TERM_ID} """ When I run `wp post update {POST_ID} --post_category='Uncategorized,{TERM_ID},Second Category'` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` And save STDOUT as {MULTI_CATEGORIES_STDOUT} Then STDOUT should contain: """ {TERM_ID} """ And STDOUT should contain: """ {SECOND_TERM_ID} """ And STDOUT should contain: """ 1 """ # Blank categories with non-blank ignored. When I run `wp post update {POST_ID} --post_category='Uncategorized, ,{TERM_ID},Second Category,'` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be: """ {MULTI_CATEGORIES_STDOUT} """ # Zero category same as default Uncategorized (1) category. When I try `wp post update {POST_ID} --post_category=0` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be: """ 1 """ # Blank category/categories same as default Uncategorized (1) category. When I try `wp post update {POST_ID} --post_category=,` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be: """ 1 """ # Null category same as no categories. When I try `wp post update {POST_ID} --post_category=' '` Then STDOUT should be: """ Success: Updated post {POST_ID}. """ When I run `wp post term list {POST_ID} category --field=term_id` Then STDOUT should be empty # Non-existent category. When I try `wp post update {POST_ID} --post_category=test` Then STDERR should be: """ Error: No such post category 'test'. """ When I try `wp post create --post_title="Non-existent Category" --post_category={SECOND_TERM_ID},Test --porcelain` Then STDERR should be: """ Error: No such post category 'Test'. """ # Error on first non-existent category found. When I try `wp post create --post_title="More than one non-existent Category" --post_category={SECOND_TERM_ID},Test,Bad --porcelain` Then STDERR should be: """ Error: No such post category 'Test'. """

gitlost · 2018-01-18T19:06:53Z

src/Post_Command.php

+	 * @return array
+	 */
+	private function get_category_ids( $arg ) {
+		$categoires = explode( ',', $arg );


Typo $categoires -> $categories

gitlost · 2018-01-18T19:07:49Z

src/Post_Command.php

+	private function get_category_ids( $arg ) {
+		$categoires = explode( ',', $arg );
+
+		foreach ( $categoires as $post_category ) {


Need to init $category_ids = array(); before loop.

gitlost · 2018-01-18T19:08:35Z

src/Post_Command.php

+		$categoires = explode( ',', $arg );
+
+		foreach ( $categoires as $post_category ) {
+			if ( trim( $post_category ) !== '' ) {


Think leave the comparison out, to just have if ( trim( $post_category ) ) {

gitlost · 2018-01-18T19:09:33Z

src/Post_Command.php

+					$category_id = category_exists( $post_category );
+				}
+				if ( ! $category_id ) {
+					$category_ids[] = $post_category;


This $category_ids[] = $post_category; can be removed as about to error and not return.

gitlost · 2018-01-18T19:10:05Z

src/Post_Command.php

+				if ( ! $category_id ) {
+					$category_ids[] = $post_category;
+					WP_CLI::error( "No such post category '$post_category'." );
+				} else {


As WP_CLI::error() doesn't return, no need to else here.

gitlost · 2018-01-18T19:14:27Z

src/Post_Command.php

+				}
+			}
+		}
+		return $category_ids;


For BC with previous behaviour, we need to do:

// If no category ids found, return exploded array for compat with previous WP-CLI versions. return $category_ids ? $category_ids : $categories;

here.

The BC behaviour is weird and non-intuitive, in that --post_category=' ' will set no categories, while --post_category=0 will set the default category (see tests above), but probably we should keep this behaviour to be safe...??

@gitlost tests look really good.
so if valid categories are passed we will send the resolved ids
if a non-existent one is passed then throw an error ?

Thanks @thrijith !

so if valid categories are passed we will send the resolved ids
if a non-existent one is passed then throw an error ?

I think so, giving an error is a BC break but makes more sense than just warning...

gitlost · 2018-01-18T19:20:23Z

src/Post_Command.php

 	}
+
+	/**
+	 * Get category id if given name/slug


Maybe Resolves post_category arg into an array of category ids. or something better?

fix code according to review add more test cases mentioned in review

gitlost · 2018-01-18T22:11:38Z

Thanks @thrijith.

@schlessera could you give this a quick check before it's merged?

gitlost · 2018-01-23T15:08:19Z

Merged this anyway as @schlessera has got enough on his plate. Thanks @thrijith !

schlessera · 2018-01-23T15:09:36Z

I quickly looked through it and LGTM!

thrijith added 3 commits January 14, 2018 16:56

update post 'create/update'

5fb8944

add condition to set post category if name/slug is given

update post 'create/update'

be0e328

update condition to set post category if name/slug is given

update post 'create/update'

0554764

update condition to set post category if name/slug is given

thrijith changed the title ~~WIP: update post create/update~~ update post create/update Jan 14, 2018

schlessera added the command:post-create Related to 'post create' command label Jan 15, 2018

update post 'create/update'

e077559

update code and fix tests

thrijith changed the title ~~update post create/update~~ WIP: update post create/update Jan 15, 2018

gitlost reviewed Jan 15, 2018

View reviewed changes

update post 'create/update'

bb17c69

fix tests

update post 'create/update'

0b52cb5

update code according to review and modify tests

thrijith changed the title ~~WIP: update post create/update~~ update post create/update Jan 18, 2018

gitlost suggested changes Jan 18, 2018

View reviewed changes

update post 'create/update'

95f1414

fix code according to review add more test cases mentioned in review

gitlost approved these changes Jan 18, 2018

View reviewed changes

gitlost added this to the 1.2.0 milestone Jan 23, 2018

gitlost merged commit 2413aa3 into wp-cli:master Jan 23, 2018

Conversation

thrijith commented Jan 14, 2018

Uh oh!

schlessera commented Jan 15, 2018

Uh oh!

thrijith commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Jan 16, 2018

Uh oh!

schlessera commented Jan 16, 2018

Uh oh!

gitlost commented Jan 16, 2018

Uh oh!

gitlost commented Jan 16, 2018

Uh oh!

gitlost commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thrijith Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitlost commented Jan 18, 2018

Uh oh!

gitlost commented Jan 23, 2018

Uh oh!

schlessera commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thrijith Jan 18, 2018 •

edited

Loading