Add a migration feature #366

ingeniumed · 2025-02-11T04:12:56Z

Description

This PR is meant to add a migration feature within the plugin. The idea is that, using the init hook the plugin will do the following:

Check the latest version set for migration. If it matches the plugin's version, skip the migration run.
Get all the data source configs
Ensure each one corresponds to a service
Get a special instance of HttpDataSource that has a migration flag set on it.
Perform a migration on each config, which will update the schema version, set active, error and run the migration configured for each service.
Save the config, if it's been changed.

What's left in order to finish this PR

Introducing breaking schema changes has to be done in two updates atm. The first updates is adding the migration code so the schema could be altered for each data source. The second update would be actually updating the schema to use it for each data source. In either update, the breaking change has to be rolled out iteratively. Tbh, one shouldn't introduce a breaking change unless they really have to. It should be rolled out without breaking anything. So I'd like to see how I could improve it.
The migration flag is only used in the migration method. The intention is that the migration version of the interface will only allow migration code to be run, and nothing more. That's why validation and sanitization is skipped. I haven't added the migration restriction anywhere else, just so I could get some feedback on this approach.
I haven't done error testing around migration. That needs to be done to ensure that everything works as expected when things fail.
Code clean up after all of the above concerns are addressed.

What I'd like feedback on

The whole approach really to get feedback on it, and to get suggestions on any changes or alternative paths I could take.

…igration version

chriszarate · 2025-02-12T21:22:49Z

Summary of quick chat with @ingeniumed

Migrations should be implemented for every class that implements ArraySerializableInterface.
Migrations can run in from_array before validation.
The result of migrations don't need to be persisted. They are pure functions, so they can just run every time the config is loaded (until such time as they are updated by the user).
__version should probably move to the top-level of config, and not be restricted to "service config". Unfortunately, this probably means updating the front-end code that manages data sources.

ingeniumed · 2025-02-12T22:30:13Z

Thanks @chriszarate for the pairing session, working on the feedback now. I'll comment back here on it's ready to go as right now it's broken.

ingeniumed · 2025-02-21T02:45:12Z

inc/Config/DataSource/HttpDataSource.php

@@ -44,6 +44,8 @@ final public function get_service_name(): string {
 	 * define their own validation schema.
 	 */
 	public static function preprocess_config( array $config ): array|WP_Error {
+		$config = static::perform_migration( $config );


@chriszarate - the new preprocess_config method seems like the right place to call the migration method from within HttpDataSource looks like. Though if a custom implementor is used then it wouldn't be called via from_array so that is a downside.

What do you think about the way the migration methods are called here?

What I haven't done yet:

Move the version field up one level

Update the server and UI to account for that

Added in a sample migration. The active/version changes would really be examples of those

Modify the schema of a data source, and see if the migration works or not properly.

Though if a custom implementor is used then it wouldn't be called via from_array so that is a downside.

from_array is now marked as final so that shouldn't be an issue

In general, I think we should delegate as much as possible and follow the pattern we've used elsewhere of "return the thing or a WP_Error, which we then elevate to the user to remedy"

chriszarate · 2025-02-24T15:11:14Z

inc/Config/DataSource/HttpDataSource.php

+		if ( is_wp_error( $migrated_config ) ) {
+			$config['active'] = false;
+		} else {
+			$config['service_config']['__version'] = static::SERVICE_SCHEMA_VERSION;


This should probably be delegated to the migrate_config version. Maybe there are scenarios where they don't want to advance to the latest version because there is missing info. I think we want to completely delegate to migrate_config and remove this method entirely.

That's a fair point. Having us control would negate the point of letting data source owners take charge of the migration process.

chriszarate · 2025-02-24T15:11:28Z

inc/Config/DataSource/HttpDataSource.php

+	 * Migrates the config to the current schema version.
+	 * Can be overridden by child classes to perform custom migrations.
+	 */
+	protected static function migrate_config( array $config ): array {


Suggested change

protected static function migrate_config( array $config ): array {

protected static function migrate_config( array $config ): array|WP_Error {

chriszarate · 2025-02-24T15:12:02Z

inc/Config/DataSource/HttpDataSource.php

@@ -44,6 +44,8 @@ final public function get_service_name(): string {
 	 * define their own validation schema.
 	 */
 	public static function preprocess_config( array $config ): array|WP_Error {
+		$config = static::perform_migration( $config );


Suggested change

$config = static::perform_migration( $config );

$config = static::migrate_config( $config );

if ( is_wp_error( $config ) ) {

return $config;

}

My intention was to allow us to be able to migrate the fields that we control within the config as well.

A good example of this would be the first migration:

Adding in active field to indicate if a data source is active or not. This could be useful for cases where a migration was unsuccessful or if a data source has errors and we need it to be fixed. This would help build that UI feature.

Moving the version field to the top of the config rather than within the service config.

There could be more down the line but that's why it's been done that way.

Are you saying to move what I have in perform_migration into migrate_config instead and allow for the data from the overriden methods to be merged with that?

Hope that makes sense, and I correctly articulated what you are suggesting.

Adding in active field to indicate if a data source is active or not.

Can't we just use WP_Error as a signal for that? The migration either works or it doesn't. And if it doesn't we have no way to inspect the problem other than to surface an error. In that case we can also treat it as inactive.

Moving the version field to the top of the config rather than within the service config.

This is a design mistake that I think we should just fix. Doesn't need to be handled by a migration, IMO

Put another way, if the migration fails and you set active = false, how are you going to read and respect that? The config still needs to pass validation (which it likely won't, so it will get elevated to an error anyway).

…iden and values merged all the way through

ingeniumed added 2 commits February 11, 2025 14:57

Add the migration service, and a sample migration for the SFCC

37b2e28

Tweaking the migration code

d83d9b5

ingeniumed self-assigned this Feb 11, 2025

ingeniumed added 3 commits February 12, 2025 15:23

Make use of the migrations variable and fix the return value of the m…

16297bf

…igration version

Fix the phpcs issue

4cb4bbb

Fix the yoda condition problems

21ddd46

Attempting to refactor the migration strategy based on feedback

7292590

ingeniumed added 2 commits February 21, 2025 13:40

Merge latest trunk

47e8c61

Remove old code

d16e8c1

ingeniumed commented Feb 21, 2025

View reviewed changes

chriszarate reviewed Feb 24, 2025

View reviewed changes

ingeniumed added 4 commits February 26, 2025 12:24

Experimenting with making a single migration method that can be overr…

d6e5201

…iden and values merged all the way through

Fix the linting issues

daf40ca

Stop the seg fault

0c52b34

Move the migrate method into from_array and make it public

ddda493

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a migration feature #366

Add a migration feature #366

ingeniumed commented Feb 11, 2025 •

edited

Loading

chriszarate commented Feb 12, 2025

ingeniumed commented Feb 12, 2025 •

edited

Loading

ingeniumed Feb 21, 2025 •

edited

Loading

chriszarate Feb 24, 2025

chriszarate Feb 24, 2025

chriszarate Feb 24, 2025

ingeniumed Feb 25, 2025

chriszarate Feb 24, 2025

chriszarate Feb 24, 2025

ingeniumed Feb 25, 2025

chriszarate Feb 25, 2025

chriszarate Feb 25, 2025

chriszarate Feb 25, 2025

	protected static function migrate_config( array $config ): array {
	protected static function migrate_config( array $config ): array\|WP_Error {

-		$config = static::perform_migration( $config );
+		$config = static::migrate_config( $config );
+		if ( is_wp_error( $config ) ) {
+			return $config;
+		}

Add a migration feature #366

Are you sure you want to change the base?

Add a migration feature #366

Conversation

ingeniumed commented Feb 11, 2025 • edited Loading

Description

What's left in order to finish this PR

What I'd like feedback on

chriszarate commented Feb 12, 2025

ingeniumed commented Feb 12, 2025 • edited Loading

ingeniumed Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingeniumed commented Feb 11, 2025 •

edited

Loading

ingeniumed commented Feb 12, 2025 •

edited

Loading

ingeniumed Feb 21, 2025 •

edited

Loading