Unrevisioned base fields are ignored during entity load

I'd say this is the correct solution. Maybe an optimization could be checking whether we do have non-revisionable fields, otherwise we would skip the join on the data table. Anyway, entity cache would avoid the performance penalty of the additional join most of the time.

Let's see what Berdir thinks about #6.

Spent half a day chasing this one, what a DX disaster.

Updated. Needs tests.

Trying a new approach.

Instead of always using dataTable. We will always use revisionDataTable, and include dataTable if any non-revisionable fields exist.

Minor change to join condition.

Adding tests.

Fix other tests adding non-revisioned field to schemas and renaming due to pre-existing name.

Thanks, the patch is looking good to me! Just a few minor remarks:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -667,29 +667,36 @@ protected function attachPropertyData(array &$entities) {
   +      $translations = array();
   ...
            $revision_ids = array();
   

   This line is misplaced also in HEAD, but I think the proper place would be right before the foreach below.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -667,29 +667,36 @@ protected function attachPropertyData(array &$entities) {
   +        // Are there any fields unique to dataTable, that are not IDs or revisionable?
   

   Can we expand this comment a bit? Also, the comment is not wrapping at column 80.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -667,29 +667,36 @@ protected function attachPropertyData(array &$entities) {
   +        $data_fields_static = array_diff($table_mapping->getFieldNames($this->dataTable), $table_mapping->getFieldNames($this->revisionDataTable), $table_mapping->getFieldNames($this->baseTable));
   +        if ($data_fields_static) {
   +          $data_fields = array_merge($data_fields, $data_fields_static);
   +
   +          $query->leftJoin($this->dataTable, 'static', "(data.$this->idKey = static.$this->idKey)");
   +          $query->fields('static', $data_fields_static);
   +        }
   +
   

   This 'static' terminology is not very clear to me. Can we use something more self-documenting, like $revision_data_fields and $data_fields? Also, there's a surplus blank line.

4. +++ b/core/modules/system/src/Tests/Entity/EntityRevisionsTest.php
   @@ -93,6 +96,10 @@ protected function assertRevisions($entity_type) {
   +      // Check 'data_table' values are loaded.
   

   I'd replace "data_table" with "non-revisionable".

5. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
   @@ -120,6 +120,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['nonrevisioned'] = BaseFieldDefinition::create('integer')
   

   What about adding an underscore to improve readability? Also, the field is non revisionable, even a revisionable field could be "non revisioned" :)
   Thus I'd suggest non_revisionable.

6. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
   @@ -120,6 +120,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setLabel(t('Foo'))
   

   Can we use a more meaningful label here?

Thank you, I've addressed your feedback.

Changed terminology of "non-revisioned" to "persistent".

Aliases are now consistent on which tables are used.

As a separate issue, we should consider changing "created" properties of core entities to non-revisioned fields.

Apparently file upload order matters for patches.

Thanks, even more minor nits:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -665,24 +665,24 @@
   +        // Find revisioned fields that are non-entity ID's
   

   This should say "entity keys" instead of "non-entity ID's". Also, missing trailing dot.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -665,24 +665,24 @@
   +        // Find fields that are non-revisioned and non ID's. The value of
   +        // data fields are persistent regardless of entity revision.
   

   As above, please use "entity keys". Also, "data" should go in the previous line, it fits within 80 chars.

3. +++ b/core/modules/serialization/src/Tests/EntitySerializationTest.php
   @@ -59,7 +59,7 @@
   +      'persistent' => 33,
   

   Mmh, I can't really connect persistent to the revisions concept at first sight. What's wrong with non_revisionable?

Load and save such an entity and kaboom. Definitely critical:

Critical

Bugs

Critical bugs include those that:

• Cause loss of data.

Hooray, a promotion!

What's wrong with non_revisionable?

"Non-revisionable fields are persistent through all revisions."

I feel calling non-something doesn't feel right. I really was hoping for an example from elsewhere.

For those looking on, looking for term for an entity property that is non-revisionable, and not an entity key.

Short list: base data, data, constant, persistent, persistent data, static.

Alternatively, give a name that describes the purpose of the field, rather than the function.

Scrap the last comment then. I have replaced the vague "persistent" integer field, with a non-revisionable "created" field type.

I have amended the comments

created makes sense to me. Maybe include that the field is not revisioned in the field definition description to make that clear?

Resolving conflicts due to #2358079: Require a specific placeholder format in db_query() in order to trigger argument expansion, and require explicit 'IN' parameter for conditions.

Fixed non translatable field...

Looks ready to go to me. @Berdir?

Looks good to me as well :)

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed cf40fa1 and pushed to 8.0.x. Thanks!

Go easy on the celebrating

lol