Open apinstein opened 12 years ago
Is your fix good enough ? Can you open a PR with unit tests ?
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.
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?
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 inPHP5ObjectBuilder
, 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: https://github.com/propelorm/Propel/issues/267#issuecomment-3936723
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.
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)
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.
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.