Split from #1998366: [meta] SQLite is broken

according to #84, suggested commit message:

Issue #2057401 by plach, socketwench, kfritsche, Damien Tournoud: Fixed Make the node entity database schema sensible.

Summary

Motto: we are basically taking node data and vomiting it on the hard drive without any consideration as to if the data we are writing is sensible or simply dreamed up by magic pixies. — chx

The database schema introduced by #1498674: Refactor node properties to multilingual has several short-comings, the primary of them being that the concept of "revision" is not materialized as a table in the schema anymore. As a consequence, we don't have no good way to generate the revision identifier (vid). The current schema works on MySQL (because it is doing some really, really ahem interesting tricks to generate the serial if the primary key is not the serial column by itself), but it would not work on virtually any other database engine.

The following modifications are being proposed:

Re-introduce a {node_revision} table

It will store fields that are specific to the revision itself and will allocate the vid.

The following structure:

  • Primary key: (vid)
  • Fields:
    • vid
    • nid
    • langcode
    • log
    • revision_timestamp
    • author_uid

Remove {node}.langcode (as it is moved to {node_revision}

Update the structure of {node_field_data} table

  • Change {node_field_data}.nid from serial to int,
  • Change the primary key from nid, vid, langcode to nid, langcode,
  • Drop the {node_field_data}.nid index that is redundant.

Update the structure of {node_field_revision} table

  • Change {node_field_revision}.vid from serial to int,
  • Change the primary key from nid, vid, langcode to vid, langcode,
  • Drop the {node_field_revision}.nid and {node_field_revision}.vid indexes.

Remaining tasks

  • Agree on the plan
  • Write a patch
  • Benchmarking/profiling

Blocks:

#2015277: Reduce the number of indexes on the node_field_* tables
#2032977: Add views support for the missing columns of node_field_revision

More related issues:

#1498674: Refactor node properties to multilingual
See from #62 to #104 of #1998366: [meta] SQLite is broken for a complete background.

CommentFileSizeAuthor
#83 et-node-ml2-2057401-83.patch73.4 KBplach
#78 et-node-ml2-2057401-78.interdiff.txt834 bytesplach
#78 et-node-ml2-2057401-78.patch73.4 KBplach
#74 et-node-ml2-2057401-74.interdiff.txt1.5 KBplach
#74 et-node-ml2-2057401-74.patch73.46 KBplach
#70 et-node-ml2-2057401-70.interdiff.txt5.2 KBplach
#70 et-node-ml2-2057401-70.patch72.25 KBplach
#60 et-node-ml2-2057401-60.interdiff.txt885 bytesplach
#60 et-node-ml2-2057401-60.patch68.5 KBplach
#58 et-node-ml2-2057401-58.patch68.39 KBplach
#57 et-node-ml2-2057401-57.patch68.28 KBplach
#57 et-node-ml2-2057401-57.interdiff.txt413 bytesplach
#57 et-node-ml2-2057401-57.nocast.patch68.31 KBplach
#55 et-node-ml2-2057401-55.patch68.28 KBplach
#55 et-node-ml2-2057401-55.interdiff.txt496 bytesplach
#55 et-node-ml2-2057401-55.nocast.patch68.21 KBplach
#54 et-node-ml2-2057401-54.patch68.26 KBplach
#54 et-node-ml2-2057401-54.interdiff.txt4.39 KBplach
#54 et-node-ml2-2057401-54.nocast.patch68.19 KBplach
#48 et-node-ml2-2057401-48.patch68.08 KBplach
#47 et-node-ml2-2057401-47.patch69 KBplach
#44 et-node-ml2-2057401-42-44-interdiff.txt1.53 KBkfritsche
#44 et-node-ml2-2057401-44.patch69.5 KBkfritsche
#42 et-node-ml2-2057401-42.patch69.65 KBsocketwench
#38 et-node-ml2-2057401-38.patch68.7 KBplach
#37 et-node-ml2-2057401-37.interdiff.txt3.34 KBplach
#37 et-node-ml2-2057401-37.patch68.7 KBplach
#34 et-node-ml2-2057401-34.patch69.69 KBplach
#30 et-node-ml2-2057401-30.interdiff.do_not_test.patch8.11 KBplach
#30 et-node-ml2-2057401-30.patch69.69 KBplach
#28 et-node-ml2-2057401-28.interdiff.do_not_test.patch6.13 KBplach
#28 et-node-ml2-2057401-28.patch62.68 KBplach
#26 et-node-ml2-2057401-26.patch57.41 KBplach
#24 et-node-ml2-2057401-24.interdiff.do_not_test.patch4.26 KBplach
#24 et-node-ml2-2057401-24.patch57.42 KBplach
#22 et-node-ml2-2057401-22.interdiff.do_not_test.patch8.5 KBplach
#22 et-node-ml2-2057401-22.patch56.98 KBplach
#20 et-node-ml2-2057401-20.interdiff.do_not_test.patch2.64 KBplach
#20 et-node-ml2-2057401-20.patch50.51 KBplach
#18 et-node-ml2-2057401-17.patch48.22 KBplach
#13 et-node-ml2-2057401-13.patch49.24 KBplach
#11 et-node-ml2-2057401-11.patch36.23 KBplach
#9 et-node-ml2-2057401-9.patch28.71 KBplach
#7 et-node-ml2-2057401-7.patch24.59 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

We will need to make this change on an entity storage level, of course. What shall be the name of the table be? As a reminder:

 *   base_table = "node",
 *   data_table = "node_field_data",
 *   revision_table = "node_field_revision",

revision_data_table looks logical to me.

plach’s picture

Yep, revision_data_table sounds sensible.

plach’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Note that I will not work on this issue.

plach’s picture

catch’s picture

Issue tags: +Testbot environments

Tagging.

plach’s picture

Status: Active » Needs review
FileSize
24.59 KB

Here is a first draft.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-7.patch, failed testing.

plach’s picture

Assigned: Unassigned » plach
Status: Needs work » Needs review
FileSize
28.71 KB

This one should be better.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-9.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
36.23 KB

More test fixes

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-11.patch, failed testing.

plach’s picture

FileSize
49.24 KB

And more

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-13.patch, failed testing.

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

plach’s picture

plach’s picture

Status: Needs work » Needs review
FileSize
48.22 KB

And now even with patch!

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-17.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
50.51 KB
2.64 KB

Some more test fixes.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-20.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
56.98 KB
8.5 KB

moar!

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-22.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
57.42 KB
4.26 KB

This contains a better fix for some storage controller-related failures. It might intoduce new failures though.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-24.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
57.41 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-26.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
62.68 KB
6.13 KB

Only upgrade path stuff should be failing now.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-28.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
69.69 KB
8.11 KB

And this should hopefully fix the upgrade path.

The last submitted patch, et-node-ml2-2057401-30.patch, failed testing.

plach’s picture

dcrocks’s picture

Tested patch on a clean install of D8-x.dev. Installed without error. Created a simple article, saved it, then revised it. No errors. Looks good. Can someone review and rtbc?

plach’s picture

FileSize
69.69 KB

Plain reroll

plach’s picture

Issue summary: View changes

Updated issue summary

andypost’s picture

+1 here. Overall patch is ready, just small nitpicks.
@plach upgrade path tests passed but I found no creation of new table!?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -62,6 +69,10 @@ public function __construct($entity_type, array $entity_info, Connection $databa
+      // TODO
+      if ($this->revisionTable) {
+        $this->revisionDataTable = $this->entityInfo['revision_data_table'];

// Init RevisionData if defined.
$this->revisionDataTable =
!empty($this->entityInfo['revision_data_table']) ? $this->entityInfo['revision_data_table'] : FALSE;

suppose this property is optional

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -493,19 +447,33 @@ protected function savePropertyData(EntityInterface $entity) {
+   * @param string $table_key
+   *   (optional) The table to prepare the record for. Defaults to the base
+   *   table.
...
+  protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'base_table') {

this need to point about usage of the key for enity_info array.
And defaults is 'base_table'

andypost’s picture

Also I think that no need to make separate methods that all calls mapToStorageRecord()

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -493,19 +447,33 @@ protected function savePropertyData(EntityInterface $entity) {
+  protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'base_table') {

@@ -519,13 +487,25 @@ protected function mapToStorageRecord(EntityInterface $entity) {
   protected function mapToRevisionStorageRecord(EntityInterface $entity) {
...
+    return $this->mapToStorageRecord($entity, 'revision_table');
...
+  protected function mapToDataStorageRecord(EntityInterface $entity, $table_key = 'data_table') {
+    $record = $this->mapToStorageRecord($entity, $table_key);
+    $record->langcode = $entity->language()->id;
+    $record->default_langcode = intval($record->langcode == $entity->getUntranslated()->language()->id);
     return $record;

@@ -534,30 +514,12 @@ protected function mapToRevisionStorageRecord(EntityInterface $entity) {
+  protected function mapToRevisionDataStorageRecord(EntityInterface $entity) {
+    return $this->mapToDataStorageRecord($entity, 'revision_data_table');

Any reason to all this methods?
Suppose savePropertyData() should care about this "base table" hacks

plach’s picture

Thanks :)

This should address #35-#36.

I think we should do some profiling/benchmarking once we are done with code reviews.

plach’s picture

Rerolled

jibran’s picture

Issue tags: +needs profiling, +VDC

Adding VDC tag for views related changes and needs profiling as per #37 . Patch looks good to me.

Berdir’s picture

Issue tags: +Entity Field API

Because we don't have enough tags yet.

jthorson’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Not enough tags? Here's another!

Doesn't apply, thanks to a bunch of updated tests.

socketwench’s picture

Status: Needs work » Needs review
FileSize
69.65 KB

ReeeeeeeeeeEEEEROOOOOOOOLLLLLL!

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-42.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
69.5 KB
1.53 KB

Next reroll.
Seems like in the last reroll we had a "merge conflict".
Hopefully this works now.

chx’s picture

Assigned: plach » Damien Tournoud

Moving to Damien for review -- as he is the maintainer of two non-mysql db drivers in core and a third in contrib it would make a lot of sense to get his approval.

Berdir’s picture

+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditorFileUsageTest.php
@@ -31,8 +31,14 @@ public static function getInfo() {
-    $this->installSchema('node', array('node', 'node_access', 'node_field_data', 'node_field_revision'));
-    $this->installSchema('file', array('file_managed', 'file_usage'));
+    $this->installSchema('system', 'url_alias');
+    $this->installSchema('node', 'node');
+    $this->installSchema('node', 'node_revision');
+    $this->installSchema('node', 'node_access');
+    $this->installSchema('node', 'node_field_data');
+    $this->installSchema('node', 'node_field_revision');
+    $this->installSchema('file', 'file_managed');
+    $this->installSchema('file', 'file_usage');

Why move to separate calls here?

plach’s picture

Rerolled and fixed #46.

plach’s picture

FileSize
68.08 KB

Rerolled again

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned

Just so that this doesn't block on me, +1 on the principal, and on the new schema for the node module.

The patch is really hard to review without actually applying it, so I haven't looked in details, but all the changes seem reasonable from a cursory look.

I'll take a closer look this week as time permit, but please don't block this on me. This really need to get in, if there are details to iron out we can iron them out later.

Berdir’s picture

Status: Needs review » Needs work

Ok, went through the patch, didn't review the big picture, but we have Damien's OK now :)

  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -370,15 +332,19 @@ public function save(EntityInterface $entity) {
    +        $entity->enforceIsNew();
             $return = drupal_write_record($this->entityInfo['base_table'], $record);
    -        $entity->{$this->idKey}->value = $record->{$this->idKey};
    +        $entity->{$this->idKey}->value = (string) $record->{$this->idKey};
             if ($this->revisionKey) {
    +          $entity->setNewRevision();
               $record->{$this->revisionKey} = $this->saveRevision($entity);
             }
    -        $entity->{$this->idKey}->value = $record->{$this->idKey};
             if ($this->dataTable) {
               $this->savePropertyData($entity);
             }
    +        if ($this->revisionDataTable) {
    +          $this->savePropertyData($entity, TRUE);
    +        }
    

    Why the string cast here?

    A few inline comments here would really help to understand the flow, e.g. making sure that the entity is still seen as new after adding the id.

    Although this is really a bit weird that we have to do it like this...

  2. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -413,53 +379,34 @@ public function save(EntityInterface $entity) {
       protected function saveRevision(EntityInterface $entity) {
    
    @@ -468,16 +415,24 @@ protected function saveRevision(EntityInterface $entity) {
    +  protected function savePropertyData(EntityInterface $entity, $revision = FALSE) {
    +    $table_key = $revision ? 'revision_data_table' : 'data_table';
    +    $table_name = $revision ? $this->revisionDataTable : $this->dataTable;
    

    Wondering if it might be easier to understand the folow if the table_key and table_name were passed directly to the method instead of the magic (and undocumented) $revision property. Then you can more easily see what it's doing when it's called.

  3. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -488,70 +443,52 @@ protected function savePropertyData(EntityInterface $entity) {
    -  protected function mapToRevisionStorageRecord(EntityInterface $entity) {
    +  protected function mapToStorageRecord(EntityInterface $entity, $table_key = 'base_table') {
    

    And match this one better.

  4. +++ b/core/modules/node/lib/Drupal/node/Tests/Condition/NodeConditionTest.php
    @@ -26,7 +26,7 @@ public static function getInfo() {
       public function setUp() {
         parent::setUp();
    -    $this->installSchema('node', array('node', 'node_field_data', 'node_field_revision'));
    +        $this->installSchema('node', array('node', 'node_field_data', 'node_field_revision', 'node_revision'));
    

    Weird indendation.

  5. +++ b/core/modules/node/node.install
    @@ -999,6 +1041,11 @@ function node_update_8017(&$sandbox) {
    +    // Populate the langcode column with the same value for each revision as we
    +    // have no other data available.
    +    $args = array('langcode' => $row->langcode, 'vid' => $row->vid);
    +    db_query('UPDATE {node_revision} SET langcode = :langcode WHERE vid = :vid', $args);
    

    This is not a db_update() query why.. ? :)

  6. +++ b/core/modules/node/node.install
    @@ -1017,10 +1064,14 @@ function node_update_8018() {
       foreach ($indexes as $index) {
    
    +++ b/core/modules/node/node.views.inc
    @@ -505,6 +474,61 @@ function node_views_data() {
    +  if (Drupal::moduleHandler()->moduleExists('language')) {
    

    Should now use \Drupal. Didn't check if there are others, just noticed this one.

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
    @@ -37,6 +37,7 @@ function setUp() {
           'entity_test_rev',
           'entity_test_rev_revision',
           'entity_test_mulrev',
    +      'entity_test_mulrev_revision',
           'entity_test_mulrev_property_data',
    

    We should really have a helper for this. But that's just going to make this patch bigger.

dcrocks’s picture

I was able to apply the patch in #48 to a copy of D8 cloned from git without error, and then install it on SQLite, also without error. I created and revised some trivial content without a problem. Looking forward to seeing this go in.

nevergone’s picture

I install the latest Drupal 8 with #48 path and SQLite 3.7.9 and works well!
Create user, versioned and unversioned nodes without errors.

Thanks! :)

das-peter’s picture

Just did a visual review and the only addition to #50 I've found is following nitpicky thing. All other things I found were already mentioned by berdir.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -488,70 +443,52 @@ protected function savePropertyData(EntityInterface $entity) {
+   * Maps from an entity object to the storage record of the field date.

Typo: field date. I guess that should be field data.

plach’s picture

Status: Needs work » Needs review
FileSize
68.19 KB
4.39 KB
68.26 KB

Addressed #50 and #53.

@berdir:

Why the string cast here?

I don't remember honestly: it was needed to fix some failing tests. I attached a patch version that reverts that to see which ones.

plach’s picture

Actually executing queries usually helps...

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-55.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
68.31 KB
413 bytes
68.28 KB

Another try

plach’s picture

FileSize
68.39 KB

Sigh

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-58.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
68.5 KB
885 bytes

Let's try this

tim.plunkett’s picture

Issue tags: -Needs reroll

Removing tags

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling, -Needs upgrade path

Ok, I think this is ready.

We have reports that this installs and works fine on sqlite, the OK from Damien, we had some code reviews. I also did some profiling, where I wasn't able to see a measurable difference.

Also did run this before and after:

$start = microtime(TRUE);
for ($i = 0; $i < 100; $i++) {
  node_load(1)->save();
}
$diff = microtime(TRUE) - $start;

Before:
21.323546886444
21.295757055283

After:
21.397483825684
21.107705116272

catch’s picture

Title: Make the node entity database schema sensible » Change notice: Make the node entity database schema sensible
Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: -beta blocker +Needs change record

This looks a lot better. Committed/pushed to 8.x

If we need to make further tweaks (for example #1528028: Add tests for reverting revisions where revision_uid and uid differ) can do those later.

Will need either a new or updated change notice.

Berdir’s picture

Title: Change notice: Make the node entity database schema sensible » HEAD BROKEN: Change notice: Make the node entity database schema sensible
Category: task » bug
Priority: Major » Critical

Ok, looks like this broke HEAD (PathLanguageTest), possibly because it went in in combination with the translation.module removal?

No idea why yet, investigating. Would be great to get a quick fix and not having to roll back this issue..

Berdir’s picture

Ok, this fails with the following exception:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2-en' for key 'PRIMARY': INSERT INTO {node_field_revision} (nid, vid, langcode, default_langcode, title, uid, status, created, changed, promote, sticky) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10), (:db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => 2 [:db_insert_placeholder_2] => en [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => ic4ufkUv [:db_insert_placeholder_5] => 2 [:db_insert_placeholder_6] => 1 [:db_insert_placeholder_7] => 1380906585 [:db_insert_placeholder_8] => 1380906585 [:db_insert_placeholder_9] => 0 [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => 1 [:db_insert_placeholder_12] => 3 [:db_insert_placeholder_13] => fr [:db_insert_placeholder_14] => 0 [:db_insert_placeholder_15] => qBwKfc7j [:db_insert_placeholder_16] => 2 [:db_insert_placeholder_17] => 1 [:db_insert_placeholder_18] => 1380906588 [:db_insert_placeholder_19] => 1380906588 [:db_insert_placeholder_20] => 0 [:db_insert_placeholder_21] => 0 ) in Drupal\Core\Entity\DatabaseStorageControllerNG->save()

Sounds like an actual problem and not just a stale test or so.

The test was changed to use entity translation with the removal of translation.module, is it possible that this is now testing a use case that we previously didn't?

tim.plunkett’s picture

Title: HEAD BROKEN: Change notice: Make the node entity database schema sensible » Make the node entity database schema sensible
Status: Active » Needs work

This was reverted since it broke PathLanguageTest.

plach’s picture

Weird, the latest rerolls were due to the translation module removal and we had a green patch... I will look into this ASAP

plach’s picture

The last submitted patch, et-node-ml2-2057401-60.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
72.25 KB
5.2 KB

I was not able to make tests pass yet, no idea of how the PathLanguageTest could be passing before.

However I think node forms are buggy wrt revision handling in the current head (a new revision is always created) and this revealed some bugs with multilingual revisions.

Let's see if I am moving in the right direction.

Status: Needs review » Needs work

The last submitted patch, et-node-ml2-2057401-70.patch, failed testing.

andypost’s picture

I think node forms are buggy wrt revision handling in the current head (a new revision is always created) and this revealed some bugs with multilingual revisions

Maybe better to file separate issue for that?

plach’s picture

Sure, I was just suggesting that buggy node forms might be the cause behind the new test failures.

plach’s picture

Status: Needs work » Needs review
FileSize
73.46 KB
1.5 KB

This should fix the last failure.

Berdir’s picture

Nice work.

I'm not sure I fully understand all the changes, can you quickly describe why some of those changes are necessary?

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -393,7 +393,10 @@ protected function getTranslatedField($property_name, $langcode) {
    -        $this->fields[$property_name][$langcode] = $this->getTranslatedField($property_name, Language::LANGCODE_DEFAULT);
    +        if (!isset($this->fields[$property_name][Language::LANGCODE_DEFAULT])) {
    +          $this->fields[$property_name][Language::LANGCODE_DEFAULT] = $this->getTranslatedField($property_name, Language::LANGCODE_DEFAULT);
    +        }
    +        $this->fields[$property_name][$langcode] = &$this->fields[$property_name][Language::LANGCODE_DEFAULT];
    

    This is needed because untranslatable fields were getting different field objects per language instead of shared ones.

  2. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -321,6 +321,7 @@ public function save(EntityInterface $entity) {
    +        $entity->setNewRevision(FALSE);
             $entity->postSave($this, TRUE);
             $this->invokeFieldMethod('update', $entity);
             $this->saveFieldItems($entity, TRUE);
    @@ -397,7 +398,6 @@ protected function saveRevision(EntityInterface $entity) {
    
    @@ -397,7 +398,6 @@ protected function saveRevision(EntityInterface $entity) {
               ->condition($this->idKey, $record->{$this->idKey})
               ->execute();
           }
    -      $entity->setNewRevision(FALSE);
    

    This prevented code following the revision record storage to know we just saved a new revision.

  3. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -419,12 +419,11 @@ protected function saveRevision(EntityInterface $entity) {
    -    $revision = TRUE;
         if (!isset($table_key)) {
           $table_key = 'data_table';
    -      $revision = FALSE;
         }
         $table_name = $this->entityInfo[$table_key];
    +    $revision = $table_key != 'data_table';
    

    This was broken when 'data_table' was passed explicitly.

  4. +++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php
    @@ -89,7 +89,7 @@ function testAliasTranslation() {
    +    $this->assertText($english_node->body->value, 'Alias works.');
    

    Node titles cannot be translated yet, so this test was totally meaningless. Moved it to node bodies which can be translated.

  1. +++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php
    @@ -65,20 +67,16 @@ function setUp() {
    +    // Ensure configuration changes are picked up in the host environment.
    +    Field::fieldInfo()->flush();
    +    $field = Field::fieldInfo()->getField('node', 'body');
    +    $this->assertTrue($field->isFieldTranslatable(), 'Node body is translatable.');
       }
    

    The test is programmatically creating a node but, since field translatability change was not picked up, the body value got the wrong language.

  2. +++ b/core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php
    @@ -65,20 +67,16 @@ function setUp() {
    -    // Set 'page' content type to enable translation.
    -    $edit = array(
    -      'language_configuration[language_show]' => TRUE,
    -    );
    -    $this->drupalPostForm('admin/structure/types/manage/page', $edit, t('Save content type'));
    -    $this->assertRaw(t('The content type %type has been updated.', array('%type' => 'Basic page')), 'Basic page content type has been updated.');
    -    variable_set('node_type_language_translation_enabled_page', TRUE);
    -
    

    Removed some unneeded cruft.

Berdir’s picture

Ok, thanks for the explanations, makes sense.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -465,17 +414,31 @@ protected function saveRevision(EntityInterface $entity) {
+  protected function savePropertyData(EntityInterface $entity, $table_key = NULL) {
+    if (!isset($table_key)) {
+      $table_key = 'data_table';
+    }
+    $table_name = $this->entityInfo[$table_key];
+    $revision = $table_key != 'data_table';

Is there a reason we don't just put $table_key = 'data_table' in the function definition now?

plach’s picture

I should definitely stop working on that method after midnight :D

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

That's a problem then, don't you only work after midnight? ;)

Ok, we fixed the bug that the changed test exposed and improved that test, not sure how it even worked before that. Let's try this again :)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -D8 upgrade path, -D8MI, -Needs change record, -sprint, -language-content, -VDC, -Entity Field API, -Approved API change, -Testbot environments

The last submitted patch, et-node-ml2-2057401-78.patch, failed testing.

plach’s picture

plach’s picture

It seems there's no way to get a result :(

Let's try a reroll...

plach’s picture

Ok, #78 is green and is identical to #83. Back to RTBC.

@committers:

Please credit @Damien as this fix is mainly due to him, I just implemented it.

plach’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Title: Make the node entity database schema sensible » Change notice: Make the node entity database schema sensible
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

YesCT’s picture

Category: bug » task
Priority: Major » Critical

critical task according to https://drupal.org/core-gates#documentation (we can change it back to critical bug report after the change notice is done.)

I read the issue summary (nice, btw) and the patch that got committed. I'm confused my some of the namings

  1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
    @@ -465,17 +414,28 @@ protected function saveRevision(EntityInterface $entity) {
    +   * @param string $table_key
    +   *   (optional) The entity key identifying the target table. Defaults to
    +   *   'data_table'.
    
    @@ -486,70 +446,52 @@ protected function savePropertyData(EntityInterface $entity) {
    +   * @param string $table_key
    +   *   (optional) The entity key identifying the target table. Defaults to
    +   *   'base_table'.
    

    data_table, base_table

    are they combined, like
    base_table.data_table to make something like: revision.data_table?

    or node_field_revision?

  2. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -34,7 +34,8 @@
      *   data_table = "node_field_data",
    - *   revision_table = "node_field_revision",
    + *   revision_table = "node_revision",
    + *   revision_data_table = "node_field_revision",
    

    data == fields ?

What kind of change notice do we want for this? Is this a: previous 8.x to now 8.x kind of change notice, 7 to 8, or should we update a previous change notice.

Maybe updating the change records linked from #1498674: Refactor node properties to multilingual:
https://drupal.org/node/1722906 Added multilingual support to the standard entity schema
or
https://drupal.org/node/2004750 Node properties are made multilingual

Are properties now base fields (revision data)? (I haven't been completely keeping up with the property stuff)

amateescu’s picture

catch’s picture

Priority: Critical » Major
plach’s picture

Status: Active » Needs review

I updated the change notices cited in #86.

plach’s picture

are they combined, like
base_table.data_table to make something like: revision.data_table?

I don't understand the question :)

The base table, data_table, revision_table and revision_data_table keys are just the names of the 4 tables implementing the 4 modes we are supporting (basic entity, multilingual entity, revisionable entity, multilingual and revisionable entity). Only revision_data_table has been introduced here.

data == fields ?

More or less: data is an abbreviation for field data. The new table layout somewhow mimics field tables, where we have a data table and a revision table.

Are properties now base fields (revision data)?

Again I am not sure I get what you mean: with the introduction of the Entity Field API, we no longer distinguish between fields and properties. Base fields are defined in the entity class, additional fields may be defined by modules, for instance the Field API module. Now all node base fields (including language) have revision support, except for revision metadata, but this was already implemented in #1498674: Refactor node properties to multilingual.

Berdir’s picture

Title: Change notice: Make the node entity database schema sensible » Make the node entity database schema sensible
Category: task » bug
Priority: Major » Critical
Status: Needs review » Fixed
Issue tags: -Needs change record

Updates look good to me.

Gábor Hojtsy’s picture

Tagging.

Gábor Hojtsy’s picture

Issue tags: +VDC, +Entity Field API

Whum?

Gábor Hojtsy’s picture

Is there something that should be brought back into https://drupal.org/node/1722906 and the base entity schema suggestions? Since that would also be taken as a best practice, we should consider keeping it up to date to current best practices.

plach’s picture

I already updated it with all the relevant information :)

https://drupal.org/node/1722906/revisions/view/2710340/2869763

Gábor Hojtsy’s picture

Yay, superb, thanks!

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

Anonymous’s picture

Issue summary: View changes

added plach's suggested commit message.