PHP5ObjectBuilder fails to cast generated primary keys properly for native autoincrement (ie postgres) primary keys

[apinstein]

This bug isn't exposed unless using PHP 5.3. There appears to have been a change in PDOStatement::fetch() where the result from a "select nextval(...)" for postgres sequences is now returned as a string instead of an integer.

This exposed a bug in the assignment of generated ID's in the BaseObject where a value is assigned to $this->{$primaryKey} without a cast.

[willdurand]

Is your fix good enough ? Can you open a PR with unit tests ?

[apinstein]

I do think my fix is good enough.

I tried to write tests but I couldn't even get the tests running... and these are generator tests so I am not sure what the protocol is for testing them. I didn't see a readme or anything.

If you can point me in the right direction for bootstrapping a test rig I'd be happy to do it. I just don't have time to figure it out on my own...

But the test would basically have to be run on a generated model I think. The way we found the bug is one of our model tests failed b/c we were asserting that the ID returned from a saved NEW object is a string and the ID returned from a ObjectPeer::retrieveByPK()->getPrimaryKeyId() is an integer. Thus they fail a === test.

Also I am not sure which platforms are affected other than postgres; I know postgres uses true sequences and the bug only happens on systems that use true sequences, otherwise a different mechanism is used.

If it's easier for you to add one test that tests that, you could easily create a failing test and merge this patch to see that it's fixed. Otherwise again if you can point me to good docs for bootstrapping a test rig I can try to add that test myself.

[fzaninotto]

If the bug relies in the way Propel retrieves identifiers in PostrgeSQL, the fix should be in PgsqlPlatform::getIdentifierPHP(), not in PHP5ObjectBuilder, where this may affect other platforms..

Why aren't the tests running? Can you paste the error you get?

[apinstein]

Ok I took a look at the README in test/ and got pretty far, but it looks like the builders are building for php 5.3 when I am still on php 5.2 and it produces code that won't run under 5.3.

I don't know if that means I found a legit bug or I haven't properly configured the tests for a 5.2 stack.

$ phpunit testsuite

PHP Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in /Users/alanpinstein/dev/sandbox/Propel/test/testsuite/generator/builder/om/QueryBuilderTest.php on line 228

Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in /Users/alanpinstein/dev/sandbox/Propel/test/testsuite/generator/builder/om/QueryBuilderTest.php on line 228

That LOC is:

            $this->assertFalse($q::$preSelectWasCalled);

Which is an obvious 5.2/5.3 fail.

Please advise, thanks!

Alan

On Feb 13, 2012, at 4:44 AM, Francois Zaninotto wrote:

If the bug relies in the way Propel retrieves identifiers in PostrgeSQL, the fix should be in PgsqlPlatform::getIdentifierPHP(), not in PHP5ObjectBuilder, where this may affect other platforms..

Why aren't the tests running? Can you paste the error you get?

---

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

[apinstein]

I am still not 100% sure how to test this. Should I test the output of the generated files? Did you look at my code and decide that this is a bug in PgsqlPlatform::getIdentifierPHP()?

LMK your thoughts.

[willdurand]

To test the generated code isn't really useful. You should set up a test which shows the use case (I don't know if it's easy or not there)

[apinstein]

I am not sure how to do that. It is an implementation bug, not a new use case... we noticed it quite simply because the API guarantees an integer return from getMyRowId() but it was returning a string, and this caused one of our tests to fail on a $id === 1.

If you can tell me how you'd test this sort of thing I'd be happy to write one, but I have no clue what is the appropriate way to do so.