Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
See #2824851-205: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-206: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-207: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.
Details to follow.
Proposed resolution
Ensure this doesn't result in a fatal error.
Remaining tasks
- Patch that adds failing test.
- Make test pass by adding a solution.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2930968-26.patch | 904 bytes | Ankit.Gupta |
#24 | 2930968-24.patch | 904 bytes | mstrelan |
#6 | 2930968-6.patch | 903 bytes | cburschka |
#3 | 2930968-2.patch | 2.3 KB | cburschka |
#3 | 2930968-2-crosstest-2824851-203.patch | 22.57 KB | cburschka |
Comments
Comment #2
BerdirI would expect that should be done on validation, might be related to #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field, if the problem indeed only happens when saving. It would be a rest-specific problem if doing that would completely break the entity, so that validation wouldn't work anymore.
Comment #3
cburschkaThis needs to be tested together with #2824851-203: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, since the request will otherwise be denied without hitting the error.
(Should return an HTTP 500.)
Comment #4
Wim Leers+1
Comment #5
Wim Leers#3: I think the fatal error should be reproducible even without using REST.
Something like:
… should trigger the same fatal error.
Such a test would not depend on #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior landing first.
Comment #6
cburschkaYou're right, this is much simpler.
(Adding this to FieldTranslationSqlStorageTest, since it's vaguely related.)
Comment #7
Wim Leers👌
Comment #8
BerdirAn invalid entity isn't prevented from being saved, you need to explicitly assert that there are validation errors.
Comment #11
Wim LeersThe point was to have a failing test, which we now do have. Adding validation plus explicit validation error assertion is the next step, right?
Comment #12
Wim LeersOr, put differently… @Berdir wrote in #2:
#6 does prove that this is a problem only when saving, because in its backtrace, it lists:
Both of which are
save()
methods.Plus, #6 of course doesn't use the REST module at all, which means it definitely proves this is not a REST-specific problem.
Comment #13
Wim Leers@effulgentsia opened the related #2933408: Remove broken allowance of empty langcode in a PATCH request.
Comment #24
mstrelan CreditAttribution: mstrelan at PreviousNext commentedReviewing this as part of the Bug Smash Initiative. Patch in #6 was no longer applying so I've re-rolled it. The patch still fails for me so I assume this is still an issue, didn't quite follow all the related issues. Would be good to have a concise description of the problem in the issue summary.
Comment #26
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedComment #27
mstrelan CreditAttribution: mstrelan at PreviousNext commentedThis is the second time I've seen Ankit.Gupta repost the exact same patch someone else posted. Unchecking the suggested credit box and setting back to NW.