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

Transaction rollback exception throwing should be optional #13949

Closed
david-duncan opened this issue Apr 5, 2019 · 1 comment

Comments

Projects
2 participants
@david-duncan
Copy link

commented Apr 5, 2019

Revival of #13324

Expected behavior

Rolling back a transaction should rollback a transaction. Not have unintended side effects.

Currently $transaction->rollback() throws an exception if successful, and if not, it just returns true

This makes exception handling on the implementation side unwieldy if the user wants a proper exception to be thrown out of their code, or if they want to resume operation post rollback

Current workaround if you don't want an exception throw

try {
catch (\Exception $e) {
  //something went wrong, roll back our transaction
  try {
    $transaction->rollback()
  catch (TransactionFailed $e) {
    //throw a proper userland exception, or swallow if behavior can be continued as normal
  }
}

Solutions

I see 4 options to be had here:

  1. Exception throwing is removed from rollback
  2. Exception throwing is made optional via an extra parameter
  3. Exception throwing is disabled via an extra parameter
  4. Scoping the boolean to the transaction class

1 makes the most sense to me but breaks anyone who is relying on this behavior. However, it seems to be the cleanest based on the existing method signature

3 seems to be the path of least resistance to implementation and would suffice.

@niden niden added the Bug - Medium label Apr 16, 2019

@niden niden added this to To do in 4.0 Release via automation Apr 16, 2019

@niden niden moved this from To do to In progress in 4.0 Release May 9, 2019

@niden niden referenced this issue May 9, 2019

Merged

T13949 rollback transaction exception #14053

4 of 4 tasks complete

niden added a commit that referenced this issue May 9, 2019

niden added a commit that referenced this issue May 9, 2019

niden added a commit that referenced this issue May 9, 2019

niden added a commit that referenced this issue May 9, 2019

niden added a commit that referenced this issue May 9, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Resolved in #14053

@niden niden closed this May 9, 2019

4.0 Release automation moved this from In progress to Done May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.