Skip to content

Model - Fix zero value primary key#4975

Closed
najdanovicivan wants to merge 1 commit intocodeigniter4:developfrom
najdanovicivan:model-zero-id
Closed

Model - Fix zero value primary key#4975
najdanovicivan wants to merge 1 commit intocodeigniter4:developfrom
najdanovicivan:model-zero-id

Conversation

@najdanovicivan
Copy link
Copy Markdown
Contributor

@najdanovicivan najdanovicivan commented Jul 28, 2021

Description
Primary key in SQL can have value of 0 or empty string. This modifies the model to use isset to handle such values instead of doing an empty check so that rows which such keys can be inserted/updated in the DB

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@najdanovicivan najdanovicivan force-pushed the model-zero-id branch 2 times, most recently from 2474ce9 to ff8e16e Compare July 28, 2021 18:35
paulbalandan
paulbalandan reviewed Jul 29, 2021
View reviewed changes
MGatner
MGatner requested changes Jul 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests for primary keys of '', 0, '0', false, and 'false'.

Also while you're here, can we replace those two awful composite if statements with a static function? Something like getObjectPrimaryKey()

@lonnieezell
Copy link
Copy Markdown
Member

lonnieezell commented Aug 12, 2021

@najdanovicivan are you able to make these changes? If not, I'm closing.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Aug 12, 2021

This also needs to be directed to 4.2 branch.

@najdanovicivan
Copy link
Copy Markdown
Contributor Author

najdanovicivan commented Aug 12, 2021

I’m on it.

@najdanovicivan
Copy link
Copy Markdown
Contributor Author

najdanovicivan commented Aug 12, 2021

@MGatner we already have getIdValue() method so I've used it in place of those if statements

Regarding the other place where we have those nasty if statements it is a deprecated static method and because it's static we cannot call getIdValue() there so I left it unchanged is should be removed in future releases anyway

I also added couple of tests. I haven't worked with DataBase test before on CI so it took me a while to figure it out how those work.

@najdanovicivan najdanovicivan force-pushed the model-zero-id branch 2 times, most recently from 8d88227 to c03d2de Compare August 12, 2021 19:50
@najdanovicivan najdanovicivan changed the base branch from develop to 4.2 August 12, 2021 19:51
@najdanovicivan najdanovicivan force-pushed the model-zero-id branch 2 times, most recently from faf9cbc to 2c32842 Compare August 12, 2021 21:59
@najdanovicivan
Copy link
Copy Markdown
Contributor Author

najdanovicivan commented Aug 12, 2021

I'm not sure what's going on with test on 4.2 branch some of them are failing.

What I also see now that MSSQL, SQLite and Postgres behave differently with empty values

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Aug 13, 2021

It looks like the failing tests are related to the way soft deletes are handled with primary keys. I'm on mobile so I can't dig very far right now, but some other tests might need changing for this to work.

In general we have a few open issues/PRs around boolean/empty values in the database layer. I think these will all need to be pulled together and a final decision made on how to handle things before 4.2 will be predictably consistent.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Sep 8, 2021

4.2 branch has been rebased and merged into develop! Those outstanding changes should all be in place - @najdanovicivan could you rebase this and point it back to develop? That should give us a good idea of what remains to be fixed.

@najdanovicivan najdanovicivan changed the base branch from 4.2 to develop September 11, 2021 06:14
@najdanovicivan @paulbalandan
Model - Fix zero value primary key
82ede90 
Primary key in SQL can have value of 0 or empty string. This modifies the model to use isset to handle such values instead of doing an empty chech so that rows wich such keys can be inserted/updated in the DB

Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
@MGatner
Copy link
Copy Markdown
Member

MGatner commented Sep 11, 2021

Tests are still failing.

@kenjis kenjis added the stale Pull requests with conflicts label Nov 2, 2021
@kenjis kenjis added database Issues or pull requests that affect the database layer bug Verified issues on the current code behavior or pull requests that will fix them labels Nov 16, 2021
@paulbalandan
Copy link
Copy Markdown
Member

paulbalandan commented Jan 23, 2022

I am seeing no progress in this since September. Please open a new PR with updated code where tests are passing.

@ECode16 ECode16 mentioned this pull request May 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulbalandan paulbalandan paulbalandan left review comments

@MGatner MGatner Awaiting requested review from MGatner

Assignees

No one assigned

Labels

bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer stale Pull requests with conflicts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@najdanovicivan @lonnieezell @MGatner @paulbalandan @kenjis