Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AsyncMigrationBase, update base classes and call async methods #17057

Open
wants to merge 1 commit into
base: v15/dev
Choose a base branch
from

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Similar to #16536, this PR makes it possible to (correctly) call async methods in migrations. I've done the following:

  • Added AsyncMigrationBase with new RunAsync() and MigrateAsync() methods, made this the base class of MigrationBase and moved most methods into this new class;
  • Similarly,. added UnscopedAsyncMigrationBase and made this the base class of UnscopedMigrationBase, added AsyncPackageMigrationBase and made this the base class of PackageMigrationBase;
  • Refactored IMigrationPlanExecutor to only have an ExecutePlanAsync() method;
  • Updated all calls to use the new async methods.

Testing should be done by ensuring a clean install creates and updates the database correctly and checking whether the code and tests are all good 😄

}

return result;
}

[Obsolete]
private void HandlePostMigrations(ExecutedMigrationPlan result)
private async Task HandlePostMigrationsAsync(ExecutedMigrationPlan result)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post migrations have been obsoleted for a while (even stated for removal in v13, which didn't happen), but removing them should be done in a separate PR.

namespace Umbraco.Cms.Infrastructure.Migrations;

public class NoopMigration : MigrationBase
public class NoopMigration : AsyncMigrationBase
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be reverted back to MigrationBase to avoid an extra breaking change (so the base class doesn't change)... We have similar base class changes in UnscopedMigrationBase and PackageMigrationBase, which also don't inherit from MigrationBase anymore, but can't be fixed (because you can't simultaneously inherit from two base classes).

@bergmania
Copy link
Member

To me, I seems like there is a lot of breaking changes, that is not strictly necessary. Couldn't we let all the existing logic work as obsoleted, but introduce async migrations next to the existing. Thereby you would not have to change any base classes, you could just make a new version of the classes, E.g. NoopMigrationAsync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants