Problem/Motivation

Follow-up to #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table and #2808335: Changed time not updated when only non-translatable fields are changed on a translated entity.

We had to hard-code revision metadata fields in the original bug fix, but now they're in the entity annotation and there's a generic method to retrieve them, can use that instead.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 interdiff.txt1.07 KBamateescu
#11 2855315-11.patch2.31 KBamateescu
#9 interdiff-2-9.txt536 byteshchonov
#9 2855315-9.patch1.24 KBhchonov
#2 2855315.patch1.21 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2855315.patch1.21 KB

Cross-posted with @catch in #2248983-257: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table, here's the interdiff from that comment :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1273,20 +1273,14 @@ public static function bundleFieldDefinitions(EntityTypeInterface $entity_type,
+    ] + array_values($entity_type->getRevisionMetadataKeys());

Not really necessary :).

But this should be fine. I also think we've covered all the other places in the other patch so this should be the only left change, right?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

I wanted to keep the keys of that array clean, just in case someone thinks they can be used for something. And yes, I think we're all good here :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Thank you!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2855315.patch, failed testing.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

It looks like we've got a bug in the revision metadata issue :(

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Assigned: Unassigned » hchonov

I am currently looking into the problem.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Assigned: hchonov » Unassigned
Status: Needs work » Needs review
FileSize
2855315-9.patch1.24 KB
interdiff-2-9.txt536 bytes

;)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

Just to mention - it wasn't a bug in the revision metadata issue, but in the initial patch, as the way we were adding the revision metadata fields wasn't correct and we skipped always the first two, so instead of + we have to use array_merge and then everything is working as excepted.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2855315-11.patch2.31 KB
interdiff.txt1.07 KB

#10 is right, sorry about that :)

I propose to also fix some minor follow-up stuff in this issue.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Let's fix it then.

  • catch committed 57b145b on 8.4.x 
    Issue #2855315 by amateescu, hchonov: Remove hard-coding of revision...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.