Alternate taxonomy archive migration approach - #97
Closed
dlh01 wants to merge 6 commits into
Closed
Conversation
The section name should use `$prefix` to match how settings are registered. Also, use the 4.5+ arguments array to `get_terms()`, use a regex to ensure we replace prefixes from only the start of a string, and avoid conflating a `WP_Error` response with an array of terms.
Developers currently bear the burden of understanding the 'wp_seo_should_taxonomy_migration_run' filter and whether they need to run it, even though most installations won't need to and the plugin provides no way of showing the version when it was installed. There's also a possibility of data loss if a site opts in to the filter but updates the plugin option from somewhere outside the settings page, since the sanitization routine assumes the submitted data already uses the 'taxonomy_' prefix when the filter is enabled. These changes replace the filter with a new internal settings key that stores the taxonomy archive prefix itself (cutting out the middleman of inferring the key based on the migration status). As with before, the prefix is set to 'taxonomy' only when `get_option()` returns the default option. The result fixes the underlying bug for new installs but puts off creating a migration path for existing installations, perhaps pending a WP-CLI command or something similarly interactive. Additionally: - Prevents the default internal settings from editing via the `wp_seo_default_options` filter. - In true WordPress fashion, prefixes the internal settings key with `_`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@davisshaver I would value your feedback on this one. It replaces the filter for opting-in to the taxonomy migration; instead, it just fixes the bug for new installs and punts on providing a migration path. Also incorporates #93.
I think your suggestion in State that new installs do not need to filter #86 both is correct and highlights how difficult it is to communicate to developers whether they need to care about the change. The plugin doesn't even provide a way to tell you the version when you installed it. So, for now, the change would be only behind-the-scenes.
There is the potential for data loss in the existing implementation in that it assumes data going into
sanitize_options()is coming from the settings page with updated field names. Butsanitize_options()will run on any call to(update|add)_option()once the setting is registered, and settings would be be lost if thearchive_keys were submitted.2748e2c has a related bugfix and some tweaks.