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

Cycle orm migrations #25

Closed
wants to merge 16 commits into from

Conversation

roxblnfk
Copy link
Member

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? no test
Fixed issues #23

Added new commands:

  migrate/generate  Generates a migration
  migrate/up        Execute all new migrations
  migrate/down      Rollback last migration
  migrate/list      Print list of all migrations

@roxblnfk roxblnfk force-pushed the cycle-orm-migrations branch from 62d2b5e to c25ad33 Compare October 15, 2019 17:22
@roxblnfk roxblnfk force-pushed the cycle-orm-migrations branch from c25ad33 to e0ec67a Compare October 15, 2019 17:25
@samdark samdark added type:feature New feature status:code review The pull request needs review. labels Oct 15, 2019
config/common.php Outdated Show resolved Hide resolved
Added option for singular naming in table names (in migrations and in syncschema)
- EntityFinderHelper renamed to CycleOrmHelper (some code now here);
- ORM Schema now cached;
- UserRepository now extends Cycle\ORM\Select\Repository
Fixed:
- Yiisoft\..\CacheInterface initialization
@roxblnfk
Copy link
Member Author

Now i'm satisfied. Please let's review this

@@ -0,0 +1 @@
!.gitignore
Copy link
Member

Choose a reason for hiding this comment

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

How about moving migrations out of src/Console? These could be treated as non-source i.e. be in the migrations at the top level or at least could be moved out of Console.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe you're right. In addition, I also wanted to make a drop cache when applying / rolling back migrations

Copy link
Member Author

Choose a reason for hiding this comment

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

About the location of the migrations:
Migrations change (add) with Entities. I think they have the right to be in Src along with Entities

Copy link
Member

Choose a reason for hiding this comment

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

OK but, I think, not in Console.

Copy link
Member Author

Choose a reason for hiding this comment

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

src/Migration ?

@samdark samdark removed the status:code review The pull request needs review. label Oct 21, 2019
@samdark
Copy link
Member

samdark commented Oct 21, 2019

@roxblnfk I've pushed some minor adjustments. Overall looks good for me except where migrations are located.


protected function execute(InputInterface $input, OutputInterface $output)
{
$this->cycleHelper->generateMigration($this->migrator, $this->config);
Copy link
Member

Choose a reason for hiding this comment

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

@wolfy-j suggested that such check is needed.

Copy link

Choose a reason for hiding this comment

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

Cycle will generate migrations based on diff with current DB schema if the schema is not consistent you will generate broken migrations. This is a small nasty issue, so such check is recommended.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand and support this recommendation. Also, it would be nice not to generate empty migrations

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... Why would empty migrations be generated?

Copy link

@wolfy-j wolfy-j Oct 21, 2019

Choose a reason for hiding this comment

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

We are checking if migration is required via Generator observer: https://github.com/spiral/framework/blob/master/src/Command/Cycle/MigrateCommand.php#L78

You are free to copy its code, it also gives you a nice description of what have changed.

https://github.com/spiral/framework/blob/master/src/Command/Cycle/Generator/ShowChanges.php

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Fixed in 1.0.5

Copy link
Member

@samdark samdark Oct 21, 2019

Choose a reason for hiding this comment

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

One question. How would it work if you write migration manually? Would that still create dummy migration classes for me?

Copy link

Choose a reason for hiding this comment

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

ORM and manual migrations works using the same engine, you can combine them if you want. Basically ORM simply writes migrations for you, the rest is the same.

Copy link

Choose a reason for hiding this comment

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

@wolfy-j
Copy link

wolfy-j commented Oct 21, 2019

I would recommend a way to add empty migration manually. There are always cases where you can't rely on ORM to solve diff.

@roxblnfk
Copy link
Member Author

I would recommend a way to add empty migration manually. There are always cases where you can't rely on ORM to solve diff.

Then you will have to change the code again, because now using \Cycle\Migrations\GenerateMigrations you can not create empty migrations

@wolfy-j
Copy link

wolfy-j commented Oct 21, 2019

You can not create empty relations for ORM, you still can create migrations directly. Migrator and ORM/Migrations are two different packages.

That's how you can register migration:
https://github.com/cycle/migrations/blob/master/src/GenerateMigrations.php#L87

And that's how you can generate empty one:
https://github.com/cycle/migrations/blob/master/src/GenerateMigrations.php#L117

We have a command which allows us to scaffold migrations with some tables and columns pre-defined (without orm).

@roxblnfk
Copy link
Member Author

@wolfy-j I just want to avoid code duplication

@wolfy-j
Copy link

wolfy-j commented Oct 21, 2019

Well, you can always use a string body like a classic template. I'm just saying that empty/user migration is nothing more than just a class in migrations directory.

@roxblnfk
Copy link
Member Author

I'm just saying that empty/user migration is nothing more than just a class in migrations directory.

Yes, that’s obvious. That is why it is useful to check for unapplied migrations before generating new ones. Because if you do not apply the previously generated migration, the new migration will take the all unapplied changes

@roxblnfk
Copy link
Member Author

All code migrated to yiisoft/yii-cycle#1

@roxblnfk roxblnfk closed this Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants