Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It's not always clear which entity type is causing the problem. Specifying would improve the usefulness of this error message.
Comment | File | Size | Author |
---|---|---|---|
#20 | 3086850-20.patch | 1.34 KB | MasterBranch |
Comments
Comment #2
joachim CreditAttribution: joachim commentedComment #3
kiwimind CreditAttribution: kiwimind commentedShould 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.
Comment #4
joachim CreditAttribution: joachim commented> "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.
Comment #5
kiwimind CreditAttribution: kiwimind commentedPersonally I think that reads better.
How about "Cannot load the test_entity_type entity with NULL ID"? Or possibly omit the "the"?
Comment #7
jhedstromMarking NW to update the message as noted in either #4 or #5.
+1 for this update though.
Comment #8
mglamanI vote for the following
Comment #9
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #5 looks good to me, so I have done the changes accordingly.
Comment #11
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have again added a patch.
Comment #12
hchonovLet'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.
Comment #13
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have uploaded a patch.
Comment #15
MasterBranch CreditAttribution: MasterBranch commentedchanged:
$this->entityTypeId
to
Entity::$entityTypeId
Comment #16
MasterBranch CreditAttribution: MasterBranch commentedadded double quotes
Comment #17
MasterBranch CreditAttribution: MasterBranch commentedconcatenating entityTypeId in the middle.
Comment #18
joachim CreditAttribution: joachim commented@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.
Comment #19
MasterBranch CreditAttribution: MasterBranch commentedThe test failed probably because he passed 2 arguments and expectExceptionMessage method only accepts 1 arg.
Comment #20
MasterBranch CreditAttribution: MasterBranch commentedAdded double quotes and refactored naming of the files uploaded.
Comment #21
MasterBranch CreditAttribution: MasterBranch commentedComment #22
hchonovLooks good. Thank you.
Comment #25
mglamanI 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.
Comment #27
hchonovWe 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():
.
Comment #28
jhedstromWe 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()
:however, the patch in #20 correctly updates the expected message in the test, so the test passes.
Comment #29
alexpottCommitted 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.
Comment #33
mglaman#28: @jhedstrom d'oh! I completely missed that. Thanks!