New Migration Logic

[MGatner]

UPDATE: this has turned into a pretty comprehensive change to the logic and implementation behind migrations.

Description
With the migration refactor findMigrations() now returns migrations with their version as their index key. latestAll() currently uses array_merge() to create one final array from all namespace migrations, but array_merge() renumbers numeric (i.e. integer, even as a technical string) keys so that the target version fails to locate in checkMigrations().

This change uses the array union operator instead of array_merge() as it does no renumbering.

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

@lonnieezell @jim-parry This might be a good one to expedite before people (like me!) start converting projects to the new migrations.

[lonnieezell]

Good catch. Thanks!

[MGatner]

Holy Redis test failure! I saw a recent Redis merge come through - perhaps related? Looks like migrations tests all passed though.

[MGatner]

One more change to fix #2139 - migrations harvested across all namespaces need to be sorted before calling end() to determine the "latest".

[MGatner]

Once again, all test failures were due to Redis->delete() - applicable tests succeeded.

[MGatner]

Well bollocks - I didn't know how to do an upstream recursive merge on the website so I did it locally but forgot this computer wasn't signed. I'll see if I can fix it, but it truly is me. ^^

[MGatner]

Okay, redone with GPG. @lonnieezell think this one is all set - still need to look at the migration logic but hopefully this takes care of the bugs.

[MGatner]

I have a lot more coming - this has grown significantly, hoping to have the new version posted tonight.

[lonnieezell]

Looking forward to it. Thanks so much for tackling this one!

[MGatner]

@lonnieezell I still have to update the commands, tests, and docs but I changed the Runner so much I wanted to get it up for your review before I build around it. Changes started small and then kept growing and growing... See what you think, I'm open to anything.

[MGatner]

This is real close but I'm hitting a snag. setPath() allows loading migrations from a source that otherwise is not detected by findMigrations(). This is convenient but it means that while regressing through batches the runner is not able to locate these migrations. Possible solutions:

1. Nix setPath(), find some way to namespace the test migrations so they are located
2. Save migration paths in the history, and have regress() check for the actual path instead of looking for it in the return of findMigrations()
3. Have addHistory() record an empty namespace when setPath() is used (currently indicates "App", which is usually wrong) and skip these when regressing batches.

I'm leaning towards 2, but would like some guidance. @lonnieezell ?

[lonnieezell]

I'm fine with option 2.

I guess ideally option 1 would be better, but might be trickier to manage.

[MGatner]

I tried to make option 1 work. Support migrations were moved to their own directory and namespace, "DatabaseTestMigrations/Database/Migrations", but now CIDatabaseTestCase isn't detecting them with findMigrations() so all the live DB tests are failing. I tried adding the namespaces manually to Config/MockServices but no bones.

I need to back-burner this project for a bit but feel free to pick it up @lonnieezell, I've allowed "edits from maintainers".

[lonnieezell]

Looks like I'm not able to push my changes directly back into this repo. Good news is that I did get it figured out how to do it. I'm thinking MockServices isn't used anymore. Ended up using Services::autoloader()->addNamespace() in CIDatabaseTestCase for the database namespace. The other one I only loaded during the migration runner test.

I've got 3 tests that are failing currently, but I'll have to get to those tomorrow. Close!

[MGatner]

That’s great news! I didn’t know that autoloader function existed, very handy.

I haven’t used the “allow edits from maintainers” before, but let me see if I can add you to my fork...

[MGatner]

Okay I added you and @jim-parry to my fork in case you ever need to modify a PR.

[MGatner]

@lonnieezell I should be available to work on this again today or tomorrow. Do you have a solution already? or progress you'd like to share? Feel free to push anything to the fork, or I can take it up from here with your addNamespace() hint.

[lonnieezell]

@MGatner I just pushed all of my changes up in to the migration-union branch. Can you pull that down and give it a spin. I think everything is working now.

[MGatner]

@lonnieezell This appears to be passing all tests and looks good to go - did you know of something else that had to happen? I've been using the new MigrationRunner and commands the last week and they work great so I can vouch for the non-test functionality.

FYI the unsigned commit is your merge of my forked branch, 7fa8f02.

[lonnieezell]

@MGatner Didn't know of anything else that needed done, no. Wasn't sure if you had more you wanted to tackle on this or if you were done so I thought I'd give you a look. Also - just to make sure I didn't miss anything. :)

I'll go ahead and merge then. Thanks!

[lonnieezell]

Then again - looking at the files that changed I still have some work to do. I refactored the progress method to latest last night and looks like my IDE was over-eager on its changes so I have a number of text-only changes that need to be undone. :(

[MGatner]

Hmm oops I didn't notice that. Did you mean that you intentionally changed progress to latest? or your IDE did that? I had renamed it because I found latest to be misleading since we aren't necessarily going in order anymore, except within batches. Partway through I regretted the choices of "progress" and "regress" and considered renaming them similar to the commands, "migrate" and "rollback" (and finding a new name for the current migrate(), like process() or run()).

If you can outline what you see that is messed up I can take a crack at it.

[lonnieezell]

I meant to rename progress() to latest() - but the IDE changed a bunch of random text from progress to latest in docs, etc.

I didn't mind regress too much, but progress didn't feel right to me at all and latest is still kind-of accurate since it grabs all un-migrated migrations so went with that. I almost renamed regress to rollback last night, actually, so wouldn't be upset if that changed :)

[MGatner]

Ohhh okay yeah I saw all those changes, I thought that was from merging codeigniter4/develop and something had been changed elsewhere. I'll see what I can sort out...

[MGatner]

Alright that was the most git-fu I've ever done but I think I got all the good stuff cherry-picked and (assuming this still passes) it should be ready to merge. I didn't touch method names but I'm definitely not opposed to changes (like regress -> rollback).

[lonnieezell]

Ok - I think this works for now. Thanks for cleaning that up!

I wouldn't mind changing the regress() method to rollback() but we can do that in a clean PR so as not to confuse this anymore :)