Sync command stop-on-error flag #3500
Conversation
oscarssanchez
changed the title
|
@oscarssanchez if I add a snippet like this in a mu-plugin and run the sync, it will only stop if --show-errors is also present: add_filter(
'ep_prepare_meta_data',
function( $post_meta, $post ) {
for ( $i = 0; $i < 100; $i++ ) {
$post_meta[ "test_meta_{$i}_title_{$post->ID}" ] = 'Lorem';
$post_meta[ "test_meta_{$i}_body_{$post->ID}" ] = 'Ipsum';
}
return $post_meta;
},
10,
2
);Is that intended? |
tests/php/TestCommands.php
Outdated
| /** | ||
| * Test sync command with stop-on-error flag. | ||
| * Expect an error message that stops the sync instead of a warning. | ||
| */ |
There was a problem hiding this comment.
@oscarssanchez can we add a @since tag here please?
tests/php/TestCommands.php
Outdated
| * Test sync command with stop-on-error flag. | ||
| * Expect an error message that stops the sync instead of a warning. | ||
| */ | ||
| public function testSyncStopOnError() { |
There was a problem hiding this comment.
Not a blocker but for newer tests we are using snake-case for test names. Do you mind renaming it to test_sync_stop_on_error()? We'll have a follow up issue to rename all test methods.
| add_filter( | ||
| 'http_response', | ||
| function( $request ) { | ||
| $fake_request = json_decode( wp_remote_retrieve_body( $request ) ); | ||
|
|
||
| if ( ! empty( $fake_request->items ) ) { | ||
| $fake_request->errors = true; | ||
|
|
||
| $fake_item = new \stdClass(); | ||
| $fake_item->index = new \stdClass(); | ||
| $fake_item->index->error = new \stdClass(); | ||
| $fake_item->index->status = 400; | ||
| $fake_item->index->_id = 10; | ||
| $fake_item->index->type = '_doc'; | ||
| $fake_item->index->_index = 'dummy-index'; | ||
| $fake_item->index->error->reason = 'my dummy error reason'; | ||
| $fake_item->index->error->type = 'my dummy error type'; | ||
|
|
||
| $fake_request->items[0] = $fake_item; | ||
|
|
||
| $request['body'] = wp_json_encode( $fake_request ); | ||
|
|
||
| return $request; | ||
| } | ||
|
|
||
| return $request; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Instead of faking the return via a filter, it would be faster to mock the remote_request response (similar to what I did here: https://github.com/10up/ElasticPress/blob/develop/tests/php/TestElasticsearch.php#L156) Do you mind making that change?
tests/php/TestCommands.php
Outdated
| Indexables::factory()->get( 'post' )->delete_index(); | ||
|
|
||
| $this->ep_factory->post->create_many( 10 ); |
There was a problem hiding this comment.
With the above, I think this can go away. If mocking the method return is not doable, can we avoid deleting the index at least?
tests/php/TestCommands.php
Outdated
| 'setup' => true, | ||
| 'show-errors' => true, | ||
| 'stop-on-error' => true, | ||
| 'yes' => true, |
There was a problem hiding this comment.
Just to make sure it already works with the minimal case, can we remove the setup, show-errors, and the yes flags?
…ress into feature/stop-on-errors-cli
Description of the Change
This PR introduces a new flag
stop-on-errorto stop sync the moment we find an error. We accomplish this by returning an error logged byoutput()that later kills the sync.Closes #2000
How to test the Change
Changelog Entry
Credits
Props @oscarssanchez
Checklist: