Bug 14708 introduced a try catch around $patron->delete in commit "Bug 14708: (QA follow-up) Use try/catch blocks when calling ". However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception. Kyle, can you have a go at this?
Created attachment 114149 [details] [review] Bug 27145: Rethrow all other exceptions Bug 14708 introduced a try catch around $patron->delete in commit "Bug 14708: (QA follow-up) Use try/catch blocks when calling ". However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception.
We should move $patron->move_to_deleted into the try and execute the two statements inside a transaction.
Created attachment 114183 [details] [review] Bug 27145: Wrap delete and move_to_deleted in a transaction
Created attachment 127545 [details] [review] Bug 27145: Rethrow all other exceptions Bug 14708 introduced a try catch around $patron->delete in commit: "Bug 14708: (QA follow-up) Use try/catch blocks when calling" However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 127546 [details] [review] Bug 27145: Wrap delete and move_to_deleted in a transaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The change makes allot of sense, though it would be nice to do something with the other types of failures we're currently rethrowing.. but it's no bad thing to rethrow for now and collect data on what they are so we can handle them later (in new bug reports).. in my opinion. Signing off.
Created attachment 127608 [details] [review] Bug 27145: Rethrow all other exceptions Bug 14708 introduced a try catch around $patron->delete in commit: "Bug 14708: (QA follow-up) Use try/catch blocks when calling" However, in the catch block it only assumes the exception was from trying to delete anonymous patron when it can be anything else as well, the code should be modified so that it will handle the anonymous patron case and if it is anything else we log the other exception. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127609 [details] [review] Bug 27145: Wrap delete and move_to_deleted in a transaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Pushed to 20.11.x for 20.11.12
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.