Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
ContentEntityStorageBase::loadRevision called with a NULL as parameter for the revision id will return an entity, which is wrong and should not happen as it might return an entity where no entity should be returned, which might happen lets say through a developer error.
ContentEntityStorageBase::loadRevision is calling SqlContentEntityStorageBase::doLoadRevisionFieldItems, which on its turn calls SqlContentEntityStorageBase::buildQuery([], $revision_id). However the buildQuery method is implemented this way that it will put a condition for the given revision id only if the parameter evaluates as TRUE otherwise without any conditions the query will return all the entities.
Proposed resolution
EntityStorageBase::load should check that the provided ID is different than NULL.
ContentEntityStorageBase::loadRevision should check that the provided revision ID evaluates to TRUE.
Remaining tasks
Review
User interface changes
none
API changes
none
Data model changes
none
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | interdiff-10-12.txt | 1.24 KB | hchonov |
| #12 | 2762737-12.patch | 2.37 KB | hchonov |
| #2 | 2762737-loadRevision-2-failing.patch | 1.14 KB | Maouna |
Comments
Comment #2
Maouna commentedHere is a test for demonstration:
Comment #3
Maouna commentedComment #5
Maouna commentedSame for 8.2.x
Comment #6
Maouna commentedComment #7
Maouna commentedComment #8
hchonovComment #10
hchonovComment #11
hchonovComment #12
hchonovComment #22
jhedstromThis appears to have been fixed as part of #1730874: Add support for loading multiple revisions at once. However, it does still throw the very unhelpful notice that
::loadused to throw if a null id was passed:but that can probably be addressed in #3037956: Improve DX of load / loadMultiple / loadRevisionload / loadMultipleRevisions.