Harmonize module options #1158
Harmonize module options #1158
Conversation
The module custom options are difficult to track due to legacy names. Make all the name consistent by migrating the old options: - `acf_qtranslate` -> `qtranslate_module_acf` - `qts_options` -> `qtranslate_module_slugs` - `qtranslate_modules` -> `qtranslate_modules_state` Legacy options belonging to external plugins are preserved. New custom options are updated with autoload=false. Migration and cleanup are done on plugin activation. Add temporary migration for master, to be removed before release. Create new constants to better track the names (e.g. ACF forms).
| * | ||
| * @return void | ||
| */ | ||
| function qtranxf_import_legacy_option( $old_name, $new_name, $autoload = null ) { |
There was a problem hiding this comment.
Should this function return update_option result (and true for other cases)?
There was a problem hiding this comment.
I haven't found a case where I'd need it.
| assert( strpos( $new_name, 'qtranslate_' ) === 0 ); | ||
| assert( strpos( $old_name, 'qtranslate_' ) === 0 ); | ||
| qtranxf_import_legacy_option( $old_name, $new_name, $autoload ); | ||
| delete_option( $old_name ); |
There was a problem hiding this comment.
Better test qtranxf_import_legacy_option before doing this.
There was a problem hiding this comment.
With a rename the old option should be deleted in any case or if it it appears again.
If the new option exist, it should not be overwritten. But the old option should removed even if nothing was updated.
| */ | ||
| function qtranxf_import_legacy_option( $old_name, $new_name, $autoload = null ) { | ||
| assert( strpos( $new_name, 'qtranslate_' ) === 0 ); | ||
| if ( ! get_option( $new_name ) ) { |
There was a problem hiding this comment.
Not sure new named option should always have the priority.
Maybe we could test also module activation state (test above AND module is active) though maybe involves a too remote scenario to complicate the function...
There was a problem hiding this comment.
The idea of the import is a one-shot action at activation, the goal is that QT-XT cares of its own options (modules).
The normal scenario is that new options don't exist yet. If we can import a value such as ACF or slugs legacy settings (coming from original plugins) the new option is created from these values to ensure the transition from the old plugins. But from that point we want to remain with the QT-XT setup and ignore the legacy stuff once we have all set in QT-XT.
The most tricky case is when the QT-XT options are reset, we don't really want to import again but in that case we expect the admin to save a new set of options right after the reset. If this is not done, a new import can happen again.
This patch is only valid for the modules as it's a new feature in QT-XT but this will be expanded to all other options later once we cut completely the links with QT-X (not XT).
| const QTX_OPTIONS_MODULES_STATE = 'qtranslate_modules_state'; | ||
| const QTX_OPTIONS_MODULE_ACF = 'qtranslate_module_acf'; | ||
| const QTX_OPTIONS_MODULE_SLUGS = 'qtranslate_module_slugs'; |
There was a problem hiding this comment.
These definitions would look better in modules source files.
It would be better to segregate code which depends on module list from core files.
There was a problem hiding this comment.
The idea is that all options definitions should be kept consistent in a unique place (the options names are used both for front and admin). Currently it's hard to to understand where a general option is set or read in the code because the values are spread everywhere and most of the settings are generated from intermediate strings (with qtranslate_ prefix). The modules should not be fundamentally different regarding the options interface. The other options should also converge to something more comprehensive.
| delete_option( QTX_OPTIONS_MODULES_STATE ); | ||
| delete_option( QTX_OPTIONS_MODULE_ACF ); | ||
| delete_option( QTX_OPTIONS_MODULE_SLUGS ); |
There was a problem hiding this comment.
These would look better somewhere in modules source files to make things cleaner, maybe a new function to be called from here.
It would be better to segregate code which depends on module list from core files.
There was a problem hiding this comment.
I also thought this, but this belongs more to general admin functionalities for the whole QT-XT plugin. For the same reason as for the option names, better keep those with the rest and not split them to avoid the risks of missing updates/deletions.
The module custom options are difficult to track due to legacy names.
Make all the name consistent by renaming the old options:
acf_qtranslate->qtranslate_module_acfqts_options->qtranslate_module_slugsqtranslate_modules->qtranslate_modules_state(already pre-renamed in Refactor module activation handler and options #1150)
Generalize migration and cleanup on plugin activation:
Create new constants to better keep track the names (e.g. ACF forms) and deletion.
New custom options for modules are updated with
autoload=false.Add temporary import for
master, to be removed before release.