We are still missing a key that tells us whether a given field is actually revisionable. This metadata is good to have anyway, but in particular it helps all the storage internal code to know how to query or load the field.

Apart from that it tells anyone using the entity revision API which fields it will actually make use of, without urging people to go through various internal.

#14 d8_field_revisionable.patch9.61 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,735 pass(es). View


fago’s picture

fago’s picture

tim.plunkett’s picture

Priority: Major » Normal

Demoting for now, it doesn't seem like this has really come up since filing this.

fago’s picture

Assigned: Unassigned » fago
Priority: Normal » Critical

This is needed for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions - this qualifies it even as beta-blocker now. I'll roll a patch.

fago’s picture

Assigned: fago » Unassigned
Status: Active » Needs review
3.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,667 pass(es), 1 fail(s), and 0 exception(s). View

Here it is.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

I guess we should mark all revisionable fields out there as such now :)

plach’s picture

Status: Reviewed & tested by the community » Needs work
tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -172,6 +172,28 @@ public function setTranslatable($translatable) {
    +    // Default to TRUE if it has not been set yet.
    +    return !isset($this->definition['revisionable']) || $this->definition['revisionable'];

    Why is this TRUE by default? Is there any reason? I checked and isTranslatable() simply does !empty($this->definition['translatable'])

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -172,6 +172,28 @@ public function setTranslatable($translatable) {
    +   * @return static

    This should be return $this now.

The last submitted patch, 5: d8_field_revisionable.patch, failed testing.

fago’s picture

9.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,609 pass(es), 15 fail(s), and 4 exception(s). View
7.61 KB

Here is a re-roll according our discussions. Not done yet (has a fail), and needs to incorporate definitions for all other content entities.

giorgio79’s picture

This key perhaps could even be used to avoid creating revision tables with dupe data in D8. This was quite an issue in d7 (https://drupal.org/project/field_sql_norevisions)

xjm’s picture

Issue tags: +beta blocker
yched’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -345,15 +345,22 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
+      // None of the entity keys except labels may be revisionable.
+      if ($key != 'label' && $base_field_definitions[$field_name]->isRevisionable()) {
+        throw new \LogicException(String::format('The @field field cannot be revisionable as it is used as @key entity key.', array('@field' => $base_field_definitions[$field_name]->getLabel(), '@key' => $key)));
+      }
+      if (isset($untranslatable_keys[$field_name]) && $base_field_definitions[$field_name]->isTranslatable()) {
+        throw new \LogicException(String::format('The @field field cannot be translatable as it is used as @key entity key.', array('@field' => $base_field_definitions[$field_name]->getLabel(), '@key' => $key)));

Since "revisionability" check is made on the hardcoded 'label' entity key, similarly the "translatability" check could be made with an in_array() on an inlined, hardcoded list of keys. No need for the $untranslatable_keys var above, it feels disconnected with the comment that precedes its definition and hinders readability IMO.

Other than that: don't we want to default to TRUE ?

fago’s picture

9.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,735 pass(es). View
2.03 KB

ok, moved to in_array(). It's not as efficient, but its minor and the result is cached anyway.

Attached patch should fix the tests. Unfortunately I had to remove the nice simple little check to ensure all entity keys are there as well, but this would require us to redo most of entitymanager unit test. So removed this for being able to move on here.

Other than that: don't we want to default to TRUE ?

That was my initial suggestion (see above), but after some discussion with berdir, tstoeckler and xano we agreed to default to FALSE. You either have to disable it for entity-key-fields manually, or you have to enable it for most others manually if it should be explicit. So it does not matter much but with the default of FALSE it's at least consistent with translatability.

fago’s picture

Status: Needs work » Needs review
plach’s picture

The last submitted patch, 10: d8_field_revisionable.patch, failed testing.

plach’s picture

Looks good to me, would it make sense to add a (qualified) @todo about restoring the exception and refactoring EntityManagerTests as needed?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Great! Have nothing to complain about. I agree with #14. I would find it unintuitive to have to ->setRevisionable(FALSE) for stuff like nid, etc.

@yched, you can obviously downgrade if you object, but marking this RTBC.

tstoeckler’s picture

Oops, crossposted with #18. Leaving at RTBC?!

plach’s picture

Yep, let's leave the decision to a committer.

  • Commit 4a1ff2d on 8.x by webchick:
    Issue #2144631 by fago: Add a revisionable key to field definitions.
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

plach’s picture


[17:34]     plach       webchick: fago just told me he still needed to mark as revisionable the actual definitions
[17:34]	webchick	Dohhh
[17:35]	plach	webchick: do we want to rollback or just post a follow-up? Nothing is broken atm
[17:35]	webchick	plach: shiould I roll it back, or follow-up?
[17:35]	webchick	hahaha
[17:35]	webchick	plach: follow-up is fine w/ me
plach’s picture

Status: Fixed » Active
plach’s picture

Assigned: Unassigned » plach

Working on this

plach’s picture

Assigned: plach » Unassigned
Status: Active » Fixed

@fago is going to post a novice critical beta-blocker follow-up :)

fago’s picture

Created #2225699: Set all revisionable fields to revisionable as follow-up.

FYI: Related issue for isRevisionable() on entity types: #2224549: Simplify checking whether an entity type is revisionable

Status: Fixed » Closed (fixed)

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