Download & Extend
Issues

DatabaseControllerNG does not rollback failed ::save() operations

Posted by larowlan on January 10, 2013 at 8:32pm
Project:Drupal core
Version:8.x-dev
Component:entity system
Category:bug report
Priority:major
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

Missing use Exception; so the catch (Exception $e) does not catch \Exception.

Patch and tests to follow.

Login or register to post comments

Comments

#1

Posted by larowlan on January 10, 2013 at 8:58pm
Status:active» needs review

Patch with test demonstrating fail and then fix.

AttachmentSizeStatusTest resultOperations
database-storage-controller-ng-1885542.1.fail_.patch2.7 KBIdleFAILED: [[SimpleTest]]: [MySQL] 49,499 pass(es), 2 fail(s), and 0 exception(s).View details
database-storage-controller-ng-1885542.1.pass_.patch3.16 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,534 pass(es).View details

#2

Posted by kim.pepper on January 10, 2013 at 10:14pm
Status:needs review» reviewed & tested by the community

Looks good to me.

#3

Posted by jhodgdon on January 10, 2013 at 10:28pm
Component:documentation» database system

Not a documentation issue.

#4

Posted by larowlan on January 10, 2013 at 10:47pm
Component:database system» entity system

sorry @jhodgdon

#5

Posted by webchick on January 10, 2013 at 10:48pm
Component:entity system» database system
Status:reviewed & tested by the community» needs work

This is somewhat minor, but AFAIK the standard is not to use Exception but instead to specify \Exception where it is used.

#6

Posted by webchick on January 10, 2013 at 10:48pm
Component:database system» entity system

Cross-post-o-rama!

#7

Posted by larowlan on January 10, 2013 at 10:52pm

@webchick, http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D... has 'use Exception', I'm not fussy either way - can change both if you like.

#8

Posted by webchick on January 10, 2013 at 10:57pm

Yeah, that's wrong. The reason for that is because if you forget the "use Exception" thing at the top, then basically stuff doesn't work. Since it's not easy to grep for ensuring for every "Exception" there's a corresponding "use Exception", we introduced a standard to prefix it with "\" everywhere so offenders are easier to pick out. This is true for any internal PHP class: See http://drupal.org/node/1353118 under "use"ing classes.

#9

Posted by larowlan on January 10, 2013 at 11:07pm
Status:needs work» needs review

thanks, again in awe of how you keep all this in your head, new patch fixed both files.
sorry, no interdiff

AttachmentSizeStatusTest resultOperations
database-storage-controller-ng-1885542.9.patch4.05 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch database-storage-controller-ng-1885542.9.patch. Unable to apply patch. See the log in the details link for more information.View details

#10

Posted by System Message on January 10, 2013 at 11:09pm
Status:needs review» needs work

The last submitted patch, database-storage-controller-ng-1885542.9.patch, failed testing.

#11

Posted by larowlan on January 10, 2013 at 11:21pm
Status:needs work» needs review

related #1634442: DatabaseStorageController can't catch exceptions

#12

Posted by larowlan on January 11, 2013 at 2:29am

Not sure what happened there, fresh re-roll against HEAD.

AttachmentSizeStatusTest resultOperations
database-storage-controller-ng-1885542.12.patch4.08 KBIdleFAILED: [[SimpleTest]]: [MySQL] 49,533 pass(es), 2 fail(s), and 0 exception(s).View details

#13

Posted by tim.plunkett on January 11, 2013 at 3:33am
Status:needs review» reviewed & tested by the community

Looks sane enough.

#14

Posted by System Message on January 11, 2013 at 3:45am
Status:reviewed & tested by the community» needs work

The last submitted patch, database-storage-controller-ng-1885542.12.patch, failed testing.

#15

Posted by tim.plunkett on January 11, 2013 at 3:46am

I'm missing something. Hmmm

#16

Posted by larowlan on January 11, 2013 at 3:51am
Status:needs work» needs review

meh, missed one
find and replace fail.

AttachmentSizeStatusTest resultOperations
database-storage-controller-ng-1885542.15.interdiff.txt607 bytesIgnored: Check issue status.NoneNone
database-storage-controller-ng-1885542.15.patch4.39 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,547 pass(es).View details

#17

Posted by webchick on January 11, 2013 at 4:54am
Status:needs review» reviewed & tested by the community

Looks good. Marking back to RTBC so it can be committed once tesbot's done verifying.

#18

Posted by sun on January 11, 2013 at 4:55am
Status:reviewed & tested by the community» needs review

Good catch.

Can we have a test-only patch to prove the test?

#19

Posted by tim.plunkett on January 11, 2013 at 5:14am
Status:needs review» reviewed & tested by the community

Wait for red/green.

AttachmentSizeStatusTest resultOperations
exception-1885542-19-FAIL.patch2.69 KBIdleFAILED: [[SimpleTest]]: [MySQL] 49,537 pass(es), 1 fail(s), and 0 exception(s).View details
exception-1885542-19-PASS.patch4.39 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,608 pass(es).View details

#20

Posted by webchick on January 11, 2013 at 8:04am
Status:reviewed & tested by the community» fixed

Great, thanks!

Committed and pushed to 8.x.

#21

Posted by System Message on January 25, 2013 at 8:10am
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nobody click here