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

WIP: Migration updates for more wholistic functionality #2065

Merged
merged 14 commits into from
Aug 12, 2019

Conversation

lonnieezell
Copy link
Member

@lonnieezell lonnieezell commented Jun 21, 2019

Many of these changes are to help unify migrations across multiple namespaces for when modular code patterns are used.

  • Creating migrations through the command line should always use UTC dates.
  • Removing current concept. This can be handled better through version control branch management by the developer, and doesn't play well with multiple namespaces.
  • Removing sequential migration files. This only adds complexity and the only problems it seemed to solve (ordering migrations across teams/developers/etc) can be handled with timestamp migrations as long as they are all in the same timezone (which is why its being moved to UTC across the board)
  • Better tracking of migration batches so that we know exactly what was ran across all namespaces and can roll those back together.
  • When running migrations across multiple namespaces, all migrations are combined into a single run and ran in order of timestamps to help projects with modular code/addons to run smoothly and better account for changes.
  • Migrations names should allow either the current timestamp (20180115011221) or a more readable version (2018_01_15_011221)
  • Migration class names are now Pascal case without the word "Migration". Since they are namespaced now there's no need to add "Migration" to avoid clashes. Brings it in line with class names use elsewhere in the framework.

@lonnieezell lonnieezell added this to the 4.0.0-rc.1 milestone Jun 21, 2019
@lonnieezell lonnieezell self-assigned this Jun 21, 2019
@MGatner
Copy link
Member

MGatner commented Jun 21, 2019

Yay! This is looking great. Let me know if I can help with anything.

@@ -323,7 +312,8 @@ public function latestAll(string $group = null): bool
}

// Get all namespaces from the autoloader
$namespaces = Services::autoloader()->getNamespace();
$namespaces = Services::autoloader()
->getNamespace();
Copy link
Contributor

Choose a reason for hiding this comment

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

small typing mistake

@lonnieezell lonnieezell merged commit 290c9f6 into develop Aug 12, 2019
@MGatner
Copy link
Member

MGatner commented Aug 12, 2019

Awesome! Very excited to try this out. I’ll give it a run through this morning.

@MGatner
Copy link
Member

MGatner commented Aug 13, 2019

@lonnieezell I think there might be a problem. I'll go over it some more in case it is something I'm doing, but migrations aren't matching their versions. I think the issue is that array_merge() is renumbering index keys from findMigrations() so it fails to match a key at this line in checkMigrations():

if ((int)$targetVersion !== 0 && ! array_key_exists($targetVersion, $migrations))

@MGatner
Copy link
Member

MGatner commented Aug 13, 2019

Yes the issue is with numeric migration versions - array_merge() is renumbering them. Versions with dashes and underscores are merged as string keys, but anything that is an int (even a string int) gets renumbered.

@MGatner
Copy link
Member

MGatner commented Aug 13, 2019

Okay easy fix is union instead of merge; PR incoming.

@jim-parry jim-parry deleted the migrationsutc branch August 14, 2019 15:24
@muradmustafayev
Copy link

muradmustafayev commented Aug 17, 2019

I downloaded CI4 via composer (devstarter). But there's problem with migrate:create. For some reason in App\Config\Migration.php file there's no timestampFormat variable. This code below is missing

/*
|--------------------------------------------------------------------------
| Timestamp Format
|--------------------------------------------------------------------------
|
| This is the format that will be used when creating new migrations
| using the cli command:
| > php spark migrate:create
|
| Typical formats:
| YmdHis_
| Y-m-d-His_
| Y_m_d_His_
|
*/
public $timestampFormat = 'Y-m-d-His_';

And this is the error of command line

An uncaught Exception was encountered Type: ErrorException Message: Undefined property: Config\Migrations::$timestampFormat Filename: E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\Commands\Database\CreateMigration.php Line Number: 146

    Backtrace:
                                            -146 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\Commands\Database\CreateMigration.php::errorHandler
                                                            -135 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CLI\CommandRunner.php::run
                                                            -108 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CLI\CommandRunner.php::runCommand
                                                            -84 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CLI\CommandRunner.php::index
                                                            -834 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CodeIgniter.php::_remap
                                                            -335 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CodeIgniter.php::runController
                                                            -245 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CodeIgniter.php::handleRequest
                                                            -85 - E:\OSPanel\domains\localhost\magazine\vendor\codeigniter4\codeigniter4\system\CLI\Console.php::run
                                                            -56 - E:\OSPanel\domains\localhost\magazine\spark::run

`

But after adding missing code it works greate.

Thank you for your attention

@MGatner
Copy link
Member

MGatner commented Aug 17, 2019

That’s odd, it’s definitely there:

public $timestampFormat = 'Y-m-d-His_';

After installing did you run composer update? Sounds like possibly app has different publish rules for the devstarter repo.

@muradmustafayev
Copy link

I've tried composer update but no changes. Should insert manually that missing code

@MGatner
Copy link
Member

MGatner commented Aug 17, 2019

That’s because I’m a dummy - that will only update Config in vendor/codeigniter4/codeigniter4/app. Could you check that the correct version is there? And @jim-perry might need to update the deployment process to seed the new version.

@jim-parry
Copy link
Contributor

With the devstarter, composer update only updates the dependencies, i.e. vendor/codeigniter4/codeigniter4. Any changes to app, public, tests & writable will be inside vendor/codeigniter4/... and will need to be copied to the project's app, public, etc.
Such changes should be mentioned in the changelog, if it has been updated. IIRC, it is the beta.4 changelog, and does not address what has been merged since, i.e. app/Config/Migration and app/Controllers/BaseController.
I don't usually update the changelog in the repo until closer to the next release.

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.

5 participants