Bad error handling of invalid Entity definition

Why don't we just let the exception bubble up?

Small things matter!

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -476,7 +476,7 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
       if (!isset($base_field_definitions[$field_name])) {
...
-          '@field' => $base_field_definitions[$field_name]->getLabel(),

Nice find. Kind of an entertaining bug actually -- if the thing doesn't exist, then get its label! :)

I don't really think there's a test to be added here for the reasons described in #7. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks!

Hey there, participating at Drupal Con Sprint in LA, was checking out the issue #2481547: attempt to throw an exception for missing entity field crashes and noted that its been fixed here. Yet @alexpott mentioned that a test needs to be written here:

Nice find.

I think we should be able to test this in EntityManagerTest - see testGetBaseFieldDefinitionsTranslatableEntityTypeLangcode and testGetBaseFieldDefinitionsInvalidDefinition for clues as to how.

Here you go with a patch to the EntityManagerTest class for this case

Thanks for working on the issue! Keep up the good work :)

1. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -539,6 +539,24 @@ public function testGetFieldDefinitions() {
   +  ¶
   ...
   +   * ¶
   ...
   +   * ¶
   ...
   +    ¶
   ...
   +    ¶
   

   These extra spaces needs to be removed.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -539,6 +539,24 @@ public function testGetFieldDefinitions() {
   +   * Tests The buildBaseFieldDefinitions() method with an invalid key
   ...
   +    // Load the class object to remove its baseFieldDefinitions
   

   Comments needs to be ended with .

Is it good to go now ?

There was still some extra spaces. I also did some cleaning for the documentation. You can use Dreditor browser plugin to review patches and see this kind of things. This test doesn't still test anything because there is not any assertions?

well the "assertion" comes from the expected exception that should be thrown. It makes sure the exception gets thrown properly and don't crash

Thanks for the cleaning :-)

To make this a proper test shouldn't there be a try/catch to catch the expected exception and then put an assertion on the catch. As a test this will currently just crash and then do nothing.

Oh cool! does it mean this code below is wrong/unnecessary? I was working with in on another issue so I assumed it was the way to do things. ModuleImplementsAlterTest.php

  function testModuleImplementsAlterNonExistingImplementation() {

    // Install the module_test module.
    \Drupal::service('module_installer')->install(array('module_test'));

    try {
      // Trigger hook discovery.
      \Drupal::moduleHandler()->getImplementations('unimplemented_test_hook');
      $this->fail('An exception should be thrown for the non-existing implementation.');
    }
    catch (\RuntimeException $e) {
      $this->pass('An exception should be thrown for the non-existing implementation.');
      $this->assertEqual($e->getMessage(), 'An invalid implementation module_test_unimplemented_test_hook was added by hook_module_implements_alter()');
    }
  }

Seeing the commit and lack of subsequent activity, I'm thinking that this issue should have been marked fixed. Please, correct me if I'm wrong.

I created a follow up to add the test coverage: #2724867: Create tests for invalid Entity definition error handling