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.

Sign up for GitHub

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

Jump to bottom

feat: do not throw exceptions during transactions by default #6917

Merged
merged 17 commits into from
Dec 11, 2022
Merged

feat: do not throw exceptions during transactions by default #6917

merged 17 commits into from
Dec 11, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 27, 2022 •
edited
Loading

Needs #6919
Supersedes #6886

Description

  • [BC] do not throw exceptions during transactions by default
  • add $db->transException(true) to throw exceptions
  • update docs
  • add tests

Ref: #6909

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.3 labels Nov 27, 2022
@kenjis kenjis marked this pull request as draft November 27, 2022 12:38
@kenjis kenjis force-pushed the feat-db-trans-suppress-expression branch 2 times, most recently from 2ca5a70 to fb5e3e7 Compare November 27, 2022 13:28
@kenjis kenjis mentioned this pull request Nov 27, 2022
Open
3 tasks
@kenjis kenjis added the database Issues or pull requests that affect the database layer label Nov 27, 2022
@kenjis kenjis mentioned this pull request Nov 27, 2022
Merged
4 tasks
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Nov 27, 2022
@kenjis kenjis mentioned this pull request Nov 27, 2022
Merged
3 tasks
@kenjis kenjis force-pushed the feat-db-trans-suppress-expression branch from 13ddde2 to b05af11 Compare November 28, 2022 00:46
michalsn
michalsn approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Handling transactions with exceptions is not easy. Our current approach with transStatus() seems to be more natural.

kenjis and others added 17 commits November 30, 2022 09:03
@kenjis
docs: update description
9f8c846
@kenjis
test: add DB transaction tests
43c1803
@kenjis
test: add tests for transStrict()
e18cb8d
@kenjis @sclubricants
test: add assertions
41fd753 
Co-authored-by: sclubricants <[email protected]>
@kenjis
test: fix setUp()
84d2b11
@kenjis
docs: add notes for DBDebug
3d3bf64
@kenjis
docs: add comment
f9658b8
@kenjis @MGatner
docs: fix typo
58cfbd1 
Co-authored-by: MGatner <[email protected]>
@kenjis
docs: add about DB transactions in changelog
821913c
@kenjis
test: add assertion
523d14e
@kenjis
docs: fix description
338345f 
All the current DB drivers throw an exception when a query error occurs.
See codeigniter4#6912

MySQLi driver sets `mysqli_report(MYSQLI_REPORT_ALL & ~MYSQLI_REPORT_INDEX)`
that throws mysqli_sql_exception.

SQLite3, Postgres and OCI8 drivers throw ErrorException because of Error Handler.

SQLSRV driver has different implementaion. It throws DatabaseException when the
query return value is false.
@kenjis
docs: add exception class name
5d6a907
@kenjis
docs: add sample code when DBDebug is true
b2e90d7
@kenjis
feat: do not throw exceptions during transactions by default
d27f133
@kenjis
test: add tests
8efd2c7
@kenjis
docs: update docs
ff2d8b7
@kenjis
docs: add about BaseConnection::transException()
0308366
@kenjis kenjis force-pushed the feat-db-trans-suppress-expression branch from b05af11 to 0308366 Compare November 30, 2022 00:07
@kenjis kenjis marked this pull request as ready for review November 30, 2022 00:09
@kenjis
Copy link
Member Author

kenjis commented Nov 30, 2022

Rebased, and this PR is ready.

@codeigniter4/database-team Please review.

@michalsn
Copy link
Member

michalsn commented Dec 5, 2022

I guess that breaking change is valid only for development mode? CI 4.3 wasn't released yet.

@kenjis
Copy link
Member Author

kenjis commented Dec 6, 2022 •
edited
Loading

This breaking change is only when DBDebug is true.
If a dev uses the default value (ENVIRONMENT !== 'production'), this does not change the behavior in production.

@michalsn
Copy link
Member

michalsn commented Dec 7, 2022

Okay, so the impact is minimal.

@kenjis kenjis merged commit bb2e614 into codeigniter4:4.3 Dec 11, 2022
@kenjis kenjis deleted the feat-db-trans-suppress-expression branch December 11, 2022 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalsn michalsn michalsn approved these changes

Assignees
No one assigned
Labels
4.3 breaking change Pull requests that may break existing functionalities database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kenjis @michalsn