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

Squash core migrations #2842

Merged
merged 4 commits into from
May 10, 2021
Merged

Squash core migrations #2842

merged 4 commits into from
May 10, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

Fixes #2258

Changes proposed in this pull request:

  • Squashes core's migrations into a schema dump. On my local machine, this shaves 7-8 seconds off of install time (likely to be more on slower machines/dbs).
  • Remove some now unnecessary parts of the migrator and migration repository public API

Unfortunately we can't do extension migrations here, as we have no way of filtering out data migrations other than hardcoding in a list. Their impact is relatively small though, so it shouldn't be that bad.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Data migrations (seed default groups, seed default permissions) are deliberately excluded.
This also allows us to remove a lot of now unnecessary public API from the migrator and migration repository.
migrations/install.dump Show resolved Hide resolved
@@ -46,6 +46,8 @@ public function register()
ResetCommand::class,
ScheduleListCommand::class,
ScheduleRunCommand::class
// Used internally to create DB dumps before major releases.
// \Flarum\Database\Console\GenerateDumpCommand::class
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to put this command inside of the source code?

We likely won't need it before a while, and it might end up broken without us even realizing it in the meantime.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Which is why I commented it out. I wanted to keep it so that we could debug how the dump was created if we run into issues with it.

Copy link
Member

Choose a reason for hiding this comment

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

I just don't really like the idea of having that code lying around unused for 1,2,5 years? in the repo. I feel like it would be best inside of an issue, or a dedicated branch so we can find it again later but don't ship it / maintain it as part of the whole 1.x release cycle.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I wanted to put it in the Flarum CLI, but didn't have time to rewrite it so that it worked for this.

src/Database/Console/GenerateDumpCommand.php Show resolved Hide resolved
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

That's probably good like is is now. If there's any issue with the approach QA should hopefully find it.

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

I looked at this previously and it works well locally.

@askvortsov1 askvortsov1 merged commit 6ecca95 into master May 10, 2021
@askvortsov1 askvortsov1 deleted the as/db-schema branch May 10, 2021 22:05
luceos added a commit that referenced this pull request May 29, 2021
luceos added a commit that referenced this pull request May 31, 2021
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.

Web timeouts when installing through web installer
3 participants