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: Too many 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.

Files: 
CommentFileSizeAuthor
#83 et-node-ml2-2057401-83.patch73.4 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,802 pass(es).
[ View ]
#78 et-node-ml2-2057401-78.interdiff.txt834 bytesplach
#78 et-node-ml2-2057401-78.patch73.4 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,729 pass(es).
[ View ]
#74 et-node-ml2-2057401-74.interdiff.txt1.5 KBplach
#74 et-node-ml2-2057401-74.patch73.46 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,934 pass(es).
[ View ]
#70 et-node-ml2-2057401-70.interdiff.txt5.2 KBplach
#70 et-node-ml2-2057401-70.patch72.25 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#60 et-node-ml2-2057401-60.interdiff.txt885 bytesplach
#60 et-node-ml2-2057401-60.patch68.5 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,733 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#58 et-node-ml2-2057401-58.patch68.39 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,623 pass(es), 19 fail(s), and 9,079 exception(s).
[ View ]
#57 et-node-ml2-2057401-57.patch68.28 KBplach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#57 et-node-ml2-2057401-57.interdiff.txt413 bytesplach
#57 et-node-ml2-2057401-57.nocast.patch68.31 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,569 pass(es), 24 fail(s), and 9,047 exception(s).
[ View ]
#55 et-node-ml2-2057401-55.patch68.28 KBplach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#55 et-node-ml2-2057401-55.interdiff.txt496 bytesplach
#55 et-node-ml2-2057401-55.nocast.patch68.21 KBplach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#54 et-node-ml2-2057401-54.patch68.26 KBplach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#54 et-node-ml2-2057401-54.interdiff.txt4.39 KBplach
#54 et-node-ml2-2057401-54.nocast.patch68.19 KBplach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#48 et-node-ml2-2057401-48.patch68.08 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,071 pass(es).
[ View ]
#47 et-node-ml2-2057401-47.patch69 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,586 pass(es).
[ View ]
#44 et-node-ml2-2057401-42-44-interdiff.txt1.53 KBkfritsche
#44 et-node-ml2-2057401-44.patch69.5 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 58,494 pass(es).
[ View ]
#42 et-node-ml2-2057401-42.patch69.65 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] 57,574 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#38 et-node-ml2-2057401-38.patch68.7 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,320 pass(es).
[ View ]
#37 et-node-ml2-2057401-37.interdiff.txt3.34 KBplach
#37 et-node-ml2-2057401-37.patch68.7 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,175 pass(es).
[ View ]
#34 et-node-ml2-2057401-34.patch69.69 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]
#30 et-node-ml2-2057401-30.interdiff.do_not_test.patch8.11 KBplach
#30 et-node-ml2-2057401-30.patch69.69 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,360 pass(es).
[ View ]
#28 et-node-ml2-2057401-28.interdiff.do_not_test.patch6.13 KBplach
#28 et-node-ml2-2057401-28.patch62.68 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,845 pass(es), 9 fail(s), and 3 exception(s).
[ View ]
#26 et-node-ml2-2057401-26.patch57.41 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,897 pass(es), 16 fail(s), and 19 exception(s).
[ View ]
#24 et-node-ml2-2057401-24.interdiff.do_not_test.patch4.26 KBplach
#24 et-node-ml2-2057401-24.patch57.42 KBplach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-node-ml2-2057401-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 et-node-ml2-2057401-22.interdiff.do_not_test.patch8.5 KBplach
#22 et-node-ml2-2057401-22.patch56.98 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,913 pass(es), 12 fail(s), and 18 exception(s).
[ View ]
#20 et-node-ml2-2057401-20.interdiff.do_not_test.patch2.64 KBplach
#20 et-node-ml2-2057401-20.patch50.51 KBplach
FAILED: [[SimpleTest]]: [MySQL] 58,367 pass(es), 26 fail(s), and 20 exception(s).
[ View ]
#18 et-node-ml2-2057401-17.patch48.22 KBplach
FAILED: [[SimpleTest]]: [MySQL] 57,474 pass(es), 47 fail(s), and 27 exception(s).
[ View ]
#13 et-node-ml2-2057401-13.patch49.24 KBplach
FAILED: [[SimpleTest]]: [MySQL] 57,503 pass(es), 52 fail(s), and 23 exception(s).
[ View ]
#11 et-node-ml2-2057401-11.patch36.23 KBplach
FAILED: [[SimpleTest]]: [MySQL] 57,668 pass(es), 55 fail(s), and 16 exception(s).
[ View ]
#9 et-node-ml2-2057401-9.patch28.71 KBplach
FAILED: [[SimpleTest]]: [MySQL] 57,319 pass(es), 57 fail(s), and 27 exception(s).
[ View ]
#7 et-node-ml2-2057401-7.patch24.59 KBplach
FAILED: [[SimpleTest]]: [MySQL] 56,369 pass(es), 385 fail(s), and 112 exception(s).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

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.

Yep, revision_data_table sounds sensible.

Issue summary:View changes

Updated issue summary.

Note that I will not work on this issue.

Issue tags:+Testbot environments

Tagging.

Status:Active» Needs review
StatusFileSize
new24.59 KB
FAILED: [[SimpleTest]]: [MySQL] 56,369 pass(es), 385 fail(s), and 112 exception(s).
[ View ]

Here is a first draft.

Status:Needs review» Needs work

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

Assigned:Unassigned» plach
Status:Needs work» Needs review
StatusFileSize
new28.71 KB
FAILED: [[SimpleTest]]: [MySQL] 57,319 pass(es), 57 fail(s), and 27 exception(s).
[ View ]

This one should be better.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new36.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,668 pass(es), 55 fail(s), and 16 exception(s).
[ View ]

More test fixes

Status:Needs review» Needs work

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

StatusFileSize
new49.24 KB
FAILED: [[SimpleTest]]: [MySQL] 57,503 pass(es), 52 fail(s), and 23 exception(s).
[ View ]

And more

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Issue tags:+beta blocker

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

Status:Needs work» Needs review
StatusFileSize
new48.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,474 pass(es), 47 fail(s), and 27 exception(s).
[ View ]

And now even with patch!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new50.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,367 pass(es), 26 fail(s), and 20 exception(s).
[ View ]
new2.64 KB

Some more test fixes.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new56.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,913 pass(es), 12 fail(s), and 18 exception(s).
[ View ]
new8.5 KB

moar!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new57.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-node-ml2-2057401-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.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.

Status:Needs work» Needs review
StatusFileSize
new57.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,897 pass(es), 16 fail(s), and 19 exception(s).
[ View ]

Rerolled

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new62.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,845 pass(es), 9 fail(s), and 3 exception(s).
[ View ]
new6.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.

Status:Needs work» Needs review
StatusFileSize
new69.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,360 pass(es).
[ View ]
new8.11 KB

And this should hopefully fix the upgrade path.

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

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?

StatusFileSize
new69.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]

Plain reroll

Issue summary:View changes

Updated issue summary

+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'

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

StatusFileSize
new68.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,175 pass(es).
[ View ]
new3.34 KB

Thanks :)

This should address #35-#36.

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

StatusFileSize
new68.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,320 pass(es).
[ View ]

Rerolled

Issue tags:+Needs profiling, +VDC

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

Issue tags:+Entity Field API

Because we don't have enough tags yet.

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.

Status:Needs work» Needs review
StatusFileSize
new69.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,574 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

ReeeeeeeeeeEEEEROOOOOOOOLLLLLL!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new69.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,494 pass(es).
[ View ]
new1.53 KB

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

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.

+++ 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?

StatusFileSize
new69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,586 pass(es).
[ View ]

Rerolled and fixed #46.

StatusFileSize
new68.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,071 pass(es).
[ View ]

Rerolled again

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.

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.

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.

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! :)

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.

Status:Needs work» Needs review
StatusFileSize
new68.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new4.39 KB
new68.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

StatusFileSize
new68.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new496 bytes
new68.28 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Actually executing queries usually helps...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new68.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,569 pass(es), 24 fail(s), and 9,047 exception(s).
[ View ]
new413 bytes
new68.28 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Another try

StatusFileSize
new68.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,623 pass(es), 19 fail(s), and 9,079 exception(s).
[ View ]

Sigh

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new68.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,733 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
new885 bytes

Let's try this

Issue tags:-Needs reroll

Removing tags

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:

<?php
$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

Title:Make the node entity database schema sensibleChange 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.

Title:Change notice: Make the node entity database schema sensibleHEAD 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..

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?

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

This was reverted since it broke PathLanguageTest.

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

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

Status:Needs work» Needs review
StatusFileSize
new72.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.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.

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?

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

Status:Needs work» Needs review
StatusFileSize
new73.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,934 pass(es).
[ View ]
new1.5 KB

This should fix the last failure.

Nice work.

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

  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.

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?

StatusFileSize
new73.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,729 pass(es).
[ View ]
new834 bytes

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

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 :)

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.

StatusFileSize
new73.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,802 pass(es).
[ View ]

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

Let's try a reroll...

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.

Issue summary:View changes

Updated issue summary.

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

Committed/pushed to 8.x, thanks!

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)

Priority:Critical» Major

Status:Active» Needs review

I updated the change notices cited in #86.

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.

Title:Change notice: Make the node entity database schema sensibleMake 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.

Tagging.

Issue tags:+VDC, +Entity Field API

Whum?

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.

I already updated it with all the relevant information :)

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

Yay, superb, thanks!

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

Issue summary:View changes

added plach's suggested commit message.