The exception class is not imported, so it won't work. Patch attached.

Files: 
CommentFileSizeAuthor
#17 entity-exception-handling-1634442-17.patch4.75 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 55,947 pass(es).
[ View ]
#15 entity-exception-handling-1634442-15.patch4.82 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 55,795 pass(es).
[ View ]
#14 entity-exception-handling-1634442-14.patch5.28 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,747 pass(es).
[ View ]
#12 entity-exception-handling-1634442-12-interdiff.txt4.43 KBBerdir
#12 entity-exception-handling-1634442-12.patch5.42 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,745 pass(es).
[ View ]
#12 entity-exception-handling-1634442-12-tests-only.patch4.62 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 40,742 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#8 entity-storage-exception-testing-1634442.patch5.69 KBchia
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]
#5 entity-exception-catching-tests-1634442.patch5.25 KBchia
PASSED: [[SimpleTest]]: [MySQL] 36,896 pass(es).
[ View ]
entity_exception.patch450 bytesfago
PASSED: [[SimpleTest]]: [MySQL] 36,749 pass(es).
[ View ]

Comments

fago’s picture

Component:user system» entity system
fago’s picture

Issue tags:+Entity system, +sprint
Berdir’s picture

Related #1553244: Incorrect method calls in exception handling code in EntityDatabaseStorageController

Change looks good, having a test that triggers an exception would be useful I think, as described in the other issue.

fago’s picture

Status:Needs review» Needs work
Issue tags:+Needs work

Agreed. Let's do that.

chia’s picture

StatusFileSize
new5.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,896 pass(es).
[ View ]

Little tests to see if exceptions are working okay, may be we can improve the messages,

fago’s picture

Status:Needs work» Needs review
fago’s picture

Status:Needs review» Needs work
+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityExceptionTest.php
@@ -0,0 +1,82 @@
+    $user1 = $this->drupalCreateUser();
+    ¶
+    variable_del('entity_exception_test_throw_exception');
+    $entity = entity_create('entity_test', array('name' => 'test', 'uid' => $user1->uid));

You got some whitespaces.. ;)

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityExceptionTest.php
@@ -0,0 +1,82 @@
+      // create a test entity

Always start with an uppercase later and end with a dot. Also maybe move the comment before entity_create().

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityExceptionTest.php
@@ -0,0 +1,82 @@
+      ¶
+    } catch(EntityStorageException $e) {

catch should be at a new line.

chia’s picture

Assigned:Unassigned» chia
Status:Needs work» Needs review
StatusFileSize
new5.69 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

Another patch with better naming and fixed the above mentioned issues ;)

Berdir’s picture

Issue tags:-Entity system, -Needs work, -sprint
Berdir’s picture

Status:Needs review» Needs work
Issue tags:+Entity system, +Needs work, +sprint

The last submitted patch, entity-storage-exception-testing-1634442.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new4.62 KB
FAILED: [[SimpleTest]]: [MySQL] 40,742 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.42 KB
PASSED: [[SimpleTest]]: [MySQL] 40,745 pass(es).
[ View ]
new4.43 KB

Re-rolled, cleaned up the tests a bit and a tests-only patch to make sure that this works. I hope this doesn't blow up the bot due to another missing use there, see #1759026: Testing broken: Exception is never caught. It will certainly conflict with it since it adds a fix for this as well.

webchick’s picture

Status:Needs review» Needs work

I committed the referenced issue, so this will need a re-roll.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new5.28 KB
PASSED: [[SimpleTest]]: [MySQL] 40,747 pass(es).
[ View ]

Re-roll.

amateescu’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs work
StatusFileSize
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,795 pass(es).
[ View ]

Updated the test to be a EntityUnitTest and cleaned up the patch a litte. I think this is good to go now.

catch’s picture

Status:Reviewed & tested by the community» Needs work

The test uses state but everything runs in one request.

Couldn't we set $entity->throw_exception to TRUE directly - would save the hook_node_presave() and the db hits/state dependency in the test.

amateescu’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new4.75 KB
PASSED: [[SimpleTest]]: [MySQL] 55,947 pass(es).
[ View ]

Discussed in IRC and decided to drop the $entity->throw_exception stuff because the storage controller doesn't have a way to know about it, and just use $GLOBALS instead of the state system, which makes perfect sense.

Berdir’s picture

Hm :)

We're in a DUBT test, so state() (or actually, it should use Drupal::state() within the hooks as well) uses the memory backend. Using it seems cleaner to me than $GLOBALS and is essentially the same. I've written other tests before that rely on state for DUBT tests, see for example \Drupal\system\Tests\DrupalKernel\ServiceDestructionTest.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

I'm not fussed if we use globals or state... I like I prefer globals because it is one less dependency.

Committed ce7ff5c and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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