Yoast / wordpress-seo

Yoast SEO for WordPress
https://yoast.com/wordpress/plugins/seo/
Other
1.77k stars 893 forks source link

14.x [database] - wp-cli search-replace fails #15096

Open Djennez opened 4 years ago

Djennez commented 4 years ago

Original issue: https://wordpress.org/support/topic/search-replace-doesnt-change-urls-that-are-already-indexed/

It looks like our new database tables yoast_indexable, yoast_indexable_hierarchy, yoast_migrations and yoast_primary_term are not (correctly) registered with WordPress. This means that the yoast_indexable table gets skipped with, for example, wp search-replace. I know from experience that this command gets used a lot in site migrations and cloning between development / staging and live environments. Seeing as we store a lot of useful data regarding URI's in those tables, I think we should consider registering at least the yoast_indexable table.

As a workaround it is possible to use wp search-replace '<search>' '<replace>' wp_yoast_* --all-tables-with-prefix to make the search-replace work, but a lot of this commands usage is in scripts where the arguments are set.

To reproduce:

  1. Have a WP installation with Yoast and have some indexables in the database
  2. Perform the following WP-CLI command: wp search-replace '0' '1' --dry-run
  3. Observe that the new tables are not listed in the column mentioning the tables that have been searched through
  4. Perform the following WP-CLI command: wp search-replace '0' '1' --dry-run --all-tables
  5. Observe the tables now being output correctly

From https://developer.wordpress.org/cli/commands/search-replace/ :

[--all-tables] Enable replacement on ALL tables in the database, regardless of the prefix, and even if not registered on $wpdb. Overrides –network and –all-tables-with-prefix.

herregroen commented 4 years ago

One thing that should be taken into consideration here is that our URLs have a permalink hash attached to them to allow an index on them.

Replacing the URL wouldn't automatically replace that hash. We should give thought on how to implement this. It may even be preferable to simply tell people to call wp yoast index --reindex afterwards instead.

So if there's a filter or action on this command that would allow us to recalculate the hashes this could work. Otherwise current behavior may be preferable.

MWDelaney commented 4 years ago

I can confirm this issue on a recent site migration. Thanks @Djennez for making this issue easy to find!

skip405 commented 4 years ago

Guys, any news on this? Tried the solution suggested by @herregroen but got an error about the unknown --reindex flag. Running just wp yoast index as part of the deployment process doesn't seem to change things either.

stodorovic commented 4 years ago

@skip405 I think that version 14.0 doesn't support --reindex flag. Did you update Yoast SEO to the latest version (14.2)?

skip405 commented 4 years ago

@stodorovic nope, we haven't. We had to roll back to v.13.5 and decided it was better to stick with it for the time being. We'll give v.14.2 a try though, but still waiting for the resolution of this issue.

skip405 commented 3 years ago

Hi folks, forgot to mention that the proposed solution works, although it does seem like a pretty costly task to run. We see a noticeable increase in deployment time on sites with more than 1k of posts. Will reindexing remain as the solution or are there plans for something else?

skip405 commented 3 years ago

Another update on this is that the proposed solution doesn't seem to work on Multisite installation. Neither wp yoast index --reindex nor wp yoast index --reindex --network seem to work for all of the blogs, just the main one.

skip405 commented 3 years ago

OK, if anyone else should have same issue the reason is that by default the reindex flag prompts for a user confirmation for each site, so running something like echo 'y' | wp yoast index --reindex does work for the initial prompt, but not for all of the sites.

There's the skip-confirmation flag that helps, couldn't find it in the documentation though, only in the source code. So for a multisite installation without any prompts the command is wp yoast index --reindex --network --skip-confirmation.

georgestephanis commented 3 years ago

I'm coming at this from zero, unsure of precisely how the hashes generated are used, but part of me wonders if it would be possible to optionally allow search-replace to work on these tables, but more critically once it detects that a search-replace has run (and possibly stored somewhere what search/replace it was and how many changes impacted the index tables), then display a ui prompt wp-admin side letting a site admin know that a reindex is required as the indices are out of date.

Bonus points if we could store the impacted rows in the index tables (assuming it's not literally all of them) and only regenerate those rows for the tables.

ssnepenthe commented 3 years ago

I just spent longer than I'd like to admit trying to figure out why the canonical tag on my site was using the .test tld from my dev environment. It was made more difficult by the fact that this also affects the db search command (and really anything that uses WP_CLI\Utils\wp_get_table_names()).

So if there's a filter or action on this command that would allow us to recalculate the hashes this could work. Otherwise current behavior may be preferable.

This seems like a pretty big deal to just stick with current behavior. It was luck that I caught this when I did, otherwise who knows how long I might have left my site with canonical URLs pointing to a non-existent site. Shouldn't custom tables always be registered properly with WordPress?

A quick scan through the search replace command source doesn't turn up any filters or actions...

WP-CLI itself has the after_invoke:<command> hook. At a minimum you could use this hook to display a warning to users after running search-replace that they might have to run yoast index --reindex.

Other than that, maybe you can use the core query filter to monitor for UPDATE queries to plugin-specific tables and trigger a reindex based on that?

wal-f commented 1 year ago

Version 20.7 now and this issue is still stinging us.