Broken sfPropelGenerator (BC?) #139

Closed
rozwell opened this Issue May 10, 2012 · 6 comments

Comments

Projects
None yet
3 participants
@rozwell
Contributor

rozwell commented May 10, 2012

There are few changes that breaks sfGuardPlugin's modules like:

  • sf_guard_user Cannot fetch TableMap for undefined table: sf_guard_permission
  • sf_guard_permission Cannot fetch TableMap for undefined table: sf_guard_group
  • sf_guard_group Cannot fetch TableMap for undefined table: sf_guard_permission

And probably more, just checked those 3.

Here is a diff between new-broken (red) code and working old one:

diff --git a/lib/generator/sfPropelGenerator.class.php b/lib/generator/sfPropelGenerator.class.php
index 2c4af1b..4b6e3fe 100644
--- a/lib/generator/sfPropelGenerator.class.php
+++ b/lib/generator/sfPropelGenerator.class.php
@@ -66,88 +66,38 @@ class sfPropelGenerator extends sfModelGenerator
   public function getManyToManyTables()
   {
     $tables = array();
-    $middleTables = array();
-    $foreignTables = array();
-    $relations = array();

-    // go through all relations
-    $table = $this->getTableMap();
-    foreach  ($table->getRelations() as $relation)
+    // go through all tables to find m2m relationships
+    foreach ($this->dbMap->getTables() as $tableName => $table)
     {
-      //we have a many to many Relation
-      if (RelationMap::MANY_TO_MANY === $relation->getType())
-      {
-        $foreignTables[$relation->getLocalTable()->getClassname()] = $relation;
-      }
-      else if (RelationMap::ONE_TO_MANY === $relation->getType())
-      {
-        $relations[$relation->getLocalTable()->getClassname()] = $relation;
-      }
-    }
+      // load this table's relations and related tables
+      $table->getRelations();

-    // find middleTable for Many to Many relation
-    foreach ($foreignTables as $tableName => $relation)
-    {
-      $foreignTable = $relation->getLocalTable();
-      foreach ($foreignTable->getRelations() as $foreignRelation)
+      foreach ($table->getColumns() as $column)
       {
-        $foreignTableClassname = $foreignRelation->getLocalTable()->getClassname();
-
-        // Test if the foreign table has a common relation with our table
-        if (RelationMap::ONE_TO_MANY === $foreignRelation->getType()
-            && array_key_exists($foreignTableClassname, $relations))
+        if ($column->isForeignKey() && $column->isPrimaryKey() && $this->getTableMap()->getClassname() == $this->dbMap->getTable($column->getRelatedTableName())->getClassname())
         {
-          $columns = $relations[$foreignTableClassname]->getLocalColumns();
-          $relatedColumns = $foreignRelation->getLocalColumns();
-
-          $middleTable = $foreignRelation->getLocalTable();
-          if ($middleTable->isCrossRef() && !isset($middleTables[$middleTable->getClassname()]))
+          // we have a m2m relationship
+          // find the other primary key
+          foreach ($table->getColumns() as $relatedColumn)
           {
-            // Add this middleTable to table list to prevent using it twice
-            $middleTables[$middleTable->getClassname()] = $middleTable;
-            $tables[] = array(
-              'middleTable'   => $middleTable,
-              'relatedTable'  => $foreignTable,
-              'column'        => reset($columns),
-              'relatedColumn' => reset($relatedColumns),
-            );
-            continue 2;
+            if ($relatedColumn->isForeignKey() && $relatedColumn->isPrimaryKey() && $this->getTableMap()->getClassname() != $this->dbMap->getTable($relatedColumn->getRelatedTableName())->getClassname())
+            {
+              // we have the related table
+              $tables[] = array(
+                'middleTable'   => $table,
+                'relatedTable'  => $this->dbMap->getTable($relatedColumn->getRelatedTableName()),
+                'column'        => $column,
+                'relatedColumn' => $relatedColumn,
+              );
+
+              break 2;
+            }
           }
         }
       }
     }

-    //Keep BC with M2M without isCrossRef = true
-    foreach ($relations as $relation)
-    {
-      $middleTable = $relation->getLocalTable();
-      //check if $middleTable is a Many 2 Many :
-      // exclude already found middle table
-      // if there it has 2  PKs
-      // if there id only 2 columns in the table
-      // if PKs are also FKs
-      if (!isset($middleTables[$middleTable->getClassname()])
-        && 2 === count($pks = $middleTable->getPrimaryKeyColumns())
-        && 2 === count($middleTable->getColumns())
-        && $pks[0]->isForeignKey()
-        && $pks[1]->isForeignKey())
-      {
-        //We found a Many to Many middle table
-        $foreignTable = $pks[0]->getRelatedTableName() != $table->getName() ? $pks[0]->getRelatedTable() : $pks[1]->getRelatedTable();
-        $relatedColumn = $pks[0]->getRelatedTableName() != $table->getName() ? $pks[0] : $pks[1];
-        $columns = $relation->getLocalColumns();
-        // Add this middleTable to table list to prevent using it twice
-        $middleTables[$middleTable->getClassname()] = $middleTable;
-
-        $tables[] = array(
-          'middleTable'   => $middleTable,
-          'relatedTable'  => $foreignTable,
-          'column'        => reset($columns),
-          'relatedColumn' => $relatedColumn,
-        );
-      }
-    }
-
     return $tables;
   }
@willdurand

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand May 12, 2012

Owner

@rozwell it should be fixed by: #90

Are you up to date?

Owner

willdurand commented May 12, 2012

@rozwell it should be fixed by: #90

Are you up to date?

@rozwell

This comment has been minimized.

Show comment Hide comment
@rozwell

rozwell May 12, 2012

Contributor

I'm quite sure I've used the latest but will check again now.

Contributor

rozwell commented May 12, 2012

I'm quite sure I've used the latest but will check again now.

@rozwell

This comment has been minimized.

Show comment Hide comment
@rozwell

rozwell May 12, 2012

Contributor

Checked and it fails with the latest versions of both, Propel and sfPropelORMPlugin:

sfPropelORMPlugin$ git log -1
commit c5adeadc462a3f0c6b609bdd85f6a2d3916e8a48
Merge: 8d55e00 9c05ec2
Author: William Durand <william.durand1@gmail.com>
Date:   Fri Apr 27 01:26:14 2012 -0700

    Merge pull request #137 from Carpe-Hora/unavailable-explain

    fixes error when explain is not available

sfPropelORMPlugin/lib/vendor/propel$ git log -1
commit 82372dddcece94f06d3f6e0143ba9a6c0529f201
Merge: 28477fa 8b9449a
Author: William Durand <william.durand1@gmail.com>
Date:   Wed May 9 00:41:55 2012 -0700

    Merge pull request #356 from chmeliuk/illegal-offset-type

    [BUG] Illegal offset type in /propel/runtime/lib/collection/PropelObjectCollection.php line 222

Contributor

rozwell commented May 12, 2012

Checked and it fails with the latest versions of both, Propel and sfPropelORMPlugin:

sfPropelORMPlugin$ git log -1
commit c5adeadc462a3f0c6b609bdd85f6a2d3916e8a48
Merge: 8d55e00 9c05ec2
Author: William Durand <william.durand1@gmail.com>
Date:   Fri Apr 27 01:26:14 2012 -0700

    Merge pull request #137 from Carpe-Hora/unavailable-explain

    fixes error when explain is not available

sfPropelORMPlugin/lib/vendor/propel$ git log -1
commit 82372dddcece94f06d3f6e0143ba9a6c0529f201
Merge: 28477fa 8b9449a
Author: William Durand <william.durand1@gmail.com>
Date:   Wed May 9 00:41:55 2012 -0700

    Merge pull request #356 from chmeliuk/illegal-offset-type

    [BUG] Illegal offset type in /propel/runtime/lib/collection/PropelObjectCollection.php line 222

@jaugustin

This comment has been minimized.

Show comment Hide comment
@jaugustin

jaugustin May 13, 2012

Member

could you give us more detail, example that is failing,

I think the table map has nothing to do with the sfPropelGenerator, do you have a stack trace ?

Member

jaugustin commented May 13, 2012

could you give us more detail, example that is failing,

I think the table map has nothing to do with the sfPropelGenerator, do you have a stack trace ?

@jaugustin

This comment has been minimized.

Show comment Hide comment
@jaugustin

jaugustin May 29, 2012

Member

Ok I got the issue and fixed it, I will do a PR and a write a test

Member

jaugustin commented May 29, 2012

Ok I got the issue and fixed it, I will do a PR and a write a test

jaugustin added a commit to jaugustin/sfPropelORMPlugin that referenced this issue May 29, 2012

willdurand added a commit that referenced this issue Jun 11, 2012

Merge pull request #142 from jaugustin/fix-m2m-admin-gen
[wip] fix admin gen m2m issue tableMap not found #139
@willdurand

This comment has been minimized.

Show comment Hide comment
@willdurand

willdurand Jun 11, 2012

Owner

Should be fixed now.

Owner

willdurand commented Jun 11, 2012

Should be fixed now.

@willdurand willdurand closed this Jun 11, 2012

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