doctrine / DoctrineFixturesBundle

Symfony integration for the doctrine/data-fixtures library
MIT License
2.45k stars 202 forks source link

Allow option to exclude purged tables/views #330

Closed Zul3s closed 3 years ago

Zul3s commented 4 years ago

Hi, I would like add small argument to manage excluded purge tables/views. I don't know how i can add test simply ?

Can be related with #320 ?

greg0ire commented 4 years ago

This looks like the kind of thing that should be managed with configuration: why would you want to sometimes exclude tables from purge, some other tables not?

Zul3s commented 4 years ago

Maybe i don't know the most simple configuration right, but for me, with command parameter it's easy to use. example : EntityA -> Simple "standard" entity hydrate with fixtures EntityB -> Entity hydrate by Web API (ex. import european cities)

On first usage, we want all fixtures and imports but after when we load only fixtures, we don't want delete cities because it's just "reference" data and make long time to import.

What do you think ?

db306 commented 3 years ago

This looks like the kind of thing that should be managed with configuration: why would you want to sometimes exclude tables from purge, some other tables not?

One example are database views mapped to entities, these tables are composed from other tables and purging fails because it tries to truncate them.

johnkary commented 3 years ago

My group would really like this PR merged! I have tested it locally and it works great.

This looks like the kind of thing that should be managed with configuration: why would you want to sometimes exclude tables from purge, some other tables not?

We scaffold our development environment database with a copy of some production data such as users, departments, projects, and all their relationships. Our enterprise organization is large and re-creating all these relationships in dev fixtures is too tedious. We purge most production data but keep data from a few tables. And a few tables even select batches of records to keep while purging the rest. Of course this is custom logic we don't expect DoctrineFixturesBundle to support.

We have already created our own Symfony Command that decorates logic around doctrine:fixtures:load, as we recognize "composition over inheritance" when integrating third-party code. Our command first does some custom logic, then programmatically calls doctrine:fixtures:load, then does more custom logic after fixture data exists.

Accepting this PR means our custom command can continue programmatically calling doctrine:fixtures:load like this:

// This code is inside our custom Symfony Command's execute() method:
$parameters = [
    "command" => "doctrine:fixtures:load",
    "--append" => $input->getOption('append'),
    "--fixtures" => $ourPathsWithFixtures,
    "--purge-excluded" => [ // NEW LIST GOES HERE
        "table1",
        "table2",
        "table3",
    ],
];
$input = new ArrayInput($parameters);

$command = $this->getApplication()->find("doctrine:fixtures:load");
$command->run($input, $output);

There are no other seams available in doctrine:fixtures:load for us to change how purging is done, unless we fork and re-implement doctrine:fixtures:load.

There is no seams to inject custom behavior for purging using ORMPurger, short of hacking up Doctrine metadata. As of today doctrine:fixtures:load does new ORMPurger($em). External code cannot inject its own behavior for that.

The only alternative I can think of is LoadDataFixturesDoctrineCommand service accepting a __construct() argument to wire in a custom ORMPurger. I'm betting 99.9% of users would never need a custom ORMPurger, else we'd have an open issue or PR adding it.

class LoadDataFixturesDoctrineCommand extends DoctrineCommand
{
    public function __construct(ManagerRegistry $doctrine = null, ORMPurger $purger = null)
    {
        parent::__construct($doctrine);

        $this->purger = $purger;
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        /** @var $doctrine \Doctrine\Common\Persistence\ManagerRegistry */
        $doctrine = $this->getContainer()->get('doctrine');
        $em = $doctrine->getManager($input->getOption('em'));

        // ... snip ...

        // NEW LINE HERE
        $purger = $this->purger ?? new ORMPurger($em);

        $purger->setPurgeMode($input->getOption('purge-with-truncate') ? ORMPurger::PURGE_MODE_TRUNCATE : ORMPurger::PURGE_MODE_DELETE);
        $executor = new ORMExecutor($em, $purger);

        // ... snip ...
    }
}

You don't want to hassle with that. Accepting this PR to support custom table purging is much easier for library maintenance and fits most use cases.

@greg0ire Is there anything else stopping this PR from being merged? Tests? Documentation? Please let me know and I'll gladly PR any fixes to implement them and get this merged in.

johnkary commented 3 years ago

Ahh, I didn't see that master branch supports a custom purger. We could work with that solution once it's tagged in a stable release! Thanks for all your hard work.

greg0ire commented 3 years ago

This PR should target master since it is not a bugfix. I'm going to rename master to 3.4.x at some point, so that it's less confusing for everyone. Other than than, I think the PR looks good, and my question has been answered by several people with different use cases, showing there is indeed a need for this.

greg0ire commented 3 years ago

Apparently there are conflicts, maybe with the custom purger you were talking about. I have some maintenance work to do here, I'll do it and then I will release 3.4.0

johnkary commented 3 years ago

Looks like this PR is no longer needed because LoadDataFixturesDoctrineCommand in master supports option --purge-exclusions which has the same behavior as this PR. See https://github.com/doctrine/DoctrineFixturesBundle/blame/master/Command/LoadDataFixturesDoctrineCommand.php#L65

greg0ire commented 3 years ago

Let's close then. I'm working on fixing the build at #334

greg0ire commented 3 years ago

I released. https://twitter.com/greg0ire/status/1327553483118096386