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

NEW LinkableMigrationTask for migrating sheadawson-lin kable links #261

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Mar 25, 2024

Description

These changes use the already developed logic of GorriecoeMigrationTask class. Since the Model structure of these two modules is almost identical, common GorriecoeMigrationTask logic was separated into a ModuleMigrationTaskTrait and implemented in LinkableMigrationTask for ease of support in the future.
Also, minor changes were made to MigrationTaskTrait and the classIsOldLink class method was removed. Tests have been added and documentation on migration from the Linkable module to LinkField has been updated.

Parent issue

Note: For further implementation, it is required that this PR be merged into the main branch 4 first.

@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch from 0288f89 to 4971a3c Compare March 25, 2024 19:36
@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch 19 times, most recently from 46b933b to 1d20319 Compare April 5, 2024 03:30
@sabina-talipova sabina-talipova marked this pull request as ready for review April 5, 2024 03:31
/**
* Returns true if the class represents an old link to be migrated
*/
abstract private function classIsOldLink(string $class): bool;
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 method classIsOldLink have been removed as redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Has this been tested with the gorriecoe migration task to make sure it doesn't cause problems there?

@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch 3 times, most recently from 8fce8e9 to 4c7fbfc Compare April 7, 2024 20:41
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Works great locally - some changes required.

/**
* Returns true if the class represents an old link to be migrated
*/
abstract private function classIsOldLink(string $class): bool;
Copy link
Member

Choose a reason for hiding this comment

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

Has this been tested with the gorriecoe migration task to make sure it doesn't cause problems there?

tests/php/Tasks/LinkableMigrationTaskTest.php Outdated Show resolved Hide resolved
tests/php/Tasks/LinkableMigrationTaskTest.php Outdated Show resolved Hide resolved
tests/php/Tasks/LinkableMigrationTaskTest.php Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch 2 times, most recently from c0773a7 to 9b3de66 Compare April 9, 2024 20:23
@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch 19 times, most recently from f382b2e to 5beb60d Compare April 10, 2024 20:37
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Just about there.

There were a lot of pushes since the last review - I'm assuming that there weren't any changes unrelated to the changes I requested. If you did make other changes, please let me know so I can review them.

docs/en/09_migrating/01_linkable-migration.md Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
src/Tasks/ModuleMigrationTaskTrait.php Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Apr 11, 2024

There were a lot of pushes since the last review - I'm assuming that there weren't any changes unrelated to the changes I requested. If you did make other changes, please let me know so I can review them.

No, there are not any new changes in the code, except one. I moved performMigration() from ModuleMigrationTaskTrait to the tasks, as you required.
I had to test remotely, since time to time some tests failed with unrelated error in the local environment.

@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch 3 times, most recently from cbe6b9b to 458b3cb Compare April 11, 2024 20:03
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Last few changes

docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Show resolved Hide resolved
docs/en/09_migrating/01_linkable-migration.md Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4/migration-task-linkable-2 branch from 458b3cb to 4d9c84f Compare April 12, 2024 00:39
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit b713978 into silverstripe:4 Apr 12, 2024
13 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4/migration-task-linkable-2 branch April 12, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants