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

Support for MySQL's MyISAM and other non-InnoDB engines in SchemaTool #865

Closed
wants to merge 1 commit into from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Nov 30, 2013

MySQL allows foreign keys only between tables using the InnoDB engine (MyISAM, memory and other engines don't support foreign keys).
This PR delay foreign keys generation after the hydration of all Table objects. It enables DBAL to check tables engines before adding foreign keys.

This PR is related to doctrine/dbal#437

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2828

We use Jira to track the state of pull requests and the versions they got
included in.

@dunglas
Copy link
Contributor Author

dunglas commented Dec 2, 2013

Existing MySQL tests fixed, new test for this feature added, branch rebased.
(The new test will fail on Travis until doctrine/dbal#437 is merged too).

Still some PostgreSQL tests to fix.

@dunglas
Copy link
Contributor Author

dunglas commented Dec 3, 2013

Everything should be OK. Can anyone review this PR?

@dunglas
Copy link
Contributor Author

dunglas commented Dec 5, 2013

@Ocramius @guilhermeblanco any update?

@beberlei
Copy link
Member

We cant support this imho, the PR is way to complex to support something that is MySQL specific and using a seldomly used feature. You can fix this problem in generation by providing your own Listener to the "postGenerateSchema" event and fix the schema for your cases.

However in general we cannot support all the possiblities and constraints that exist in DDL modelling as this is just way to complex for us to maintain.

@beberlei beberlei closed this Dec 17, 2013
@beberlei
Copy link
Member

Sorry to be this strict on the issue, using a Listener in your project will help you achieve the same though.

@deeky666
Copy link
Member

@beberlei There is a related issue in DBAL which we cannot support either, then.
doctrine/dbal#437

@dunglas
Copy link
Contributor Author

dunglas commented Dec 17, 2013

@beberlei this patch is not so complex and has nothing MySQL specific (the PR in DBAL is MySQL specific). It's mainly a code cleanup. The complex part (line 301 to 338) is already in the code base. It has been introduced by #440 and I've just refactored it a bit. This the same algorithm and mostly the same implementation.

The fact is that my patch is even proper than the current implementation of foreign key addition by the ORM Schema generator. In the current implementation, the foreign table is passed as a string in parameter of Table::addForeignKeyConstraint() and when a string is used instead of an instance of Table, the existence of the foreign table is not checked: https://github.com/doctrine/dbal/blob/fa74679e13b2a107e12946b79c0f38c493e5774f/lib/Doctrine/DBAL/Schema/Table.php#L380
This can lead to unexpected errors. For instance, in functional tests, foreign keys are added to table that does not exist without any warning or error. This PR fix that too.
This PR also remove a call to the deprecated addNamedForeignKey() method.

I fully agree that Doctrine should not support all features of all supported DBMS. But MySQL is probably the more popular DBMS used with Doctrine and the MEMORY engine is widely used for performance. It can even be a lightweight replacement of Memcache stuffs in some cases.
MyISAM is also widely used (especially because it supports full text search which was not the case of InnoDB until recently) and is still officially supported by MySQL.

It's strange to only have partial support of table engines. The engine keyword is supported in mapping information (@Table annotation) but is not taken into account by the generator tool.

Please reconsider this PR: it's not that complex and should be easy to maintain (we talk about 50 lines of production code that is fully tested) and it can be very useful.

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.

4 participants