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

Consider removing ability to exclude tables from purging #322

Open
alcaeus opened this issue Oct 8, 2019 · 5 comments
Open

Consider removing ability to exclude tables from purging #322

alcaeus opened this issue Oct 8, 2019 · 5 comments

Comments

@alcaeus
Copy link
Member

alcaeus commented Oct 8, 2019

When adding the excluded option to the Symfony command in doctrine/DoctrineFixturesBundle#289, a couple of questions came up with respect to how this is supposed to handle duplicate key constraints in fixtures (in case fixtures are loaded that write to tables that weren't purged), and how to handle cascading foreign key constraints that would remove data from tables that aren't supposed to be purged.

Since those are problems with the concept of excluding tables from purging (and not with the integration of this library into Symfony), I suggest discussing the issues around this functionality here.

Given the potentially severe issues, I would suggest deprecating the functionality in the next minor release and removing it in the next major version. Since the library is designed to pre-populate a database with initial data, I'm not sure if this is a use-case we really want to cover.

@Jasonoro
Copy link

Let's start by saying I understand if you want to deprecate and remove this feature, however I would like to say that the option to exclude tables is extremely useful: I've been using it for a while in a couple of projects and it's also the reason why I created doctrine/DoctrineFixturesBundle#289.

In my opinion misuse of this option is a problem for the programmer: If you incorrectly configure what tables should be excluded and it errors that is something you have to solve. It seems completely logical to me that this would start throwing foreign key errors, and I hope everyone would realize that this is not a fault in the library but with their own code.

The only this I find strange is that the cascading of foreign keys is platform specific. However this could be solved easily. Simply changing the cascade of line 154 from true to false will make the behaviour consistent across platforms and prevent cascading foreign key constraints.

@SenseException
Copy link
Member

In my opinion misuse of this option is a problem for the programmer

I don't fully agree with this sentence. It is often sold as "flexible" if a developer is allowed to do a lot of different things with a library in many different ways, but this form of "flexibility" opens exotic kind of BC breaks that are hard to cover with tests. I prefer to prevent creating new possibilities of misuse. It isn't always easy to prevent that because all the creative ways a library can be used can't be foreseen, but if we can provide less angles of how something can be (mis)used, then it can also reduce the amount of new opened issues. A documentation is rarely read from the beginning to the end (in case the information of misuse can be found in there).

@darthbanana13
Copy link

We are using the symfony data fixtures bundle for test right now and this seems like a huge oversight for us. We have inserts in migrations for common data like countries, timezone, currencies and so on and we found that we have to reimplement the same insertions for the fixtures. It's really not desirable because the inserts in the migration represents data that the application can not function without and the data in the fixtures represents development or test data that should also contain critical application data.

@no-ivan
Copy link

no-ivan commented Mar 20, 2020

Another case. I've created materialized view in postgres db and received this message on purging step:
ERROR: cannot change materialized view "transactions_view
For me obvious that fixture loading must skip materialized view or this should be possibility to exclude this table.

@skylord123
Copy link

I actually wish the exclude option was configurable in Symfony so I could just specify tables that should never be purged. I'm basically under the same use case as @DarthRevan13 in that I have several tables that are seeded with data from migrations but when developers run fixtures these tables get cleared.

My solution was to simply create a custom purger factory that will always pass a list of predefined tables to the ORMPurger for exclusion (I made a blog post about this here: https://skylar.tech/symfony-doctrine-fixture-always-exclude-table/ )

I considered making my fixtures just generate the same data but then I have two places I need to maintain this (migrations and fixtures) and that is just awful for development. I also hated the idea that every time I run fixtures I need to pass the exclude argument. I really like keeping my frequently used dev commands short and easy to use (remembering what tables need to be excluded isn't great).

Anyways, I think this option should never be removed as it is too useful and necessary in some situations. If you are excluding tables from fixtures and getting errors it should be obvious why. The only tables I am excluding are tables that don't have any relations to prevent issues.

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

No branches or pull requests

6 participants