"Cannot load a NULL ID" assertion in EntityStorageBase should say the entity type

+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -246,7 +246,7 @@ protected function doCreate(array $values) {
+    assert(!is_null($id), sprintf('Cannot load a NULL ID %s entity.', $this->entityTypeId));

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -564,7 +564,7 @@ public function testLoad() {
+    $this->expectExceptionMessage('Cannot load a NULL ID test_entity_type entity.');

Should this perhaps say something more along the lines of "Cannot load a NULL ID from %s entity" or similar?

from / on / etc.

"Cannot load a NULL ID test_entity_type entity." just doesn't read right to me.

Personally I think that reads better.

How about "Cannot load the test_entity_type entity with NULL ID"? Or possibly omit the "the"?

Marking NW to update the message as noted in either #4 or #5.

+1 for this update though.

I vote for the following

Cannot load %s with NULL ID

Comment #5 looks good to me, so I have done the changes accordingly.

I have again added a patch.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
@@ -564,7 +564,7 @@ public function testLoad() {
+    $this->expectExceptionMessage('Cannot load the test_entity_type entity with NULL ID.');

Let's not hardcode the entity type ID here, but use the one defined in the class property.

I don't care that much about the message, but would surround the entity type with double quotes.

Here I have uploaded a patch.

changed:
$this->entityTypeId
to
Entity::$entityTypeId

added double quotes

concatenating entityTypeId in the middle.

The test failed probably because he passed 2 arguments and expectExceptionMessage method only accepts 1 arg.

Added double quotes and refactored naming of the files uploaded.

Looks good. Thank you.

I have one concern here: there are no test failures by changing this error message, which means there are no tests for loading an entity by a null ID. If there was full test coverage this string change would have caused some kind of test failure.

We don't have tests for passing NULL because this wasn't a supported use case. The assertion itself was introduced recently. See the issue summary from the corresponding issue #3035980: Provide a better error when a NULL is passed to EntityStorageBase::load():

When a null value is passed into EntityStorageBase::load() it gets passed into loadMultiple as [0 => NULL], which triggers this warning that doesn't provide much help to developers:

Warning : array_flip(): Can only flip STRING and INTEGER values!

.

I have one concern here: there are no test failures by changing this error message, which means there are no tests for loading an entity by a null ID. If there was full test coverage this string change would have caused some kind of test failure.

We actually do have a test that the assert is thrown, and it is checking for the message in Drupal\Tests\Core\Config\Entity\ConfigEntityStorageTest::testLoad():

    $this->expectException(\AssertionError::class);
    $this->expectExceptionMessage('Cannot load a NULL ID.');
    $this->entityStorage->load(NULL);

however, the patch in #20 correctly updates the expected message in the test, so the test passes.

Committed and pushed c0f736b320 to 9.0.x and 43b42be33c to 8.9.x and 6ca0c8cca3 to 8.8.x. Thanks!

Backported to 8.8.x because the new exception message is helpful.

#28: @jhedstrom d'oh! I completely missed that. Thanks!