Change delete term trigger

[pschoffer]

Description of the Change

Deleting term needs to do 2 actions:

• Delete the term
• Reindex all the child terms

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

6. Term is deleted

7. Check that index is correctly showing the children of deleted term pointing to the parent of deleted term as their parent now

[rebeccahum]

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')]

[rebeccahum]

Is pre_delete_term too early of a hook?

Would it be better to use a later hook such as deleted_term_taxonomy hook which fires right after the deletion occurs to ensure that the term is actually deleted on the DB level right before ES deletes on its side? https://github.com/WordPress/WordPress/blob/243e75eb416657464870ea9092c8750b7221b8fc/wp-includes/taxonomy.php#L2039-L2046

[pschoffer]

That was my initial thought too, but that hook happens after the child terms were reassigned to another parent. So say you have this scenario if using deleted_term_taxonomy:

1. A <- B <- C
2. B is being deleted
3. pre_delete_term is executed (we do nothing)
4. C is assigned as a child to A
5. deleted_term_taxonomy is executed. We look up B's children and find nothing.
6. Delete B
7. END (C was not scheduled for reindexing)

vs pre_delete_term

1. A <- B <- C
2. B is being deleted
3. pre_delete_term is executed. We look up B's children and find C.
4. C is assigned as a child to A
5. deleted_term_taxonomy is executed. We do nothing
6. Delete B
7. END
8. On shutdown we will reindex C.

I guess another solution is to split the two things into two hooks do the "lookup term being deleted children and queue them up" on pre_delete_term and leave "delete the term" on delete_term. What do you think @rebeccahum ?

[rebeccahum]

Oh, good point with the nesting hierarchies!

Breaking it up into two separate hooks seems like a good idea to me.

[pschoffer]

Done.

[rebeccahum]

What are your thoughts on checking if ep_sync_insert_permissions_bypass is applied as well instead of completely removing the permissions check? I'm seeing that it's hooked onto filter_bypass_permission_checks_for_machines to account for CLI: https://github.com/10up/ElasticPress/blob/e3639bbcdec9efdc4680162563bc215c85c6e6bb/includes/classes/Indexable/Post/SyncManager.php#L68.

[pschoffer]

Yeah there is ep_sync_delete_permissions_bypass as well. I would say it is a plan B on push back :) But I don't really see the benefit of doing permission checking at this state....oooh now I do. I guess anybody's evil plugin could invoke the delete_term hooks not just core WordPress? So even if we are not really deleting the term.

But then again, we are not deleting anything here. We are just saying please update these items to what they really are in DB.

[pschoffer]

I re-added the check now. Thanks for the input.

[rebeccahum]

LGTM! 🚢

[pschoffer]

Upstream PR: 10up#2349