Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Add support for non-empty string as a primaryKey in update() #1325

Closed
wants to merge 7 commits into from

Conversation

nowackipawel
Copy link
Contributor

Description
In current code every update which contains ID in string format was ignored inside update() method.
Actually in docs (and comments) there is an information that $id passed to update() method can contain an array... - I think it won't work as well; I will try to write a fix in a week but I am not sure about exact date so you can merge this little fix without waiting.

In current code every update which contains ID in string format was ignored inside update() method.
Actually in docs (and comments) there is an information that $id passed to update() method can contain an array... - I think it won't work as well; I will try to write a fix in a week but I am not sure about exact date so you can merge this little fix without waiting.
Makes update more universal i.e. for comlpex primary keys
$id = 1'll produce: WHERE id = 1
$id = 'aaa''ll produce WHERE id = 'aaa'
$id = [2,3,4]'ll produce WHERE id IN(2,3,4);
$id = ['a' => 3, 'b'=>5]'ll produce  WHERE a = 3 AND b = 5
@nowackipawel
Copy link
Contributor Author

ps. lemme know if you will be able to merge this, i don't wanna touch any docs before you make me sure it will merge it cuz entire description of update() method is ...let's be gently... a mess [https://bcit-ci.github.io/CodeIgniter4/database/query_builder.html -> update()]

@lonnieezell
Copy link
Member

Please provide some tests for this feature, and I'm good with it.

Test cases: update    $id -> string || $id -> assoc|index array
@nowackipawel
Copy link
Contributor Author

@lonnieezell: pls chk it.

@lonnieezell
Copy link
Member

For starters - you've got a test breaking:

CodeIgniter\Database\Live\ModelTest::testSetWorksWithUpdateString

Click on the Travis Details link to see specifics.

@nowackipawel
Copy link
Contributor Author

It is working now for mysqli and sqlite. I am not sure how to fix this test for postgress.

@jim-parry jim-parry added the help wanted More help is needed for the proper resolution of an issue or pull request label Oct 29, 2018
@jim-parry jim-parry changed the title Add support for non-empty string as a primaryKey in update() WIP Add support for non-empty string as a primaryKey in update() Nov 14, 2018
nowackipawel added a commit to nowackipawel/ci4-old that referenced this pull request Jan 2, 2019
Replacement for codeigniter4#1441  and codeigniter4#1325  + improvements (outer function for parsing id, and better condition checking).

Allows to pass more complex data for update and delete methods.
Before delete and update supports only numeric $id. and produces: WHERE primaryKey IN(1); or WHERE primaryKey IN(2,3,4) - for arrays.

I've added support for:
- non empty string -> WHERE table.primaryKey = 'value'    `$id = 'value'`  (it was wired it doesn't support it and we are thinking about production version of ci4)
- associative arrays -> WHERE table.field = 'value'  `$id =['field' => 'value']`
- more complex associative arrays -> WHERE table.field = 'value' AND table.fieldin IN(2,3,4) `$id =['field' => 'value', 'fieldin' => [2,3,4]]`
@nowackipawel
Copy link
Contributor Author

I think it could be closed according to #1644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted More help is needed for the proper resolution of an issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants