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

Allow option to exclude purged tables/views #330

Closed
wants to merge 8 commits into from

Conversation

Zul3s
Copy link

@Zul3s Zul3s commented Sep 1, 2020

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 ?

phpunit.xml.dist.bak Outdated Show resolved Hide resolved
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Please remove all .idea files from your PR.

@greg0ire
Copy link
Member

greg0ire commented Sep 2, 2020

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
Copy link
Author

Zul3s commented Sep 2, 2020

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
Copy link

db306 commented Oct 7, 2020

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
Copy link

johnkary commented Nov 10, 2020

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
Copy link

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
Copy link
Member

greg0ire commented Nov 11, 2020

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 greg0ire changed the base branch from 3.3.x to master November 11, 2020 09:46
@greg0ire
Copy link
Member

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
Copy link

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
Copy link
Member

greg0ire commented Nov 13, 2020

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

@greg0ire greg0ire closed this Nov 13, 2020
@greg0ire
Copy link
Member

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

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

6 participants