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

Added migrations #779

Closed
wants to merge 3 commits into from
Closed

Added migrations #779

wants to merge 3 commits into from

Conversation

lsv
Copy link

@lsv lsv commented Apr 12, 2018

  • Added "real" migrations
  • Tests works with both sqlite and mysql (and yes the blog_test.sqlite file are deleted), I can not test with postgres at the moment

What this "gives" to the demo, which I think would be lovely to see.

  • It gives real migration, using doctrine migrations, (and populating the database with data on the fly) meaning that doctrine/fixtures actually is not needed anymore (only for tests)
    • Due to its not possible to inject services into migrations, it also showcases how to make a public service
  • It also showcases how to test database in functional tests (creating database, deleting database, insert fixture data)

As discussed in #778

@lsv lsv changed the title [WIP] Added migrations Added migrations Apr 12, 2018
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is worth it, because running this initial migration should be equal to run:

$ php bin/console doctrine:schema:create && doctrine:fixtures:load


public function up(Schema $schema): void
{
$userTable = $schema->createTable('symfony_demo_user');
Copy link
Member

Choose a reason for hiding this comment

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

It will be hard to keep up to date this migration file every time you make some changes in the schema.

Copy link
Author

Choose a reason for hiding this comment

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

You should not change this migration file any time. You should make a new.
By using doctrine:migrations:diff its possible to create the difference from the migrations and the current database.

Though doctrine:migrations:diff will only create a migration for the current DBMS so some handwork are required to make it useable on different DBMS.

Copy link
Member

Choose a reason for hiding this comment

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

You should not change this migration file any time. You should make a new.

I thought the idea was just one "initial" migration as this demo isn't a real app.

Copy link
Author

Choose a reason for hiding this comment

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

Well - it could be forked by someone and actually being developed as a real app

Copy link
Member

Choose a reason for hiding this comment

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

Sure, In that case the real app will be the forked, right? how can we maintain compatibility after doctrine:migrations:diff for all DBMS?

Copy link
Author

Choose a reason for hiding this comment

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

Handcrafting is needed to create migrations which will work with other DBMS, and not just the installed platform which created the diff.

But it would be awesome if diff actually created "real" migrations, but this is something that doesnt fit into this discussion :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, that's why I'm -1 on this :)

use Doctrine\Common\Persistence\ManagerRegistry;
use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;

class MigrationInitialImport
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated of AppFixtures.

Copy link
Author

Choose a reason for hiding this comment

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

True (well not 100% duplicate - self::$references and how to get the entitymanager is different)

I wasnt sure if I should make the AppFixtures data provider methods public or static.
(not possible to inject AppFixtures into MigrationInitialImport)

So I decided to keep them both.

@lsv
Copy link
Author

lsv commented Apr 13, 2018

@yceruto as in #778 doctrine:fixtures:load should not be available in production mode, due to best practices.


public function postUp(Schema $schema): void
{
$this->container->get(MigrationInitialImport::class)->importInitialData();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bad practice to use entities in the migrations (entity can be deleted in the future). Here you use entities implicitly. You should load fixtures by raw SQL.

@nicolas-grekas nicolas-grekas changed the base branch from master to main November 19, 2020 12:37
@javiereguiluz
Copy link
Member

Let's close this old PR as "won't fix".

The problem is that this app ships with the database already created. We do this to ease the installation of this app. We can't change this because e.g. it is what makes it possible for this app to be used by PHP source code to benchmark changes, etc.

So, let's not use migrations in this project. It shouldn't be a big problem because migrations are explained in the docs and are usually easy to implement in your own project.

Thanks!

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.

None yet

4 participants