The entity system has the baked in assumption that an entity type is revisionable when a revision id is specified. This should be explicit and checking for being revisionable should be simple, so let's introduce EntityType::isRevisionable() which checks for the revision id key.

CommentFileSizeAuthor
#19 2224549-16-entity-type-revisionable.patch5.09 KBfago
#18 2224549-18-entity-type-revisionable--content-entity-type.patch7.96 KBtstoeckler
#18 2224549-16-18-interdiff--content-entity-type.txt4.67 KBtstoeckler
#16 2224549-16-entity-type-revisionable--content-entity-type.patch5.35 KBtstoeckler
#16 2224549-move-to-content-entity-type-interface-interdiff.txt2.37 KBtstoeckler
#16 2224549-16-entity-type-revisionable.patch5.09 KBtstoeckler
#16 2224549-9-16-interdiff.txt1.32 KBtstoeckler
#9 drupal_2224549_9.patch4.27 KBXano
#1 d8_entity_revisionable.patch4.29 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Active » Needs review
FileSize
d8_entity_revisionable.patch4.29 KB
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Needs work

I'd say we should mark the relevant entity types as revisionable :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

That is:

  • custom_block
  • node
  • entity_test_rev
  • entity_test_mulrev
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Parent issue: » #2144263: Decouple entity field storage from configurable fields
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs work » Needs review

As described, the patch checks the revision id - so no need to separately mark them revisionable .

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

I still think we should be using an explicit entity key instead of relying on the revision key, to make revisionability more easily alterable, anyway let's discuss this in a follow-up...

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

ok, or we can try to discuss at the ddd to speed up things :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

d8_entity_revisionable.patch no longer applies.

error: core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php: No such file or directory
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_2224549_9.patch4.27 KB

Re-roll.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Reviewed & tested by the community

Re-roll is good, back to RTBC.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Issue tags: -Needs reroll
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Related issues: +#2226437: Create ContentEntityTypeInterface

Yeah, I'm fine with this for now. @plach wants to make the entity key information non-mandatory and provide sensible defaults for them, in which case this has to become a separate property, but since this will centralize the check it should not be a problem to change the implementation at that point.

Also once we have it we can move isRevisionable() along with isTranslatable() onto ContentEntityTypeInterface

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: drupal_2224549_9.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review

9: drupal_2224549_9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: drupal_2224549_9.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2224549-9-16-interdiff.txt1.32 KB
2224549-16-entity-type-revisionable.patch5.09 KB
2224549-move-to-content-entity-type-interface-interdiff.txt2.37 KB
2224549-16-entity-type-revisionable--content-entity-type.patch5.35 KB

- Fix unit test
- Switch from (bool) getKey() to hasKey() in isRevisionable()

Also providing a separate patch and interface for moving this method to ContentEntityTypeInterface. I personally think that's the better patch. It's certainly more correct and we will want to do that at some point, but maybe people will want to all appropriate methods at once. I.e. isTranslatable() should be on ContentEntityTypeInterface as well.

Status: Needs review » Needs work

The last submitted patch, 16: 2224549-16-entity-type-revisionable--content-entity-type.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs work » Needs review
FileSize
2224549-16-18-interdiff--content-entity-type.txt4.67 KB
2224549-18-entity-type-revisionable--content-entity-type.patch7.96 KB

This should fix the test failures for the second patch in #16.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
FileSize
2224549-16-entity-type-revisionable.patch5.09 KB

I don't think we can move this to ContentEntityTypeInterface as RevisionableInterface is a separate interface, which entity types may implement independently from ContentEntityTypeInterface. Besides that, it would be weird if you would have to check for ContentEntityTypeInterface before being able to call isRevisionable().

Therefore, I think the first patch from #16 is the one to go + reroll looks good (again). I'm re-uploading the patch to avoid confusion, no changes.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Reviewed & tested by the community
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

+1 on #19

  • Commit 938ac1f on 8.x by catch: 
    Issue #2224549 by tstoeckler, fago, Xano: Simplify checking whether an...
catch’s picture

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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