-
Notifications
You must be signed in to change notification settings - Fork 432
Fix migrations #1224
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
Fix migrations #1224
Conversation
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
don't migrate after put in /tmp Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
php/EE/Migration/Executor.php
Outdated
|
|
||
| Utils\delem_log( "ee migration start" ); | ||
| EE::log( "Migrating EasyEngine data to new version" ); | ||
| EE::log( "Executing migrations" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be printed in the first execution.
|
|
||
| if( empty( $migrations ) ) { | ||
| EE::success( "Noting to migrate" ); | ||
| exit( 0 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add return statement here. No need to take execution further if $migrations are empty.
php/EE/Migration/Executor.php
Outdated
| $migrations = self::get_migrations_to_execute(); | ||
|
|
||
| if( empty( $migrations ) ) { | ||
| EE::success( "Noting to migrate" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be printed in the first execution.
Also, take care of the same in this success message:
easyengine/php/EE/Migration/Executor.php
Line 36 in 2ce1a03
| EE::success( "Successfully migrated EasyEngine" ); |
| // array_slice is used to remove . and .. returned by scandir() | ||
| $migrations = array_slice( scandir( self::MIGRATION_PATH ), 2 ); | ||
| array_walk( $migrations, function( &$migration, $index ) { | ||
| $migrations = scandir( self::MIGRATION_PATH ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove first two elements using array_slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is only needed if migrations are executed from outside phar. I've added the condition for that below.
php/EE/Migration/Executor.php
Outdated
| $migrations = scandir( self::MIGRATION_PATH ); | ||
|
|
||
| if( ! Utils\inside_phar() ) { | ||
| $migrations = array_slice( scandir( self::MIGRATION_PATH ), 2 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason of having the same scandir call when it is already there above:
easyengine/php/EE/Migration/Executor.php
Line 109 in 2ce1a03
| $migrations = scandir( self::MIGRATION_PATH ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Fixed.
php/EE/Runner.php
Outdated
| * Triggers migration if current phar version > version in ee_option table | ||
| */ | ||
| private function maybe_trigger_migration() { | ||
| $db_version = Option::get( 'version' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run phpcbf. Equal to sign should match the lower one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't catch this was because we don't have a rule in PHPCS that checks for alignment of = sign (and marks it as error). The tool that we use to fix phpcs errors is a phpstrom code formatter that also does additional code cleanings as aligning = signs.
If my editor would've flagged this warning, It would've been fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the editor configurations accordingly.
php/commands/src/CLI_Command.php
Outdated
| } | ||
|
|
||
| $cache = EE::get_cache(); | ||
| $cache->write( 'migrated', '0' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed now that we are handling it in the options table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Removed.
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
Signed-off-by: Kirtan Gajjar <kirtangajjar95@gmail.com>
No description provided.