fix issue #129 and #130 with BC break of generated form for M2M without isCrossRef parameters #131

Merged
merged 5 commits into from Apr 10, 2012

Projects

None yet

3 participants

@jaugustin
Member

Hi,

This fixed issue #129 and #130.
It fixed a BC break introduced by my PR #90 witch force the use of isCrossRef to generate Many to Many form list

@xplo , @ComOcean , @bugbyte could you test this PR and say if it fixed your issues ?

Tests passed : Build Status

@bdujon
bdujon commented Apr 5, 2012

Hello,

i tried it but :
i still have table not generated without isCrossRef
i still have multiple widget n-n generated with the same name and different model option ( 1 time is ok the other time is wrong)

While looking the form generation in xdebug (it s very hard to follow) i saw "// if PKs point two different tables" : why can't it be a n-n relation ? i have multiple n-n relation pointing on the same table and it worked perfectly fine before.

Also i don't understand your functionnal test "$b->test()->isa_ok($form->getWidget('extra_seller_list'), 'sfWidgetFormPropelChoice', 'The Seller form should have a sfWidgetFormPropelChoice of extras when there is a isCrossRef on a middle table')" since there extra_seller table has isCrossRef true in the schema and has more than 2 columns

@jaugustin
Member

@xplo
I could remove the limit of N-N to different tables.

My fonctionnal test is there two validate the fact that you could force a table to be a CrossRef even if it doesn't match rules used for BC.

Could you provide schema of the relation that don't generate without isCrossRef
Could you also provide schema of the relation that generate wrong name.

@jaugustin
Member

@xplo you could test last commits I remove the limitation of M2M related two different tables

@bdujon
bdujon commented Apr 5, 2012

you last commit corrected the n-n relation on the same table.

i managed to reproduce the bug on the test database. It s a mix a relation table with crossref and withour crossref. If you add this :
extra_seller_without_crossref:
seller_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
extra_id: { type: integer, primaryKey: true, foreignTable: extra, foreignReference: id }
extra_seller_without_crossref2:
seller_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
extra_id: { type: integer, primaryKey: true, foreignTable: extra, foreignReference: id }
book_seller_without_crossref:
_attributes: { isCrossRef: true }
book_id: { type: integer, primaryKey: true, foreignTable: book, foreignReference: id }
seller_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
seller_seller:
_attributes: { isCrossRef: true }
seller_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
seller_cross_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
seller_seller2:
seller_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }
seller_cross_id: { type: integer, primaryKey: true, foreignTable: seller, foreignReference: id }

The BaseSellerForm will have 2 time extra seller list :
'extra_seller_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Extra')),
'extra_seller_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Extra')),
'book_seller_without_crossref_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Book')),
'extra_seller_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Seller')),
'extra_seller_without_crossref_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Extra')),
'extra_seller_without_crossref2_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Extra')),
'seller_seller_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Seller')),
'seller_seller2_list' => new sfWidgetFormPropelChoice(array('multiple' => true, 'model' => 'Seller')),

@jaugustin
Member

@xplo I fixed case when it duplicate "extra_seller_list"

I also fix issue with Many-to-Many with the same table, but only when isCrossRef is not used.

I still got an issue with Many-to-Many with the same table and isCrossRef=true the issue is that the getter of the middle table has a special name : getMiddleTableRelatedByColumnA or getMiddleTableRelatedByColumnB, I need to find a way to build those getter because with a Many-to-Many relation we don't have the direct relation to the middle table :(

@fzaninotto do you have a solution for the last point ?

@jaugustin jaugustin commented on an outdated diff Apr 5, 2012
...rm/default/template/sfPropelFormGeneratedTemplate.php
@@ -78,7 +78,7 @@ public function updateDefaultsFromObject()
if (isset($this->widgetSchema['<?php echo $this->underscore($tables['middleTable']->getClassname()) ?>_list']))
{
$values = array();
- foreach ($this->object->get<?php echo $tables['middleTable']->getPhpName() ?>s() as $obj)
+ foreach ($this->object-><?php echo $tables['relatedGetter'] ?>() as $obj) //Old: $this->object->get<?php echo $tables['middleTable']->getPhpName() ?>s()
@jaugustin
jaugustin Apr 5, 2012 Member

For Debug only : don't forget (me) to remove that comment before merge ;)

@bdujon
bdujon commented Apr 6, 2012

@jaugustin awesome my project is now generating the form nicely again. Sorry for the mess of a schema you had to add i wasn't sure how to exactly reproduce it :)

There is indeed some case where isCrossRef break the code generation or the runtime with function name conflict. But i think it should be another issue because that s not a BC break and it s probably inside propel code ( not the symfony plugin ), it was like that from the start.

@willdurand
Member

@jaugustin let me know when it's good to merge ;)

@jaugustin jaugustin closed this Apr 10, 2012
@bdujon
bdujon commented Apr 10, 2012

@jaugustin just in case you said something like "For Debug only : don't forget (me) to remove that comment before merge ;)"

@jaugustin jaugustin reopened this Apr 10, 2012
@jaugustin
Member

oups wrong click ;)

Yes I will remove the comment

@jaugustin
Member

This could be merge

@willdurand willdurand merged commit 28dfbde into propelorm:master Apr 10, 2012
@willdurand
Member

Thanks!

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