Automattic / ElasticPress

A fast and flexible search and query engine for WordPress.
https://elasticpress.io
GNU General Public License v2.0
20 stars 7 forks source link

Change delete term trigger #117

Closed pschoffer closed 3 years ago

pschoffer commented 3 years ago

Description of the Change

Deleting term needs to do 2 actions:

However, if the hook is invoked on delete_term the term is already deleted in WP by then. The operations needed to get the list of child terms then fails.

This change switches the two actions into 2 hooks. On pre_delete_term which happens both before the term is deleted but also before the child terms were updated in WP already, we will enqueue child terms to sync queue. Then on delete_term (same as before) we will remove the term from ES.

The sync queue is then processed on shutdown, that is after the children terms were already updated with the correct parent.

Another change in this PR is adding the permission check bypass for CLI and cron on term resyncing.

Test steps

1) dev-env exec -- wp elasticpress activate-feature terms 2) dev-env exec -- wp vip-search index --setup 3) Create some hierarchy of categories 4) Delete middle one `dev-env exec -- wp term delete category

5) No errors 5) Term is deleted 5) Check that index is correctly showing the children of deleted term pointing to the parent of deleted term as their parent now

rebeccahum commented 3 years ago

Just testing this and it looks like you don't even need to create a hierarchy of categories to trigger the below error:

Warning: array_merge(): Expected parameter 2 to be an array, object given in /wp/wp-content/mu-plugins/search/elasticpress/includes/classes/Indexable/Term/SyncManager.php on line 185 [example-site.vipdev.lndo.site/] [wp-content/mu-plugins/search/elasticpress/includes/classes/Indexable/Term/SyncManager.php:185 array_merge(), wp-includes/class-wp-hook.php:305 ElasticPress\Indexable\Term\SyncManager->action_sync_on_delete(), wp-includes/class-wp-hook.php:327 WP_Hook->apply_filters(), wp-includes/plugin.php:470 WP_Hook->do_action(), wp-includes/taxonomy.php:2072 do_action('delete_term'), phar:///usr/local/binvendor/wp-cli/entity-command/src/Term_Command.php:452 wp_delete_term(), Term_Command->delete(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php:100 call_user_func(), WP_CLI\Dispatcher\CommandFactory::WP_CLI\Dispatcher\{closure}(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/Subcommand.php:491 call_user_func(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:399 WP_CLI\Dispatcher\Subcommand->invoke(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:422 WP_CLI\Runner->run_command(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:1194 WP_CLI\Runner->run_command_and_exit(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php:23 WP_CLI\Runner->start(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/bootstrap.php:77 WP_CLI\Bootstrap\LaunchRunner->process(), phar:///usr/local/binvendor/wp-cli/wp-cli/php/wp-cli.php:27 WP_CLI\bootstrap(), phar:///usr/local/binphp/boot-phar.php:11 include('phar:///usr/local/binvendor/wp-cli/wp-cli/php/wp-cli.php'), /usr/local/bin/wp:4 include('phar:///usr/local/binphp/boot-phar.php')]
pschoffer commented 3 years ago

Upstream PR: https://github.com/10up/ElasticPress/pull/2349