Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

kiwimind’s picture

+++ 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.

joachim’s picture

> "Cannot load a NULL ID from %s entity" or similar?

We're not trying to load something *from* an entity, we're trying to load an entity, so I don't think that reads right either.

"Cannot load a test_entity_type entity with NULL ID." might be better, but then there's the a/an problem.

kiwimind’s picture

Personally I think that reads better.

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

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jhedstrom’s picture

Status: Needs review » Needs work

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

+1 for this update though.

mglaman’s picture

I vote for the following

Cannot load %s with NULL ID
ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
1.31 KB

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

Status: Needs review » Needs work

The last submitted patch, 9: 3086850-9.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

I have again added a patch.

hchonov’s picture

Status: Needs review » Needs work
+++ 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.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

Here I have uploaded a patch.

Status: Needs review » Needs work

The last submitted patch, 13: 3086850-13.patch, failed testing. View results

MasterBranch’s picture

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

MasterBranch’s picture

added double quotes

MasterBranch’s picture

concatenating entityTypeId in the middle.

joachim’s picture

@MasterBranch you're uploading an incremental patch rather than the whole thing.

Also, rather than nitpicking on the code style, we should be looking at the test failure.

MasterBranch’s picture

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

MasterBranch’s picture

Added double quotes and refactored naming of the files uploaded.

MasterBranch’s picture

Status: Needs work » Needs review

The last submitted patch, 15: differences.patch, failed testing. View results

The last submitted patch, 16: differences_v2.patch, failed testing. View results

mglaman’s picture

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.

The last submitted patch, 17: differences_v3.patch, failed testing. View results

hchonov’s picture

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!

.

jhedstrom’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed c0f736b on 9.0.x
    Issue #3086850 by MasterBranch, ravi.shankar, joachim, hchonov, kiwimind...

  • alexpott committed 43b42be on 8.9.x
    Issue #3086850 by MasterBranch, ravi.shankar, joachim, hchonov, kiwimind...

  • alexpott committed 6ca0c8c on 8.8.x
    Issue #3086850 by MasterBranch, ravi.shankar, joachim, hchonov, kiwimind...
mglaman’s picture

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

Status: Fixed » Closed (fixed)

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