Fix ModelChoiceList problem with string key #6150

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
3 participants
@woodspire
Contributor

woodspire commented Nov 29, 2012

The ModelChoiceList code was based on EntityChoiceList for doctrine, but a critical misinterpretation happen.

I have a list where one key is : "1SER+"

This crash symfony in ChoiceList addChoice code when the index (1SER+) is not a valid form name.
This happen because the ModelChoiceList thinks that because there is only 1 column in the primary key, it will be an integer.

I modified the code to reflect the way EntityChoiceList does it.

I will also send a pull request in the propel1 project to add the isInteger() function in ColumnMap.

isNumeric() is present, but it cannot be used, because 2.34 contains a dot, and it cannot be used has a form name.

Felix Labrecque
Fix problem when 1 column identifier in propel is a string.
The ModelChoiceList thinks it can be used as the index of the ChoiceList, when it can only be used if it is an integer.

@woodspire woodspire referenced this pull request in propelorm/Propel Nov 29, 2012

Closed

add isInteger to help symfony form choicelist index #520

@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Nov 29, 2012

Contributor

The pull request for the isInteger function

propelorm/Propel#520

Contributor

woodspire commented Nov 29, 2012

The pull request for the isInteger function

propelorm/Propel#520

@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Nov 29, 2012

Contributor

It's my first pull request, so I was monitoring to see if everything was fine.

I saw that the (automated ?) build failed, and I checked why.

The query class used in the test, /Bridge/Propel1/Tests/Fixtures/ItemQuery.php is not correct.

the function getPrimaryKeys() returns an invalid array.

it should not return a string but a ColumnMap class (check in TableMap class and you will see that the primaryKeys private field never contains string, but keyed ColumnMap classes.

I am not currently fluent in how to generate test code, so I cannot help here.

Contributor

woodspire commented Nov 29, 2012

It's my first pull request, so I was monitoring to see if everything was fine.

I saw that the (automated ?) build failed, and I checked why.

The query class used in the test, /Bridge/Propel1/Tests/Fixtures/ItemQuery.php is not correct.

the function getPrimaryKeys() returns an invalid array.

it should not return a string but a ColumnMap class (check in TableMap class and you will see that the primaryKeys private field never contains string, but keyed ColumnMap classes.

I am not currently fluent in how to generate test code, so I cannot help here.

@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Nov 29, 2012

Contributor

we could add this verification but it should not be necessary:

&& current($this->identifier) instanceof ColumnMap

Contributor

woodspire commented Nov 29, 2012

we could add this verification but it should not be necessary:

&& current($this->identifier) instanceof ColumnMap

@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Dec 4, 2012

Contributor

Nice. I finally found the problems with the test suite and fix them. It also helped me find a problem with my pull request, so I also fixed that at the same time. Now everything pass the automatic build.

Contributor

woodspire commented Dec 4, 2012

Nice. I finally found the problems with the test suite and fix them. It also helped me find a problem with my pull request, so I also fixed that at the same time. Now everything pass the automatic build.

@woodspire woodspire referenced this pull request in propelorm/Propel Dec 4, 2012

Merged

Add IsInteger function to ColumnMap #526

@@ -0,0 +1,61 @@
+<?php

This comment has been minimized.

@willdurand

willdurand Dec 4, 2012

Contributor

Why don't you use the Propel class?

@willdurand

willdurand Dec 4, 2012

Contributor

Why don't you use the Propel class?

This comment has been minimized.

@woodspire

woodspire Dec 4, 2012

Contributor

Well the ColumMap class from Propel except a TableMap object, and looking at the code in Bridge/Propel1/Tests/Fixtures/ItemQuery.php, I saw that this object was implementing some function from TableMap (getPrimaryKeys) instead of using Problem TableMap class. I taught I would need to create the ColumnMap instead of using the Propel one. Trying to use the Propel class return an error saying that a TableMap was expected, but ItemQuery was given.

I simply saw that it seem that no Propel classes were used in these files, so there must have been a good reason. I continued this trend.

@woodspire

woodspire Dec 4, 2012

Contributor

Well the ColumMap class from Propel except a TableMap object, and looking at the code in Bridge/Propel1/Tests/Fixtures/ItemQuery.php, I saw that this object was implementing some function from TableMap (getPrimaryKeys) instead of using Problem TableMap class. I taught I would need to create the ColumnMap instead of using the Propel one. Trying to use the Propel class return an error saying that a TableMap was expected, but ItemQuery was given.

I simply saw that it seem that no Propel classes were used in these files, so there must have been a good reason. I continued this trend.

This comment has been minimized.

@willdurand

willdurand Dec 5, 2012

Contributor

Could you use a ColumnMap or not?

@willdurand

willdurand Dec 5, 2012

Contributor

Could you use a ColumnMap or not?

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

No we cannot. ColumnMap expect to be pass a TableMap, an we do not have a table class in the test procedure.

@woodspire

woodspire Dec 5, 2012

Contributor

No we cannot. ColumnMap expect to be pass a TableMap, an we do not have a table class in the test procedure.

This comment has been minimized.

@willdurand

willdurand Dec 5, 2012

Contributor

What if you mock a TableMap?

William Durand | http://www.williamdurand.fr

On Wed, Dec 5, 2012 at 9:20 PM, woodspire notifications@github.com wrote:

In src/Symfony/Bridge/Propel1/Tests/Fixtures/ColumnMap.php:

@@ -0,0 +1,61 @@
+<?php

No we cannot. ColumnMap expect to be pass a TableMap, an we do not have a
table class in the test procedure.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6150/files#r2326252.

@willdurand

willdurand Dec 5, 2012

Contributor

What if you mock a TableMap?

William Durand | http://www.williamdurand.fr

On Wed, Dec 5, 2012 at 9:20 PM, woodspire notifications@github.com wrote:

In src/Symfony/Bridge/Propel1/Tests/Fixtures/ColumnMap.php:

@@ -0,0 +1,61 @@
+<?php

No we cannot. ColumnMap expect to be pass a TableMap, an we do not have a
table class in the test procedure.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6150/files#r2326252.

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

Sorry but I am not here to fix all the test suites because it's not currently properly formatted.

Someone, in the past, decided not to mock a TableMap and use ItemQuery and ReadOnlyItemQuery instead.

I am not here to discuss if their choice was right or not, I was ok creating test cases for my PR, but fixing|rewriting other test cases because they do not match the required formatting expected right now, I do not have the man power to do that and I am not that experience in the Propel code to do so.

I found a problem with my test data, understood why it was happening and found a fix that will not break other people code. Trying to understand other test cases to be able to modify their test so that the test suite looks good, I cannot do.

An I am pretty sure it might not end there, trying to mock TableMap might ask for another class to be mocked, etc ...

@woodspire

woodspire Dec 5, 2012

Contributor

Sorry but I am not here to fix all the test suites because it's not currently properly formatted.

Someone, in the past, decided not to mock a TableMap and use ItemQuery and ReadOnlyItemQuery instead.

I am not here to discuss if their choice was right or not, I was ok creating test cases for my PR, but fixing|rewriting other test cases because they do not match the required formatting expected right now, I do not have the man power to do that and I am not that experience in the Propel code to do so.

I found a problem with my test data, understood why it was happening and found a fix that will not break other people code. Trying to understand other test cases to be able to modify their test so that the test suite looks good, I cannot do.

An I am pretty sure it might not end there, trying to mock TableMap might ask for another class to be mocked, etc ...

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

@woodspire If what your class is accessing is only the TableMap, you won't need to mock other stuff. The goal of using mocks is to avoid having to build the full dependency graph. You only mock the direct deps.

@stof

stof Dec 5, 2012

Member

@woodspire If what your class is accessing is only the TableMap, you won't need to mock other stuff. The goal of using mocks is to avoid having to build the full dependency graph. You only mock the direct deps.

This comment has been minimized.

@willdurand

willdurand Dec 5, 2012

Contributor

I'll do that.

William Durand | http://www.williamdurand.fr

On Wed, Dec 5, 2012 at 9:41 PM, Christophe Coevoet <notifications@github.com

wrote:

In src/Symfony/Bridge/Propel1/Tests/Fixtures/ColumnMap.php:

@@ -0,0 +1,61 @@
+<?php

@woodspire https://github.com/woodspire If what your class is accessing
is only the TableMap, you won't need to mock other stuff. The goal of using
mocks is to avoid having to build the full dependency graph. You only mock
the direct deps.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6150/files#r2326592.

@willdurand

willdurand Dec 5, 2012

Contributor

I'll do that.

William Durand | http://www.williamdurand.fr

On Wed, Dec 5, 2012 at 9:41 PM, Christophe Coevoet <notifications@github.com

wrote:

In src/Symfony/Bridge/Propel1/Tests/Fixtures/ColumnMap.php:

@@ -0,0 +1,61 @@
+<?php

@woodspire https://github.com/woodspire If what your class is accessing
is only the TableMap, you won't need to mock other stuff. The goal of using
mocks is to avoid having to build the full dependency graph. You only mock
the direct deps.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6150/files#r2326592.

@willdurand

View changes

src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
@@ -69,6 +76,13 @@ public function __construct($class, $labelPath = null, $choices = null, $queryOb
$choices = array();
}
+ if (1 === count($this->identifier)) {
+ // TODO this should be current($this->identifier)->isInteger() when propel ColumnMap contains the isInteger function

This comment has been minimized.

@willdurand

willdurand Dec 4, 2012

Contributor

I think it's better to keep this behavior, because even if Propel gets this feature, it will be only available for Propel 1.6.8 and upper.

@willdurand

willdurand Dec 4, 2012

Contributor

I think it's better to keep this behavior, because even if Propel gets this feature, it will be only available for Propel 1.6.8 and upper.

@stof

View changes

src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
+ // TODO this should be current($this->identifier)->isInteger() when propel ColumnMap contains the isInteger function
+ if ($this->isInteger(current($this->identifier))) {
+ $this->identifierAsIndex = true;
+ }

This comment has been minimized.

@stof

stof Dec 4, 2012

Member

Please use spaces, not tabs

@stof

stof Dec 4, 2012

Member

Please use spaces, not tabs

@stof

View changes

src/Symfony/Bridge/Propel1/Tests/Fixtures/PropelColumnTypes.php
+ * @version $Revision$
+ * @package propel.runtime.util
+ */
+class PropelColumnTypes

This comment has been minimized.

@stof

stof Dec 4, 2012

Member

why copying the file from Propel ?

@stof

stof Dec 4, 2012

Member

why copying the file from Propel ?

woodspire added some commits Dec 4, 2012

Removed the PropelColumnTypes.php copy
Using the Propel class instead, like in Column class
@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Dec 4, 2012

Contributor

I removed the PropelColumnTypes.php copied from Propel and changed ColumnMap to use the Propel file (same way as in the Column.php file)

Contributor

woodspire commented Dec 4, 2012

I removed the PropelColumnTypes.php copied from Propel and changed ColumnMap to use the Propel file (same way as in the Column.php file)

@stof

View changes

src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
+ if (1 === count($this->identifier)) {
+ if ($this->isInteger(current($this->identifier))) {
+ $this->identifierAsIndex = true;
+ }

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

Please use 4 spaces per level for the indentation, not tabs

@stof

stof Dec 5, 2012

Member

Please use 4 spaces per level for the indentation, not tabs

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

You are right. I fixed my commit.

@woodspire

woodspire Dec 5, 2012

Contributor

You are right. I fixed my commit.

@woodspire

This comment has been minimized.

Show comment
Hide comment
@woodspire

woodspire Dec 5, 2012

Contributor

Do I need to do anything else ?

Contributor

woodspire commented Dec 5, 2012

Do I need to do anything else ?

@stof

View changes

src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
@@ -69,6 +76,12 @@ public function __construct($class, $labelPath = null, $choices = null, $queryOb
$choices = array();
}
+ if (1 === count($this->identifier)) {
+ if ($this->isInteger(current($this->identifier))) {

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

you should use && instead of nesting 2 conditions here

@stof

stof Dec 5, 2012

Member

you should use && instead of nesting 2 conditions here

+ *
+ * @return boolean
+ */
+ private function isInteger($col)

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

you should typehint the argument here

@stof

stof Dec 5, 2012

Member

you should typehint the argument here

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

I cannot typehint here, because when running the code in prod, the $col will be an instance of Propel\Runtime\Map\ColumnMap

but in test, the $col will be an instance of Symfony\Bridge\Propel1\Tests\Fixtures\ColumnMap

@woodspire

woodspire Dec 5, 2012

Contributor

I cannot typehint here, because when running the code in prod, the $col will be an instance of Propel\Runtime\Map\ColumnMap

but in test, the $col will be an instance of Symfony\Bridge\Propel1\Tests\Fixtures\ColumnMap

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

Then your test is wrong. You should not pass a totally unrelated object. Use mocks or stubs.

@stof

stof Dec 5, 2012

Member

Then your test is wrong. You should not pass a totally unrelated object. Use mocks or stubs.

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

But look at the code in Symfony\Bridge\Propel1\TestsFixtures ItemQuery and ReadOnlyItemQuery.

Both these class are unrelated object trying to be a TableMap (check the getTableMap method) !

I am not saying your request cannot be done, I am saying that typehinting ColumnMap will break other test code that are build correct, and theses tests would also be needed to be changed, but I do not understand them, so I cannot change them !

@woodspire

woodspire Dec 5, 2012

Contributor

But look at the code in Symfony\Bridge\Propel1\TestsFixtures ItemQuery and ReadOnlyItemQuery.

Both these class are unrelated object trying to be a TableMap (check the getTableMap method) !

I am not saying your request cannot be done, I am saying that typehinting ColumnMap will break other test code that are build correct, and theses tests would also be needed to be changed, but I do not understand them, so I cannot change them !

@stof

View changes

src/Symfony/Bridge/Propel1/Tests/Fixtures/ReadOnlyItemQuery.php
- return array('id');
+ $cm = new ColumnMap('id', $this);
+ $cm->setType('INTEGER');
+ return array('id' => $cm);

This comment has been minimized.

@stof

stof Dec 5, 2012

Member

missing empty line before the return statement

@stof

stof Dec 5, 2012

Member

missing empty line before the return statement

This comment has been minimized.

@woodspire

woodspire Dec 5, 2012

Contributor

I did not knew you require a blank line before a return.

strange because 10 lines about, in the getTableMap method, there is no space between the return statement and the comment line.

@woodspire

woodspire Dec 5, 2012

Contributor

I did not knew you require a blank line before a return.

strange because 10 lines about, in the getTableMap method, there is no space between the return statement and the comment line.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Dec 5, 2012

Contributor

#6199 now replaces this PR. Please close that one.

Contributor

willdurand commented Dec 5, 2012

#6199 now replaces this PR. Please close that one.

@woodspire woodspire closed this Dec 5, 2012

fabpot added a commit that referenced this pull request Dec 6, 2012

merged branch willdurand/woodspire-master (PR #6199)
This PR was merged into the master branch.

Commits
-------

a3a832c Fix CS in the whole Propel1 bridge
86ab4b3 Add typehint to isInteger(), fix tests
a26a690 remove useless ColumnMap
ffd8759 fix some formatting issue
6fb9536 fix indentation problem
e5e3341 oups. It seems that here, we need \PDO
36d6c40 fix indentation problem
6f8cd9d Removed the PropelColumnTypes.php copy
8125163 removed the TODO mention. Will keep the Propel code here so it can work with older version of Propel
0e4419b I found the error in my latest commit. This pass the test suite.
cf8a6c0 Fix my code and also fix the test suite.
737b596 Merge remote-tracking branch 'upstream/master'
972e503 Fix problem when 1 column identifier in propel is a string.

Discussion
----------

Fix ModelChoiceList problem with string key

Replaces #6150

---------------------------------------------------------------------------

by willdurand at 2012-12-05T20:51:44Z

Note that 5f54ed1 is a "CS fix" commit. I don't want to open a PR just for that. Let me know if I should to remove it.

Also, I'm 👍 on this PR. Review has been made already, so it seems mergeable.

c33s pushed a commit to c33s/Propel that referenced this pull request Feb 5, 2014

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