Allow the exclusion of certain tables when purging

[Jasonoro]

The ORMPurger has a $excluded option that allows you to ignore certain tables when purging. The DoctrineFixturesBundle however didn't implement this feature while it does seem very useful (1).

So I added a exclude-table option that allows users to exclude certain tables from purging. I also added the option to the documentation.

Feel free to give feedback if something seems off!

[SenseException]

Thank you for your contribution and extending the documentation for this new feature idea. There are a few questions that come into my mind about this:

• Your addition lacks how it should behave on an integrity constraint violation, which will result in an error. Currently the executor-logger only supports info-output. Should there be an error output?
• There's also the possibility that a cascade delete will remove data despite ignoring it. How should the command behave in that case?
• After some tables aren't purged because of this new option, what happens when the fixtures of these specific tables are loaded by the ORMExecutor?

Questions like these aren't relevant when every table gets purged. As far as I can see, this feature might introduce more maintenance work. What is the use case where you don't want to purge tables? Your table data would be re-inserted into the tables anyway.

If this PR will be accepted, can you please add new tests that will cover the new feature?

[Jasonoro]

Hi,

Thanks for the response. Let's start with the use case for not purging tables. The use case we currently use it for is when there is some (semi-)static data inserted inside the migrations which, if changed, would require some code changes. Without this feature, the tables with this static data get purged which requires us to write code twice. Once in the migrations for the production environment and once in the fixtures for the dev environments. Mistakes can happen here which cause production and dev to have different behaviour. With this feature we'll simply exclude the table that's filled in the migration and we don't have to write the code twice.

Your addition lacks how it should behave on an integrity constraint violation, which will result in an error. Currently the executor-logger only supports info-output. Should there be an error output?

Getting a integrity constraint violation is already possible if there is a mistake in your relation between entities. The current behaviour is just to log it, and changing that doesn't seem feasible at this moment due to the fact the ORMExecutor doesn't support different levels of log messages. As such I don't think we should change that and just keep the behaviour the same as it has always been.

There's also the possibility that a cascade delete will remove data despite ignoring it. How should the command behave in that case?

This is a tricky one. The current behaviour of the ORMPurger is to CASCADE delete all foreign keys, but this is only implemented on PostgreSQL. All other platforms just TRUNCATE the table without being able to follow the foreign keys. I find this behaviour a bit strange, as it's so platform-dependent. Not cascading the deletes might be the best option, however that would require a change in the doctrine repository and I have my doubts if that solution is preferable. For now I think this should be documented clearly and I would like to hear your/other people's opinion on this.

After some tables aren't purged because of this new option, what happens when the fixtures of these specific tables are loaded by the ORMExecutor?

Nothing, if you decide to exclude a table from purging that excludes the table from purging, not from running the fixture. That is something you can do with the fixture groups. Unique (or other) constraint failures can already happen if you make a mistake in your fixtures and are something the programmer will have to fix.

If this PR will be accepted, can you please add new tests that will cover the new feature?

Of course, once the discussion has run it's course and if it looks like the MR will be accepted I'll gladly take the time to add some tests.

If you have any other points, or don't agree with something I wrote please say so!

[Jasonoro]

@SenseException Did you see my response? I have some feeling you might have missed it. 😄

[SenseException]

I'm sorry for missing your response. As far as I understand you're having migrations, which are adding data rows to a table. A later executed fixture load will purge the previously added data from that table.

Getting a integrity constraint violation is already possible if there is a mistake in your relation between entities.

This is usually because of a mistake in the code. This feature would introduce something similar but during runtime of a correctly configured entity relation.

I would like to hear your/other people's opinion on this.

I'd like to hear others from @doctrine/doctrinecore about this PR too. Documentation shouldn't be a place for something that hasn't been figured out yet. A decision that is made for a feature has to be covered by BC. Maybe there are also other processes involved I can't see and that can cause problems here.

[alcaeus]

I'm not quite sure about adding this. On one hand, it would make sense exposing it in the command since we already have it as an option in the classes.

On the other hand, as evidenced by the discussion above, this can have very strange side-effects. Keep in mind that the idea of the data-fixtures library is to pre-populate the database when setting up a new environment or when running tests. In such cases, you normally don't want to selectively exclude tables from purging. The issues brought up above however aren't issues with the integration in the bundle, but rather with the functionality itself and should probably have been asked when the feature was introduced.

As such, I'd say we expose this option, along with a couple of notes in the documentation about the caveats of using it, and move the discussion to doctrine/data-fixtures. I've created doctrine/data-fixtures#322 to consolidate discussion on this.

[lstrojny]

This can be closed as it was resolved with #307

[malarzm]

Thanks for the heads up @lstrojny!

[Jasonoro]

@lstrojny Amazing, thank you!