add ComparatorInterface to allow custom Comparator implementation #152

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

This pull request passes (merged 2b84652b into cb0cda2).

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -100,7 +100,7 @@ public function compare(Schema $fromSchema, Schema $toSchema)
foreach ($toSchema->getSequences() as $sequence) {
$sequenceName = $sequence->getShortestName($toSchema->getName());
- if ( ! $fromSchema->hasSequence($sequenceName)) {
+ if (!$fromSchema->hasSequence($sequenceName)) {
@stof

stof May 21, 2012

Member

this change is wrong according to the Doctrine coding standards. Please don't change the CS of the existing code

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -261,7 +261,7 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
}
}
- foreach ($renameCandidates as $candidateColumns) {
+ foreach ($renameCandidates as $candidate => $candidateColumns) {
@stof

stof May 21, 2012

Member

why adding a useless variable ?

lib/Doctrine/DBAL/Schema/ComparatorInterface.php
+ * @license http://www.opensource.org/licenses/lgpl-license.php LGPL
+ * @link www.doctrine-project.org
+ * @since 2.0
+ * @version $Revision$
@stof

stof May 21, 2012

Member

this annotation should be removed as it will not be replaced anyway

This pull request passes (merged 6c12c20e into cb0cda2).

lib/Doctrine/DBAL/Schema/ComparatorInterface.php
+/**
+ * Compare to Schemas and return an instance of SchemaDiff
+ *
+ * @copyright Copyright (C) 2005-2009 eZ Systems AS. All rights reserved.
@beberlei

beberlei May 22, 2012

Owner

Please remove the copyright and license tags here. They don't apply to this code.

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -78,7 +78,7 @@ public function compare(Schema $fromSchema, Schema $toSchema)
$tableName = $table->getShortestName($fromSchema->getName());
$table = $fromSchema->getTable($tableName);
- if ( ! $toSchema->hasTable($tableName) ) {
+ if (!$toSchema->hasTable($tableName) ) {
@beberlei

beberlei May 22, 2012

Owner

please revert this to ( ! $toSchema with spaces, also the two occurances below.

This pull request fails (merged 7517418d into cb0cda2).

This pull request fails (merged 3b5da550 into 4030787).

This pull request fails (merged f829dd06 into 4030787).

This pull request fails (merged 8cc00bb into 4030787).

Owner

beberlei commented Jul 9, 2012

Can you explain what you need this for? I think we are missing an opportunity here to fix a bug or a problem that you are having an not telling us about :-)

catacgc commented Jul 9, 2012

One of the use cases is very specific: we don't want schema tool to CRUD relations so we needed this extension point.

The other one is a bug fix (but it's a bit hacky) - schema tool isn't aware of enums not changing so it always runs the alter table for those (not a big deal because mysql doesn't do anything when the alter command doesn't effectively changes the schema, but if you use a lot of custom enum types, the --dump-sql flag produces a lot of unneeded output and it becomes unusable for dev purposes)

Catalin Costache
catacgc@gmail.com

On Jul 9, 2012, at 11:04 AM, Benjamin Eberlei wrote:

Can you explain what you need this for? I think we are missing an opportunity here to fix a bug or a problem that you are having an not telling us about :-)


Reply to this email directly or view it on GitHub:
#152 (comment)

Owner

beberlei commented Jul 9, 2012

what do you mean with relations? Foreign keys? In that case you just need a custom platform which has supportsForeignKeys = false.

catacgc commented Jul 10, 2012

Yes, I mean foreign keys.
I'll look into changing the platform - it seems that I can supply a Driver implementation for DriverManager::getConnection calls

Anyway, this change is not backward incompatible, breaks the hard coupling between SchemaTool and Comparator and provides an extra extension point (or maybe you want to avoid this? )

Catalin Costache
catacgc@gmail.com

On Jul 9, 2012, at 12:15 PM, Benjamin Eberlei wrote:

what do you mean with relations? Foreign keys? In that case you just need a custom platform which has supportsForeignKeys = false.


Reply to this email directly or view it on GitHub:
#152 (comment)

Owner

beberlei commented Jul 10, 2012

Yes i want to avoid extension points if they only lead to people not talking and/or contributing back and fixing their "bugs" on their own.

Owner

beberlei commented Jul 10, 2012

class MyPlatform extends MySQLPlatform
{
    public function supportsForeignKeys() { return false; }
}

Then:

DriverManager::getConnection(array('platform' => new MyPlatform()));

@beberlei beberlei closed this Nov 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment