Add a revisionable key to field definitions

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

Looks good.

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

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.

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)

+++ 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)));
       }

Nitpick:
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 ?

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

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.

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

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

Committed and pushed to 8.x. Thanks!

@fago:

[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

Working on this

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