Skip to content

Conversation

@sagarnasit
Copy link
Contributor

@sagarnasit sagarnasit commented Sep 7, 2018

Fixes #1203

Depends on:
EasyEngine/auth-command#9

Move migration execution in individual packages. Change migration execution logic for package migratoin as well.

@sagarnasit sagarnasit self-assigned this Sep 7, 2018
@sagarnasit sagarnasit changed the title Fix migration into individual package WIP: Fix migration into individual package Sep 7, 2018
}

private static function get_migrations_to_execute() {
private static function get_migrations_to_execute( $path ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add doc comment in function

);
}

private static function get_migrations_from_db() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add doc comment in function

}

private static function get_migrations_from_fs() {
private static function get_migrations_from_fs( $path ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add doc comment in function

}
}

private static function get_migration_class_name( $migration_name ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add doc comment in function

@kirtangajjar
Copy link
Contributor

inform users that migrations are picked up from filesystem based on package name and inform where code is trying to find migration. https://github.com/EasyEngine/easyengine/pull/1209/files#diff-bfc37b6395768a2f0ee3bc8a300c5fceR78

@sagarnasit sagarnasit changed the title WIP: Fix migration into individual package Fix migration into individual package Sep 10, 2018
@sagarnasit sagarnasit changed the title Fix migration into individual package WIP: Fix migration into individual package Sep 10, 2018
kirtangajjar
kirtangajjar previously approved these changes Sep 14, 2018
@kirtangajjar
Copy link
Contributor

The PR is fine but we need to discuss how do we trigger migrations for the first time. Hence I'm blocking this PR for now.

Copy link
Member

@mrrobot47 mrrobot47 left a comment

Choose a reason for hiding this comment

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

Update PR according to changes in #1224

@sagarnasit sagarnasit changed the title WIP: Fix migration into individual package Fix migration into individual package Sep 21, 2018
@sagarnasit
Copy link
Contributor Author

sagarnasit commented Sep 21, 2018

@kirtangajjar Please review this PR again as this is updated with latest migration changes from PR #1224

use EE\Model\Migration;
use EE\Utils;

class Executor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless string interpolation is required, use single quotes.

This should have been flagged by PHPCS ideally. If it has not on your machine, ping me tommorow, well set it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay. I didn't fix those PHPCS errors intentionally #1124 since this PR of @sagarnasit was already there and I informed him to take care of fixes.

mrrobot47
mrrobot47 previously approved these changes Sep 27, 2018
* @package EE\Migration
*/
class Blank extends \EE\Migration\Base {
class Test extends \EE\Migration\Base {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this file

@rahulsprajapati rahulsprajapati merged commit 5bedd05 into EasyEngine:develop-v4 Oct 3, 2018
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.

5 participants