Problem/Motivation
Entity API tries to validate that an entity field definition array is valid. However, it's error handling has a fatal error. This makes figuring out what the problem is quite difficult.
Proposed resolution
Fix the fatal error, and catch the exception so we can report what the problem was. Yay, DX! The code will still fatal on an invalid entity definition, which is probably a good thing, but now it shows you what you did wrong.
Remaining tasks
Commit the patch?
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 1.21 KB | lauriii |
#18 | bad_error_handling_of-2457427-18.patch | 1.06 KB | lauriii |
#17 | 2457427-entity-error-test.diff | 1.19 KB | JulienF |
#15 | 2457427-entity-error-test.patch | 1.19 KB | JulienF |
#10 | 2457427-entity-error.patch | 802 bytes | Crell |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch.
Comment #3
XanoWhy don't we just let the exception bubble up?
Comment #4
Crell CreditAttribution: Crell at Palantir.net commentedBecause currently if we do that the error message is not displayed at all. You just get a generic "something bad happened" error, which is even less useful for debugging that you messed up your field definitions.
And I've no idea why tests won't even run, that's bizarre...
Comment #7
Crell CreditAttribution: Crell at Palantir.net commentedHm. OK then.
1) drupal_set_message() needed a \ prefix as it is in a namespace. However...
2) There are existing tests that are checking for a LogicException being thrown. Which does make sense after all, because...
3) LogicExceptions that bubble up DO get caught and a useful error displayed... IF you have error reporting turned up. Drupal now has it set to super-silent mode by default, which I didn't realize.
So, if we just fix the bug itself everything seems to work out in the end now. :-) (Not bothering with an interdiff as all I did was remove half of what I did before.)
Comment #10
Crell CreditAttribution: Crell at Palantir.net commentedRerolled.
Comment #11
dawehnerSmall things matter!
Comment #12
xjmNice 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!
Comment #14
JulienF CreditAttribution: JulienF commentedHey 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:
Comment #15
JulienF CreditAttribution: JulienF commentedHere you go with a patch to the EntityManagerTest class for this case
Comment #16
lauriiiThanks for working on the issue! Keep up the good work :)
These extra spaces needs to be removed.
Comments needs to be ended with .
Comment #17
JulienF CreditAttribution: JulienF at Eweev LTD commentedIs it good to go now ?
Comment #18
lauriiiThere 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?
Comment #19
JulienF CreditAttribution: JulienF at Eweev LTD commentedwell 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 :-)
Comment #20
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedTo 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.
Comment #21
Crell CreditAttribution: Crell as a volunteer commentedsmccabe: The @ExpectedException annotation on the test means that PHPUnit will take care of that for us. It will catch any exception, and the test will fail if it doesn't catch the one we said it should catch.
Comment #22
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedOh 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
Comment #23
Crell CreditAttribution: Crell as a volunteer commentedCorrect, that's unnecessary. And also a bit off topic for this thread, so if you've further questions please ask them in #drupal-contribute. Thanks. :-)
Comment #26
stpaultim CreditAttribution: stpaultim as a volunteer commentedSeeing 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.
Comment #27
lauriiiI created a follow up to add the test coverage: #2724867: Create tests for invalid Entity definition error handling