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.This is a follow-up of #2928778: Exception when trying to save a new revision after manually setting the original revision ID.
That breaks our default content. It is possible that it only happens with the ERR patch that we use from #2848878: Embed Paragraph Content in Normalized Parent Entities (REST) because what I think happens is that we denormalize an entity with revision_id and then immediately setting it on the reference and relying on auto-save and it also calls setNewRevision() because the parent is saved as a new revision.
This seems to cause a regression in our install profile with paragraphs default content, which has a fixed revision_id and ERR references to it, which then fails.
What's happening is that the revision ID is set/changed on a new entity which does not yet have a revision_id and then the revision id is the same as the loaded (= empty) and newRevision is set to FALSE. And then this has some impact later, possibly due to other calls to setNewRevision(TRUE) that then again unset the revision_id because it thinks that previously, it did not save as a new revision, despite actually being a new entity.
This seems to fix it for me: https://gist.githubusercontent.com/Berdir/9468aff9027edbcdc7bfb66975f127...
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | revision-id-unset-2934517-2.patch | 1.78 KB | Berdir |
| #2 | revision-id-unset-2934517-2-test-only.patch | 1.03 KB | Berdir |
Comments
Comment #2
BerdirOh, this is fun. This is even more complex to reproduce, because my initial understanding was wrong. When setting a revision ID, getRevisionID() does already return that. So what we have to do first explicitly set the revision ID to NULL and then set an ID.
While that may seem weird, it is actually exactly what happens in \Drupal\serialization\Normalizer\FieldableEntityNormalizerTrait::denormalizeFieldData(), first it explicitly sets no value and then it sets the actual value.
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedVery nice find :)
Comment #5
Wim LeersNice find indeed! I'm pretty sure I ran into this with Quick Edit, and perhaps also REST/Serialization/HAL.
Comment #7
larowlanCommitted as 895db7d and pushed to 8.5.x.
As per https://www.drupal.org/core/release-cycle-overview#current-development-c... 8.4 is in 'critical fixes only', so this can't be backported to 8.4
Comment #8
BerdirEven though the next 8.4.x release is critical only and I set this to major since the only direct impact I know only happens with an uncommitted patch for entity_reference_revisions it is a regression between 8.4.3 and 8.4.4 so can we consider this for 8.4.5?
Finishing and committing #2848878: Embed Paragraph Content in Normalized Parent Entities (REST) is not possible now until this bug is fixed in a stable version of core.
Comment #9
Wim Leers@larowlan: doesn't "criticals only for current minor" start when the beta of the next minor is released?
Comment #10
Berdir@Wim: Not according to https://www.drupal.org/project/drupal/issues/2932574#comment-12406510
Comment #11
mtodor CreditAttribution: mtodor at Thunder commentedWe have encountered a problem with the update of Thunder to Drupal Core 8.4.4. Since we are using our demo content for our tests (demo content is imported with
default_contentmodule). And this patch solves a problem. Thanks, @Berdir.It would be nice if this patch could be applied for 8.4.5 since it is a regression introduced with 8.4.4.
Comment #12
alexpottAs a patch (#2928778: Exception when trying to save a new revision after manually setting the original revision ID) applied to 8.4.x and released in 8.4.4 has caused this regression - I think this should be committed to 8.4.x too. It is a clear regression. I think the original priority of this bug was incorrect since Thunder broke when upgrading from 8.4.3 to 8.4.4 with no work around (other than applying this patch). Note that Thunder does not use the ERR patch. Therefore I'm changing the priority to critical.
Comment #13
plachI think this fits the definition of critical, since not creating a new revision could qualify as data loss. Regardless of that I agree this should be backported, since it completes the original "fix". The latter was targeting 8.5.x and I should have warned committers that backporting it to 8.4.x was potentially risky. My bad, sorry.
Comment #14
catchFine with critical (and backport) here too.
Comment #16
larowlanDiscussed with @xjm who (in addition to @catch and @plach above) was happy with the backport here.
Cherry-picked as 84785ee and pushed to 8.4.x.