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

CommentFileSizeAuthor
#17 entity-exception-handling-1634442-17.patch4.75 KBamateescu
#15 entity-exception-handling-1634442-15.patch4.82 KBamateescu
#14 entity-exception-handling-1634442-14.patch5.28 KBBerdir
#12 entity-exception-handling-1634442-12-interdiff.txt4.43 KBBerdir
#12 entity-exception-handling-1634442-12.patch5.42 KBBerdir
#12 entity-exception-handling-1634442-12-tests-only.patch4.62 KBBerdir
#8 entity-storage-exception-testing-1634442.patch5.69 KBchia
#5 entity-exception-catching-tests-1634442.patch5.25 KBchia
entity_exception.patch450 bytesfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Component: user system » entity system
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Issue tags: +Entity system, +sprint
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

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

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Needs work
Issue tags: +Needs work

Agreed. Let's do that.

chia’s picture

chia CreditAttribution: chia commented
FileSize
entity-exception-catching-tests-1634442.patch5.25 KB

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

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs work » Needs review
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
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

chia CreditAttribution: chia commented
Assigned: Unassigned » chia
Status: Needs work » Needs review
FileSize
entity-storage-exception-testing-1634442.patch5.69 KB

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: -Entity system, -Needs work, -sprint

#8: entity-storage-exception-testing-1634442.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#8: entity-storage-exception-testing-1634442.patch queued for re-testing.

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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
entity-exception-handling-1634442-12-tests-only.patch4.62 KB
entity-exception-handling-1634442-12.patch5.42 KB
entity-exception-handling-1634442-12-interdiff.txt4.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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
entity-exception-handling-1634442-14.patch5.28 KB

Re-roll.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs work
FileSize
entity-exception-handling-1634442-15.patch4.82 KB

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

catch
he/him
Primary language English
CreditAttribution: catch commented
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

amateescu CreditAttribution: amateescu commented
Status: Needs work » Reviewed & tested by the community
FileSize
entity-exception-handling-1634442-17.patch4.75 KB

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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

follow-up #2015849: Some standards fixes in entity_test.module and EntityApiTest.php, motivated by "DatabaseStorageController can't catch exceptions"