Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for editable id columns #610

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

tefra commented Aug 12, 2012

Currently the id column can't change value, because the function _entityConditions of class lithium\data\model\Query returns a condition based on the new value instead of the old.

tefra added some commits Aug 12, 2012

Fix for editable id columns
Added function 'stored' for easy access to the currently stored data
Fix for editable id columns
Provide the stored data in the function _entityConditions to get editable id columns

This pull request fails (merged 3016730 into c3419a4).

Fix the fix for the editable id column
With the previous fix i managed to break the create, it should be fine now

This pull request fails (merged 9a15650 into c3419a4).

Contributor

jails commented Aug 12, 2012

Thanks for your PR, but imho the actual behavior seems correct. In what kind of use case your PR is needed ?

Contributor

tefra commented Aug 13, 2012

You are thinking about tables with auto-increment integer for primary key, in that case yes it's bad practise, but even so to have the following code

$foo = Foo::find(15);
$foo->id = 14;
$foo->save();

produce this

UPDATE foo SET id = 14 WHERE id = 14;

is wrong...

Now about my case i have a table with a varchar pk that i need it to be editable, instead i would have to add an AI int for no reason and use something like this to find them

$foo = Foo::findByAlias('main');
Contributor

jails commented Aug 14, 2012

Makes sense. However this PR seems using 4 spaces instead of tab for indenting your code and break some tests (and would need it's own test case). But it's preferable to wait for @nateabele comments before modifying something ;-).

This pull request fails (merged 7151dd6 into c3419a4).

This pull request fails (merged 110e531 into c3419a4).

@tefra tefra closed this Jan 14, 2013

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