Work is ongoing in the D8MI - Entity Translation sandbox.

Problem/Motivation

Currently node non-configurable fields are not translatable, since the node schema only allows for a single record for each node. This is a big blocker for the D8MI initiative since we want to make all entities translatable through the upcoming #1188388: Entity translation UI in core and remove the node translation model from core.

Proposed resolution

Refactor the node schema to match the new standard described in Added multilingual support to the standard entity schema.
Some documentation examples used {node} but were really generic, so we use {example} instead. See @Berdir's feedback in #49.

Implementation details

Adopting the new schema standard required, among the rest, to refactor the DatabaseStorageController to properly take into consideration revision translation. On node load we no longer join on the revision table, instead we attach all the field data by subsequently querying it, since field values may be multilingual and thus involve multiple records.

The (internal) refactoring of the storage controller allows for splitting the node table into two separate {node} and {node_field_data} tables. All the field values have been moved into the latter, thus gaining the ability to have a different value per language. Complying to the standard required to make all the field values revisable: the revision table has been renamed to {node_field_revision} and all the missing columns were added to it. For instance now we can track the node authoring date per revision and have a different value per-language.

All the queries on the {node} table has been updated to act (generally) on the {node_field_data} table instead. We needed also to adapt the various queries to multilingual scenarios: in some cases language-agnostic queries were fine (e.g. count queries). In other cases it was ok to just take the default language into account (e.g. when retrieving a set of nids to load the corresponding nodes subsequently). In many cases a contextual information about the language to filter on would be needed, but such information is not available yet so these queries were just adapted to use the entity language. We will fix them after #1810370: Entity Translation API improvements is done. Updated queries were analyzed through MySQL explain to find out the most efficient alternative. Also microbenchmarks results were used to confirm the choice.

Lastly all views definitions were updated to use the {node_field_data} table where necessary.

An (apparently) working upgrade path has also been provided. Manual tests were performed and automated tests are green.

Performance analysis

A series of benchmarks on a fresh installation with 1000 nodes and ~500 comments was run on a sample of three common pages: the front page (50 nodes shown), the node administration page and the comment administration page (where node titles are shown). We had ~6%, ~9% and ~2% of performance penalty, respectively (head logs, patch logs | head db dump, patch db dump).

Microbenchmarking sessions were run for each updated query, showing analogous results: multilingual queries introduce performance penalties varying from nearly 0 to 15%. All the mb sessions were run to determine the most performant multilingual query, but it should be noted that the simple switch to the new schema introduces a performance penalty. This happens even for "monolingual" queries, i.e. considering the scenario of monolingual sites, where we have exactly one row for each node in the data table. For instance:

SELECT title FROM {node_field_data} WHERE status = 1 AND promote = 1 ORDER BY created

was ~10% slower than

SELECT title FROM {node} WHERE status = 1 AND promote = 1 ORDER BY created

The main culprit here seems to be the composite primary key, in fact dropping it and setting just nid as primary key makes the monolingual query above as performant as in the current HEAD.

Remaining tasks

User interface changes

None

API changes

No API changes. There are changes to queries and now it is even more important that others us EFQ and not direct database queries. We have to use EFQ to ensure translatability.

Issues blocked by this one

Follow-ups

Original report by dawehner

Based on #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) there should be a new schema. The name should be node_translation it's connected to node directly.

It's not clear how the revision should be handled, though this should be discussed on the main issue.

CommentFileSizeAuthor
#303 et-node-ml-1498674-303.patch203.73 KBplach
#301 et-node-ml-1498674-301.patch203.61 KBdas-peter
#301 interdiff-1498674-299-301-do-not-test.diff1.04 KBdas-peter
#299 et-node-ml-1498674-299.patch202.58 KBdas-peter
#299 interdiff-1498674-296-299-do-not-test.diff1.35 KBdas-peter
#297 et-node-ml-1498674-296.patch202.5 KBdas-peter
#293 et-node-ml-1498674-291.xhprof-log.zip185.97 KBplach
#284 282with.xhprof.txt866.21 KBattiks
#284 282without.xhprof.txt835.2 KBattiks
#282 et-node-ml-1498674-282.interdiff.do_not_test.patch2.93 KBplach
#282 et-node-ml-1498674-282.patch202.53 KBplach
#280 et-node-ml-1498674-280.interdiff.do_not_test.patch4.15 KBplach
#280 et-node-ml-1498674-280.patch202.37 KBplach
#274 without.xhprof.txt982.76 KBattiks
#274 with.xhprof.txt1001.76 KBattiks
#270 without-patch..xhprof.txt345.6 KBattiks
#270 with-patch..xhprof.txt360.6 KBattiks
#269 interdiff.txt890 bytesattiks
#269 i1498674-269.patch201.2 KBattiks
#266 et-node-ml-1498674-265.interdiff.do_not_test.patch1.87 KBplach
#266 et-node-ml-1498674-265.patch200.99 KBplach
#263 node-multilingual-1498674-avoid-magic-interdiff.txt1005 bytesBerdir
#258 et-node-ml-1498674-258.interdiff.do_not_test.patch4.52 KBplach
#258 et-node-ml-1498674-258.patch201.06 KBplach
#255 et-node-ml-1498674-255.interdiff.do_not_test.patch16.07 KBplach
#255 et-node-ml-1498674-255.patch201.06 KBplach
#253 ml_node.ods13.21 KBplach
#252 et-node-ml-1498674-252.patch193.98 KBplach
#249 ml_node.ods13.4 KBplach
#249 mb-results.php_.txt19.49 KBplach
#243 et-node-ml-1498674-243.interdiff.do_not_test.patch670 bytesplach
#243 et-node-ml-1498674-243.patch193.71 KBplach
#240 mb.zip3.75 KBplach
#238 et-node-ml-1498674-238.patch193.86 KBplach
#238 drupal_8x.sql_.zip1.55 MBplach
#238 head.txt4.26 KBplach
#238 patch.txt4.26 KBplach
#239 drupal_8x_dev.sql_.zip1.58 MBplach
#236 compare.png836.72 KBYesCT
#233 et-node-ml-1498674-233.patch193.46 KBYesCT
#233 interdiff-232-233.txt4.25 KBYesCT
#232 et-node-ml-1498674-232.patch193.61 KBYesCT
#231 et-node-ml-1498674-231.interdiff.do_not_test.patch7.24 KBplach
#231 et-node-ml-1498674-231.patch193.6 KBplach
#229 et-node-ml-1498674-229.interdiff.do_not_test.patch7.53 KBplach
#229 et-node-ml-1498674-229.patch191.91 KBplach
#228 et-node-ml-1498674-228.interdiff.do_not_test.patch8.43 KBplach
#228 et-node-ml-1498674-228.patch189.92 KBplach
#227 et-node-ml-1498674-227.interdiff.do_not_test.patch5.93 KBplach
#227 et-node-ml-1498674-227.patch190.95 KBplach
#226 et-node-ml-1498674-226.interdiff.do_not_test.patch1.11 KBplach
#226 et-node-ml-1498674-226.patch188.53 KBplach
#224 et-node-ml-1498674-224.interdiff.do_not_test.patch10.11 KBplach
#224 et-node-ml-1498674-224.patch187.42 KBplach
#223 et-node-ml-1498674-223.patch180.55 KBplach
#222 et-node-ml-1498674-222.interdiff.do_not_test.patch1.04 KBplach
#222 et-node-ml-1498674-222.patch181.58 KBplach
#220 et-node-ml-1498674-220.interdiff.do_not_test.patch695 bytesplach
#220 et-node-ml-1498674-220.patch180.55 KBplach
#216 et-node-ml-1498674-216.do_not_test.patch194.95 KBplach
#216 et-node-ml-1498674-216.interdiff.do_not_test.patch13.64 KBplach
#214 et-node-ml-1498674-214.interdiff.do_not_test.patch913 bytesplach
#214 et-node-ml-1498674-214.patch181.69 KBplach
#213 et-node-ml-1498674-213.interdiff.do_not_test.patch8.62 KBplach
#213 et-node-ml-1498674-213.patch181.62 KBplach
#207 et-node-ml-1498674-207.interdiff.do_not_test.patch1.37 KBplach
#207 et-node-ml-1498674-207.patch180.44 KBplach
#203 et-node-ml-1498674-203.interdiff.do_not_test.patch2 KBplach
#203 et-node-ml-1498674-203.patch179.25 KBplach
#201 et-node-ml-1498674-201.interdiff.do_not_test.patch1.26 KBplach
#201 et-node-ml-1498674-201.patch179.66 KBplach
#196 et-node-ml-1498674-196.interdiff.do_not_test.patch1.05 KBplach
#196 et-node-ml-1498674-196.patch178.39 KBplach
#195 et-node-ml-1498674-195.interdiff.do_not_test.patch839 bytesplach
#195 et-node-ml-1498674-195.patch178.39 KBplach
#194 et-node-ml-1498674-194.patch178.14 KBplach
#193 et-node-ml-1498674-193.patch1.29 KBplach
#190 et-node-ml-1498674-190.interdiff.do_not_test.patch1.31 KBplach
#190 et-node-ml-1498674-190.patch178.34 KBplach
#188 et-node-ml-1498674-188.interdiff.do_not_test.patch3.45 KBplach
#188 et-node-ml-1498674-188.patch178.3 KBplach
#185 et-node-ml-1498674-185.patch177.14 KBYesCT
#181 et-node-ml-1498674-181.patch177.15 KBYesCT
#181 interdiff-179-181.txt7.6 KBYesCT
#179 et-node-ml-1498674-179.patch177.75 KBplach
#177 et-node-ml-1498674-176.patch175.64 KBplach
#176 et-node-ml-1498674-175.patch171.45 KBplach
#173 et-node-ml-1498674-172.patch171.45 KBplach
#171 et-node-ml-1498674-171.patch163.97 KBplach
#169 et-node-ml-1498674-167.patch164.94 KBplach
#167 et-node-ml-1498674-165.patch145.49 KBplach
#165 et-node-ml-1498674-165.patch140.42 KBplach
#163 et-node-ml-1498674-163.patch137.29 KBplach
#161 et-node-ml-1498674-161.patch133.48 KBplach
#159 et-node-ml-1498674-159.patch132.84 KBplach
#157 et-node-ml-1498674-157.interdiff.do_not_test.patch5.81 KBplach
#157 et-node-ml-1498674-157.patch132.47 KBplach
#155 et-node-ml-1498674-155.patch126.03 KBplach
#149 et-node-ml-1498674-149.interdiff.do_not_test.patch698 bytesplach
#149 et-node-ml-1498674-149.patch32.09 KBplach
#147 et-node-ml-1498674-147.interdiff.do_not_test.patch7.07 KBplach
#147 et-node-ml-1498674-147.patch31.41 KBplach
#145 et-node-ml-1498674-145.do_not_test.patch25.05 KBplach
#145 et-node-ml-1498674-145.interdiff.do_not_test.patch6.25 KBplach
#144 et-node-ml-1498674-144.do_not_test.patch19.09 KBplach
#132 image003.jpeg31.66 KBGábor Hojtsy
#122 node-property-1498674-122.patch150.81 KBSchnitzel
#121 node-property-1498674-121.patch150.88 KBSchnitzel
#121 interdiff-191-121.txt1.7 KBSchnitzel
#119 node-property-1498674-119.patch150.87 KBSchnitzel
#114 node-property-1498674-115.patch150.87 KBBerdir
#114 node-property-1498674-115-interdiff.txt2.51 KBBerdir
#109 drupal-1498674-109.patch150.8 KBSchnitzel
#106 drupal-1498674-106.patch149.89 KBSchnitzel
#104 drupal-1498674-104.patch149.42 KBSchnitzel
#104 interdiff-102-104.txt1.15 KBSchnitzel
#102 drupal-1498674-102.patch148.72 KBSchnitzel
#102 interdiff-100-102.txt3.47 KBSchnitzel
#100 drupal-1498674-100.patch148.75 KBCarsten Müller
#97 drupal-1498674-97.patch148.75 KBSchnitzel
#94 drupal-1498674-94.patch171.94 KBdawehner
#94 interdiff.txt1.16 KBdawehner
#91 drupal-1498674-91.patch156.59 KBdawehner
#91 interdiff.txt47.41 KBdawehner
#88 interdiff.txt49.81 KBdawehner
#88 core-1498674-88.patch147.13 KBdawehner
#87 interdiff.txt10 KBdawehner
#76 drupal-node-properties-multilingual-1498674-76.patch97.32 KBdas-peter
#76 interdiff-75-76.txt17.48 KBdas-peter
#75 drupal-node-properties-multilingual-1498674-75.patch91.42 KBdas-peter
#75 interdiff-61-75.txt10.25 KBdas-peter
#73 drupal-node-properties-multilingual-1498674-73.patch92.05 KBdas-peter
#73 interdiff-61-73.txt11.18 KBdas-peter
#69 drupal-node-properties-multilingual-1498674-69.patch91.65 KBdas-peter
#69 interdiff-61-69.txt10.48 KBdas-peter
#67 drupal-node-properties-multilingual-1498674-67.patch91.71 KBdas-peter
#67 interdiff-61-67.txt7.47 KBdas-peter
#64 interdiff-1498674-59-61.txt10.67 KBdas-peter
#61 drupal-node-properties-multilingual-1498674-61.patch85.5 KBdas-peter
#59 drupal-node-properties-multilingual-1498674-59.patch84.12 KBdas-peter
#56 drupal-node-properties-multilingual-1498674-56.patch85.91 KBdas-peter
#56 query_diff-1498674-50-56.txt29.31 KBdas-peter
#54 query_diff-1498674-50-54.txt30.79 KBdas-peter
#54 drupal-node-properties-multilingual-1498674-54.patch85.93 KBdas-peter
#52 drupal-node-properties-multilingual-1498674-52.patch87.35 KBdas-peter
#52 query_diff-1498674-50-52.txt31.04 KBdas-peter
#50 drupal-node-properties-multilingual-1498674-50.patch77.9 KBdas-peter
#48 drupal-node-properties-multilingual-1498674-48.patch76.54 KBdas-peter
#46 drupal-node-properties-multilingual-1498674-45.patch75.61 KBdas-peter
#38 drupal-node-properties-multilingual-1498674-38.patch73.47 KBdas-peter
#35 drupal-node-properties-multilingual-1498674-35.patch72.59 KBdas-peter
#31 drupal-node-properties-multilingual-1498674-31.patch422.06 KBdas-peter
#29 drupal-node-properties-multilingual-1498674-27.patch409.12 KBdas-peter
#27 drupal-node-properties-multilingual-1498674-27.patch409.45 KBdas-peter
#25 drupal-node-properties-multilingual-1498674-25.patch409.45 KBdas-peter
#23 drupal-node-properties-multilingual-1498674-23.patch26.35 KBdas-peter
#18 drupal-node-properties-multilingual-1498674-18.patch2.66 KBdas-peter
#10 node-translation-schema-1.1.patch8.57 KBSoul88
#6 node-translation-schema.patch8.28 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klonos’s picture

Title: Define node translation » Define node translation schema

...title edit in order to be in tandem with the rest of the issues from #1498634: [meta] Define revision/translation SQL schema for core entities (no patch)

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +sprint

As per #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) this should be worked on first and foremost with a static copy of the base node and node_revision tables for the needed properties.

andypost’s picture

What to do with title? should this issue postponed for #1188394: Make title behave as a configurable, translatable field (again)

effulgentsia’s picture

IMO, we should not postpone this on #1188394: Make title behave as a configurable, translatable field (again). {node} and {node_revision} have title at this time. Therefore, {node_translation} and {node_revision_translation} should have title in any patch posted here. If the other issue ends up getting committed first and removes title from {node} and {node_revision}, then patches here can be rerolled accordingly. OTOH, if this issue lands first, then patches on that issue can be rerolled to remove title from 4 tables instead of 2. Seems to me that the work needed on either issue to adjust for the other one is tiny compared to the benefit of allowing both to proceed independently.

catch’s picture

One issue that will definitely conflict with this one is #1528028: Add tests for reverting revisions where revision_uid and uid differ, I'm not sure if I can get away with it but it'd be really nice to fix the node_revisions schema before rather than after duplicating it (/me ducks).

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
8.28 KB

@catch: I don't think we even remotely have the bandwith to take on #1528028: Add tests for reverting revisions where revision_uid and uid differ to move this one forward as we are pretty much behind on property/entity translation work.

@all: Here is a quick patch to get us started. I think this is pretty simple and easily updated later if the node or revisions schema changes. The regular schema has the schemas as copies, just the description for the table is different. The update functions need to replicate the whole thing. I looked at the properties, and we plan to loose tnid on nodes later (but we have a long way to go to be able to replicate that functionality first) and changing the type for translation sounds very dangerous. Otherwise all properties on node and node_revision seemed to make complete sense to allow for translation, such as the author or title or sticky but.

Soul88’s picture

Hello, Gábor.

I looked at your patch and was a little bit confused about the foreign keys that are cloned from node* tables to node_translation and node_revision_translation. I'm not sure whether they should be cloned this way.

So does "$schema['node_translation']" table need to have such a foreign key:

    'foreign keys' => array(
      'node_revision' => array(
        'table' => 'node_revision',
        'columns' => array('vid' => 'vid'),
      ),

maybe `node_translation`should point on `node_revision_translation`?

andypost’s picture

Another issue with this approach - schema could be altered but current implementation relay onto un-altered schema values

Gábor Hojtsy’s picture

@andypost: can you elaborate on that?

Soul88’s picture

I've slightly changed the patch

Gábor Hojtsy’s picture

Status: Needs review » Postponed

There is renewed interest in discussing #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) and this approach might not go well after all. Marking postponed for that.

Gábor Hojtsy’s picture

Issue tags: -sprint

There is (very slowly) ongoing discussion in #1498634: [meta] Define revision/translation SQL schema for core entities (no patch), so there is no point in trying to keep this on the high visibility sprint queue.

plach’s picture

Title: Define node translation schema » Refactor node schema to multilingual
plach’s picture

Title: Refactor node schema to multilingual » Refactor node properties to multilingual

Retitling

effulgentsia’s picture

Status: Postponed » Needs work

#1498634: [meta] Define revision/translation SQL schema for core entities (no patch) landed, so this doesn't need to be postponed any more. I presume next step here is to rework #10 to be in line with patterns established in #1658712: Refactor test_entity schema to be multilingual, so "needs work".

Gábor Hojtsy’s picture

Issue tags: +sprint

We hoped that #1691952: Make EntityFieldQuery work with multilingual properties would greatly help with this to rewrite existing queries, but you indicated in person that there might not be as many queries as we envision there are, so why not get started on this? The schema needs to be reworked anyway, which will then result in massive fails to resolve :D

plach’s picture

I'll start working on this soon. If anyone else wishes to take it meanwhile, feel free.

das-peter’s picture

Status: Needs work » Needs review
FileSize
2.66 KB

Attached patch should introduce the schema as defined in [#1722906]
I'm not sure if really all of the fields need to be cloned - feedback appreciated.

The update hook creates tables and sets the initial data.

das-peter’s picture

Status: Needs review » Needs work

Oh, looks like I duplicated to much.
I'm not really sure how the schema has to look :|
Currently I'm thinking about something like this:

node
Contains only stateless information:
nid
uuid
vid
type
langcode
node_property_data
Contains stateful and multilingual information of the current version:
nid
uuid
vid
type
langcode
title
uid
status
created
changed
comment
promote
sticky
tnid
translate

node_property_data_revision
Contains stateful, multilingual and version information:
nid
uuid
vid
type
langcode
title
uid
status
created
changed
comment
promote
sticky
tnid
translate
log
plach’s picture

According to #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) uuid and type should not be repeated in the data and revision tables. Probably also tnid and translate should be left out of the revision table: they aren't there right now and are likely to be dropped if we will be able to remove node translation from core.

das-peter’s picture

Okay, I'll move uuid, type, tnid and translate.
What about the created field? This seems to be stateless too.
Further I forgot to add the field timestamp to node_property_data_revision in the above list. And I thought about rename this field to changed, does that make sense?
Another thing I'm unsure about is, if it is okay to rename node_revision to node_property_data_revision?

plach’s picture

Honestly I'd leave column names alone to avoid having to perform too much changes in a single patch. This should make things easier. Having a different created per language might make sense if we don't make all the similar authoring metadata entity translation properties. For now I'd simply move it in the data table along with the rest.

@Gabor, @effulgentsia: what do you think?

das-peter’s picture

Status: Needs work » Needs review
FileSize
26.35 KB

I'll created a patch already with all the changes mentioned.
If it's to much we can revert some of the stuff.
I tested this on my local environment: admin list, viewing and editing seem to work, but I didn't run the tests - let's see what happens here.

Attention: The patch also contains changes of Drupal\Core\Entity\DatabaseStorageController, the default Controller wasn't able to deal with the data table. I've added similar handling as for revisions.

das-peter’s picture

Status: Needs review » Needs work

Berdir just told me that the handling of the data table is covered in Drupal\Core\Entity\DatabaseStorageControllerNG.
But to use this we've to properly define the properties of the node entity.
An example of such a property definition can be found here: #1778178: Convert comments to the new Entity Field API

das-peter’s picture

Status: Needs work » Needs review
FileSize
409.45 KB

Hmm, the new Entity Property API is interesting. Let's see what fails...
If this patch really should go in, I'll split it in smaller junks so a review is possible!
Currently it contains parts of #1778178: Convert comments to the new Entity Field API and some custom adjustments make DatabaseStorageControllerNG working with the data table.

das-peter’s picture

Damn, silly copy paste error. Let's try again.

das-peter’s picture

Silly me, I should commit before I create the patch :|

das-peter’s picture

Status: Needs review » Needs work
FileSize
422.06 KB

I'm still working on this.
As soon as I've created a test worthy version I'll split this up into testable junks - this version just fails everywhere.
For now I stay with this monster to reduce the overhead.

I guess there are still some flaws in the new Entity Property API.
Interestingly the behaviour in WebTests is totally different from the behaviour on the site itself. So far I've no clue why...

Gábor Hojtsy’s picture

@plach, @das-peter: indeed having a different creation time per language is definitely desired. Additionally I would not perform any column renames or removals. Even with this patch landed, it is still not possible to translate nodes without the content translation module in core, so the DB fields for that are needed. If/once we remove that, we can remove the corresponding fields, we need to maintain them until then.

Are changes like $node->nid to $node->id() required? I'd hope not :) This patch is starting to look like a monster :/

das-peter’s picture

Okay, then creation goes in node_property_data and <code>node_property_data_revision.

Are changes like $node->nid to $node->id() required? I'd hope not :) This patch is starting to look like a monster :/

Yes and yes.
$node->nid becomes $node->nid->value with EntityNG so why not switch to <code>$node->id() right away?
And it will become a monster, but I think I'll be able to split it into reviewable junks.

Gábor Hojtsy’s picture

$node->nid becomes $node->nid->value with EntityNG so why not switch to $node->id() right away?

If it is not needed to introduce this change, then please, pretty please DO NOT include it. A couple hundred more kilobytes in the patch and a lot more to argue about is not what we need and this is a cornerstone feature for Drupal 8 that we absolutely need to be able to drop the node-copy translation method which we'd need to do in the remaining 54 days as well. So this patch ideally has something like a 2 weeks (maybe 3 weeks at best) to be fully baked and get committed, which is precious little time in practice.

das-peter’s picture

Status: Needs work » Needs review
FileSize
72.59 KB

As with Gabor discussed in IRC here's a patch that just covers the absolute minimum of changes. Attention: This still includes changes in the Drupal\Core\Entity\DatabaseStorageController and core/includes/entity.inc.
All other changes should be really related to the changed db schema.

Locally the node tests were successful. Let's see if there are other tests that fail.

Once this patch is RTBC I'd like to continue with my other approach and convert nodes to the new Entity Field/Property API (entityNG).
The work I already did revealed some flaws in the current entityNG version and thus was a very good proof of concept. However, I'll create a spin-off similar to #1778178: Convert comments to the new Entity Field API to keep this issue clean - or does somebody know if there's already such an issue?

effulgentsia’s picture

Status: Needs review » Needs work

@das-peter: I don't think anyone's opened an issue for converting Node to EntityNG yet. Go for it.

das-peter’s picture

Status: Needs work » Needs review
FileSize
73.47 KB

Here comes the next version.
Interdiff needed?

plach’s picture

Will have a look to this later today.

plach’s picture

Status: Needs review » Needs work

Sorry, couldn't finish my code review. Too tired. So far the patch doesn't look bad, however I found a couple of serious issues with it:

  • The node schema does not match the standard we agreed upon: we are missing a default_langcode column. See the test entity and the related storage controller for details. Moreover the revision table should be named node_property_revision (see the Field API tables).
  • The patch never takes into account (at least in the first half) the fact that the data table may hold more than one record per node. Every ported query needs to be adjusted to use either a distinct clause or a language condition, depending on the business logic. In the latter case, sometimes it might make sense to hardcode a default_langcode = 1 condition, in other cases an explicit language condition would be needed.
das-peter’s picture

@plach: Thanks for taking the time for a review, I know this is quite a bit :)

*Gnaaa* I didn't recognize that #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) and the change record differ :| ( I've updated the issue summary now)
Changing that should be quite easy.

The patch never takes into account (at least in the first half) the fact that the data table may hold more than one record per node.

That's absolutely right. I've changed nothing in the logic so far.
And I'd like to clarify first which of the queries are "nececssary" anyway - as far as I know we shouldn't use direct queries at all and use EFQ instead.

I'll create a patch with the schema adjustments asap. But before I start to change the query stuff I'd like to clarify the approach.

Berdir’s picture

Agreed about EFQ, those queries should most likely be converted to that. Note that EFQ is currently being re-designed/refactored to provide a better API and be entity type specific in #1801726: EntityFieldQuery v2. Downside of that is that it would conflict with an attempt to convert all that stuff to EFQ in here. So if there's an easy way to avoid that, we should probably do that, open a follow-up (I guess that could be tackled after the feature freeze once EFQv2 is in place?)

plach’s picture

I definitely think we should split the EFQ conversion from the work here. EFQs do and will support multilingual properties, but don't provide anything out of the box: it's still responsibility of the API consumer to set the correct language conditions to implement the business logic correctly. Defining/implementing this logic is exactly the goal of this issue, while moving to EFQ is not. It should be a straightforward follow-up, once we have the updated queries in place, to covert them to EFQ. And it would make a lot of sense to wait for EFQs to be refactored.

das-peter’s picture

Had the same discussion with berdir in IRC. Let's wait with the EFQ change - this will keep the review a bit easier too.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Updated the issue summary.

das-peter’s picture

Updated patch:

  • Adjusted schema. I hope it matches to the change record now.
  • Adjusted queries.
  • Adjusted the way how nodes are inserted. The revision is created first to ensure we've a vid to insert into the property data table, otherwise we can't use vid in the primary key.

ToDo: Update query logic where necessary.

plach’s picture

Didn't review the patch yet. However rules of thumb to convert queries:

  • for instance count queries can use a distinct clause
  • also queries selecting nids, probably most of the time can use a distinct
  • admin listings can probanly go with the default langauge (or the current content language if explictly selected)
das-peter’s picture

Added "handling" of default_langcode property.

Berdir’s picture

is there a reason you didn't send the patches to the testbot?

+++ b/core/includes/database.incundefined
@@ -41,8 +41,8 @@
  * one would instead call the Drupal functions:
  * @code
- * $result = db_query_range('SELECT n.nid, n.title, n.created
- *   FROM {node} n WHERE n.uid = :uid', 0, 10, array(':uid' => $uid));
+ * $result = db_query_range('SELECT npd.nid, npd.title, n.created
+ *   FROM {node_property_data} npd WHERE npd.uid = :uid', 0, 10, array(':uid' => $uid));
  * foreach ($result as $record) {
  *   // Perform operations on $record->title, etc. here.
  * }
@@ -69,7 +69,7 @@

@@ -69,7 +69,7 @@
  *
  * Named placeholders begin with a colon followed by a unique string. Example:
  * @code
- * SELECT nid, title FROM {node} WHERE uid=:uid;
+ * SELECT nid, title FROM {node_property_data} WHERE uid=:uid;
  * @endcode
  *
  * ":uid" is a placeholder that will be replaced with a literal value when
@@ -81,7 +81,7 @@

@@ -81,7 +81,7 @@
  *
  * Unnamed placeholders are simply a question mark. Example:
  * @code
- * SELECT nid, title FROM {node} WHERE uid=?;
+ * SELECT nid, title FROM {node_property_data} WHERE uid=?;
  * @endcode
  *
  * In this case, the array of arguments must be an indexed array of values to
@@ -91,11 +91,11 @@

@@ -91,11 +91,11 @@
  * running a LIKE query the SQL wildcard character, %, should be part of the
  * value, not the query itself. Thus, the following is incorrect:
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title%;
+ * SELECT nid, title FROM {node_property_data} WHERE title LIKE :title%;
  * @endcode
  * It should instead read:
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title;
+ * SELECT nid, title FROM {node_property_data} WHERE title LIKE :title;
  * @endcode
  * and the value for :title should include a % as appropriate. Again, note the

These are generic DB API usage examples. I'm wondering if we should switch node to example or something, instead of an actual existing table? Among other problems, this does incorrectly suggest to use db_query() to load node titles, which you shouldn't be doing.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.phpundefined
@@ -33,7 +33,7 @@ public function orderRandom() {
    *   $query = db_select('node', 'n');
-   *   $query->join('node_revision', 'nr', 'n.vid = nr.vid');
+   *   $query->join('node_property_revision', 'nr', 'n.vid = npr.vid');
    *   $query
    *     ->distinct()

Same thing here.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -117,7 +117,7 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
       '#title' => t('Revision information'),
       '#collapsible' => TRUE,
-      // Collapsed by default when "Create new revision" is unchecked
+      // Collapsed by default when "Create new revision" is unchecked.
       '#collapsed' => !$node->revision,
       '#group' => 'additional_settings',
       '#attributes' => array(
@@ -363,9 +363,9 @@ protected function submitNodeLanguage(array $form, array &$form_state) {

@@ -363,9 +363,9 @@ protected function submitNodeLanguage(array $form, array &$form_state) {
   /**
    * Form submission handler for the 'preview' action.
    *
-   * @param $form
+   * @param array $form
    *   An associative array containing the structure of the form.
-   * @param $form_state
+   * @param array $form_state
    *   A reference to a keyed array containing the current state of the form.

Unrelated changes?

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -272,7 +288,7 @@ protected function preSave(EntityInterface $node) {
    */
-  function postSave(EntityInterface $node, $update) {
+  public function postSave(EntityInterface $node, $update) {
     // Update the node access table for this node, but only if it is the
     // default revision. There's no need to delete existing records if the node
     // is new.
@@ -283,7 +299,7 @@ function postSave(EntityInterface $node, $update) {

@@ -283,7 +299,7 @@ function postSave(EntityInterface $node, $update) {
   /**
    * Overrides Drupal\Core\Entity\DatabaseStorageController::preDelete().
    */
-  function preDelete($entities) {
+  public function preDelete($entities) {
     if (module_exists('search')) {
       foreach ($entities as $id => $entity) {

When fixing it, fix it correctly, these should be protected. Or leave it alone ;)

+++ b/core/modules/node/node.installundefined
@@ -677,6 +739,110 @@ function node_update_8006(&$sandbox) {
+  if (db_table_exists('node_property_data')) {
+    $node_property_count = db_select('node_property_data')->countQuery()->execute()->fetchColumn();
+    if ($node_property_count != $node_count) {
+      $source_query = db_select('node')
+        ->fields('node', array_merge(
+          array('nid', 'vid', 'langcode'),
+          $deprecated_base_table_fields
+        ));
+      $source_query->addField('node', 'langcode', 'default_langcode');
+      db_insert('node_property_data')->from($source_query)->execute();
+      $node_property_count = db_select('node_property_data')->countQuery()->execute()->fetchColumn();

Not sure if I wan't to know what will happen with this query on d.o :) Might need to be done in batching?

Also, I think it's preferable to split the initial table creating and filling them with data into separate update functions, especially when having to do it in a batch.

And same for dropping the old columns/tables I guess.

das-peter’s picture

Status: Needs work » Needs review
FileSize
77.9 KB

These are generic DB API usage examples.

I would be happy to change them. And I think example would be a good table name. If there are no objections I'll change that.

Unrelated changes?

*SCNR* It's really hard for me to check my code and leave other parts unchanged, but I reduce it to a minimum - promised :D

When fixing it, fix it correctly, these should be protected. Or leave it alone ;)

My goal was not to change the behaviour while having code that follows the coding standards - but if you say me protected is ok, here we go.

Not sure if I wan't to know what will happen with this query on d.o :)

You're right. I've split up the update in 4 functions.

Still no query changes, that's what I'm doing now.
I give the testbot a shot but I don't think it's test worthy atm.

das-peter’s picture

Fixed issue with storing revisions - someone should take a look at this :|
Hope we can switch to the revision enhancement by berdir soon #1723892: Support for revisions for entity save and delete operations

I started to convert queries, but it doesn't feel right. I guess I make more mess than solving a single thing.
The file query_diff-1498674-50-52.txt shows which queries I changed after simply changing the table names / adding necessary joins.

ReExp-Patterns I used to search for related queries:

  • Old Tables:
    (db_(select|insert|delete|update)\(('|")(node|node_revision)('|")|\{(node|node_revision)\}|\>(inner|outer|left|right|)join\(('|")(node|node_revision)('|")|(FROM|INTO|JOIN) (node|node_revision)))
  • New Tables:
    (db_(select|insert|delete|update)\(('|")(node|node_property_data|node_property_data_revision)('|")|\{(node|node_property_data|node_property_data_revision)\}|\>(inner|outer|left|right|)join\(('|")(node|node_property_data|node_property_data_revision)('|")|(FROM|INTO|JOIN) node)
das-peter’s picture

Ouch, there were some quite stupid mistakes in the patch.
Unfortunately, it seems like I messed even more up with rebase to the latest 8.x, cross fingers I was able to clean up the mess. :|

das-peter’s picture

plach’s picture

Yay, will look into this as soon as I can!

das-peter’s picture

#1723892: Support for revisions for entity save and delete operations just was committed *yay*
I guess this needs a re-roll, and even if not I suggest to switch to this general approach :)

das-peter’s picture

And here's the re-roll.

plach’s picture

Status: Needs review » Needs work

I couldn't review every single query yet, but it seems to me they are not dealing with multilingual properties yet. It might make sense to leav the tougher ones to dedicated follow-ups but I don't think every query here deserves this. Also we need new or updated upgrade path tests, because the current update functions are broken but tests pass.

Moreover I think a bit more consistency with the EntityTestStorageController will help the future merge of all the property stuff into the base storage controller.

+++ b/core/includes/database.inc
@@ -37,12 +37,12 @@
- * SELECT n.nid, n.title, n.created FROM node n WHERE n.uid = $uid LIMIT 0, 10;
+ * SELECT e.id, e.title, e.created FROM example e WHERE e.uid = $uid LIMIT 0, 10;
  * @endcode
  * one would instead call the Drupal functions:
  * @code
- * $result = db_query_range('SELECT n.nid, n.title, n.created
- *   FROM {node} n WHERE n.uid = :uid', 0, 10, array(':uid' => $uid));
+ * $result = db_query_range('SELECT e.id, e.title, e.created
+ *   FROM {example} e WHERE e.uid = :uid', 0, 10, array(':uid' => $uid));

These examples are node-related: node-specific queries should be kept or the textual description updated as well.

+++ b/core/includes/database.inc
@@ -109,7 +109,7 @@
+ * INSERT INTO example (id, title, body) VALUES (1, 'my title', 'my body');

Can we make this example even less nodesque? Title and body smell bad.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -124,6 +131,11 @@ public function __construct($entityType) {
+    // Check if the entity type  has multilingual support.

Double space before 'has'. Also: there is nothing in code that explicitly refers to multilingual stuff. Not sure whether the comment should mention it.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -324,6 +336,10 @@ protected function buildQuery($ids, $revision_id = FALSE) {
+    if (!empty($this->dataTable)) {
+      $query->join($this->dataTable, 'data', "data.{$this->idKey} = base.{$this->idKey}");
+    }

@@ -348,6 +364,22 @@ protected function buildQuery($ids, $revision_id = FALSE) {
+    // Add fields from the entity data table.
+    if (!empty($this->dataTable)) {
...
+    }

The current EntityTestStorageController attaches data values in a second pass avoiding the join. Whether this approach is more performant is debatable, but it mimicks the current field attach process, and lets the door open for more unification at storage level.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -513,6 +545,9 @@ public function save(EntityInterface $entity) {
+        if ($this->dataTable) {
+          drupal_write_record($this->entityInfo['data table'], $entity, $this->idKey);
+        }

@@ -522,6 +557,9 @@ public function save(EntityInterface $entity) {
+        if ($this->dataTable) {
+          drupal_write_record($this->entityInfo['data table'], $entity);
+        }

This is not storing per-language values, moreover the primary key should be (nid, vid, langcode). Probably data and revisions table should be handled in the postSave() method as in the test entity controller.

+++ b/core/modules/book/book.module
@@ -394,20 +394,22 @@ function book_get_books() {
+        $node = node_load($link['nid']);

+++ b/core/modules/comment/comment.admin.inc
@@ -97,8 +96,9 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    $node = node_load($row->nid);

Can we use node_load_multiple() here?

+++ b/core/modules/comment/comment.install
@@ -34,14 +34,15 @@ function comment_uninstall() {
+  $query->groupBy('npd.nid');

I don't think this is dealing with multingual properties correctly. We need at very least a @todo, since we need per-language ncs. Perhaps just making them node properties?

+++ b/core/modules/language/language.api.php
@@ -52,8 +52,8 @@ function hook_language_update($language) {
-    ->fields(array('language' => ''))
-    ->condition('language', $language->langcode)
+    ->fields(array('langcode' => ''))
+    ->condition('langcode', $language->langcode)

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -368,9 +368,9 @@ protected function submitNodeLanguage(array $form, array &$form_state) {
-   * @param $form
+   * @param array $form
    *   An associative array containing the structure of the form.
-   * @param $form_state
+   * @param array $form_state

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -80,7 +79,7 @@ protected function invokeHook($hook, EntityInterface $node) {
+    elseif ($hook == 'predelete') {

@@ -133,7 +119,7 @@ protected function preSaveRevision(array &$record, EntityInterface $entity) {
-  function postSave(EntityInterface $node, $update) {
+  protected function postSave(EntityInterface $node, $update) {

@@ -144,7 +130,7 @@ function postSave(EntityInterface $node, $update) {
-  function preDelete($entities) {
+  protected function preDelete($entities) {

+++ b/core/modules/node/node.install
@@ -365,7 +427,7 @@ function node_schema() {
+        'size' => 'tiny',

Unrelated?

+++ b/core/modules/node/lib/Drupal/node/Node.php
@@ -61,6 +61,13 @@ class Node extends Entity implements ContentEntityInterface {
   /**
+   * The node default language code.
+   *
+   * @var string
+   */
+  public $default_langcode;
+
+  /**

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -95,32 +94,19 @@ protected function invokeHook($hook, EntityInterface $node) {
+
+    // Make sure the default language is set.
+    if (empty($node->default_langcode)) {
+      $node->default_langcode = $node->langcode;
+    }

We should not define new properties this way now that we have the Entity Field API. However it's not clear to me why we need this.

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -95,32 +94,19 @@ protected function invokeHook($hook, EntityInterface $node) {
-      // this code allows us to avoid clobbering an existing log entry with an
-      // empty one.
+    // Make sure an existing log entry isn't overwritten unnecessarily.
+    if (empty($record['log'])) {
       unset($record['log']);
     }

Why do we need this change?

+++ b/core/modules/node/node.admin.inc
@@ -477,42 +481,43 @@ function node_admin_nodes() {
-    ->fields('n',array('nid'))
+    ->fields('npd',array('nid'))

Missing space after comma.

+++ b/core/modules/node/node.install
@@ -47,6 +47,68 @@ function node_schema() {
+      'node_type'     => array(array('type', 4)),
+      'tnid'          => array('tnid'),
+      'translate'     => array('translate'),

Aligning stuff vertically is discouraged AFAIK.

+++ b/core/modules/node/node.install
@@ -47,6 +47,68 @@ function node_schema() {
+        'description' => 'The current {node_revision}.vid version identifier.',

Should be {node_property_revision}.

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+  $node_count = db_select('node')->countQuery()->execute()->fetchColumn();

Useless leftover?

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+    $sandbox['last'] = (int) db_query('SELECT npd.nid FROM node_property_data npd  ORDER BY nid DESC')->fetchField();
+    $sandbox['max'] = db_query('SELECT COUNT(*) FROM {node} n LEFT JOIN node_property_data npd ON npd.nid = n.nid WHERE npd.nid IS NULL')->fetchField();
...
+    $sandbox['last'] = db_query('SELECT npd.nid FROM node_property_data npd  ORDER BY nid DESC')->fetchField();
...
+    $sandbox['max'] = db_query('SELECT COUNT(*) FROM {node_revision} nr LEFT JOIN node_property_revision npr ON npr.vid = nr.vid WHERE npr.vid IS NULL')->fetchField();

Missing table prefixing braces.

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+      ->range(0, 5000)
...
+    $sandbox['progress'] += 5000;

Normally we use a step of 10 or so. Five thousands sounds definitely too high.

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+      ->range(0, 1)

Only one?

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+  $deprecated_base_table_fields = array(
...
+  $deprecated_base_table_indexes = array(

Can we call these just old instead of deprecated? They are going to be dropped after all.

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+  if ($node_property_count == $node_count) {
...
+  if (db_table_exists('node_revision') && $node_revision_count == $node_property_revision_count) {

I guess we should fail and notify if things go wrong here.

+++ b/core/modules/node/node.install
@@ -677,6 +739,155 @@ function node_update_8006(&$sandbox) {
+        db_drop_index('node', 'node_changed');

Should be $deprecated_index

das-peter’s picture

Status: Needs work » Needs review
FileSize
85.5 KB

Also we need new or updated upgrade path tests, because the current update functions are broken but tests pass.

I didn't update / extend the tests yet. This is just a small re-roll.

Moreover I think a bit more consistency with the EntityTestStorageController will help the future merge of all the property stuff into the base storage controller.

I'm not sure how much of that is applicable without going "NG".

These examples are node-related: node-specific queries should be kept or the textual description updated as well.

Text adjusted.

Can we make this example even less nodesque? Title and body smell bad.

Tried to change it to something less nodesque.

Double space before 'has'. Also: there is nothing in code that explicitly refers to multilingual stuff. Not sure whether the comment should mention it.

Removed space, changed description.

The current EntityTestStorageController attaches data values in a second pass avoiding the join. Whether this approach is more performant is debatable, but it mimicks the current field attach process, and lets the door open for more unification at storage level.

I've no qualified opinion on this.

This is not storing per-language values, moreover the primary key should be (nid, vid, langcode). Probably data and revisions table should be handled in the postSave() method as in the test entity controller.

Primary keys adjusted, but I don't think that will work, a db_merge() looks more appropriate. What about using db_merge() in drupal_write_record() when primary keys are provided?
And I've no qualified opinion about where this should be handled.

Can we use node_load_multiple() here?

I know not enough about the comment system to be sure if e.g. fetchAllAssoc('nid') could be used to fetch all node ids at once.

I don't think this is dealing with multingual properties correctly. We need at very least a @todo, since we need per-language ncs. Perhaps just making them node properties?

There are looooots of locations where I've absolutely no clue how we shall handle multilingual properties correctly. The grouping is a lazy approach to try to keep the existing behaviour.
query_diff-1498674-50-56.txt shows the adjusted queries.

Unrelated?

Yes. Is it really too disturbing?

We should not define new properties this way now that we have the Entity Field API. However it's not clear to me why we need this.

Exactly, we should use EntityNG - but that's from the table for now. I surely had a reason to add it, but I can't remember why :|

Why do we need this change?

Because this is the chance to get rid of this strange construct and the todo that says make log nullable.

Missing space after comma.

Fixed.

Aligning stuff vertically is discouraged AFAIK.

Was copy pasted - now adjusted.

Should be {node_property_revision}.

Fixed.

Useless leftover?

Yes, removed.

Missing table prefixing braces.

Fixed.

Normally we use a step of 10 or so. Five thousands sounds definitely too high.

Changed to 10 - but as this is a purely storage based migration it should be quite performant, thus 10 looks very low to me.

Only one?

Damn, debugging stuff left. Fixed.

Can we call these just old instead of deprecated? They are going to be dropped after all.

We can. Changed.

I guess we should fail and notify if things go wrong here.

Added an Exception with a message indicating what could be wrong.

Should be $deprecated_index

Indeed, fixed.

I expect failing tests for this, because of the adjusted drupal_write_record() in DatabaseStorageController::save()!

plach’s picture

Thanks, can you post an interdiff?

das-peter’s picture

Status: Needs review » Needs work
FileSize
10.67 KB

Thanks, can you post an interdiff?

Sure :)

das-peter’s picture

I just tried to fix the drupal_write_record() in DatabaseStorageController::save()
Doing so I realized that we don't have language as entity key in the hook_entity_info().
Gabor told me that langcode was briefly a must have before but then was made optional.
This means I simply can't define the primary_keys for drupal_write_record() statically e.g. similar to this:

$primary_keys = array(
            $this->idKey,
            $this->revisionKey,
            'langcode',
          );

First I don't know if there's really a language and second if there's one I don't know if it's really called langcode.
Looks like we should add language to hook_entity_info() and mark it as optional - similar to uuid.
Then we handle it like idKey or revisionKey.

If this is clarified my next concern is drupal_write_record() itself. The function is really handy for our non "NG" case because it deals with the entity object and figures out what to store of it.
Unfortunately, we will come across cases in which the base table is updated while the property table needs an insert. E.g. Properties are stored in a new language.
There drupal_write_record() let's us down because it doesn't use db_merge(). So we can't rely on the base table action, we need to query the table to figure out what kind of action is necessary for the property table.
I'm really wondering :

  • if we should do this extra query,
  • if we should use db_merge() what means building the whole schema object property mapping on our own (This mapping would be already done in the "NG" controller), or
  • if we should adjust drupal_write_record() to use db_merge()

Ideas, suggestions, solutions? Everything's welcome :)

das-peter’s picture

Another thing that will be ugly to handle is the langcode / default_langcode naming.
Since the base table uses langcode as name for the default language, while the property tables use default_langcode we've to switch the data in the record we want to save between saving it into the base table and saving it into the property table.
Result is something like:

$property_langcode = $node->langcode;
$node->langcode = $node->default_langcode;
drupal_write_record($this->entityInfo['base table'], $entity, $this->idKey);

$node->default_langcode = $node->langcode;
$node->langcode = $property_langcode;
drupal_write_record($this->entityInfo['data table'], $entity, array($this->idKey, $this->revisionKey, 'langcode'));

This looks very ugly :| Was there a reason why we don't use default_langcode in the base table as well?

That said, how is default_langcode handled in the entity_info? Will the decision for the possible 'language' entity key apply to this naming too?

das-peter’s picture

Status: Needs work » Needs review
FileSize
7.47 KB
91.71 KB

Because of lack of feedback I just try to go on with the ugly workaround for the langcode / default_langcode issue.
Further I updated the storing mechanism of revisions and added some tests to check if properties / revisions are created language dependent.

das-peter’s picture

Status: Needs review » Needs work
FileSize
10.48 KB
91.65 KB

*blargh* note for myself: I've to check the code more careful after doing a merge. :D

Reordered and renamed the update hooks.

das-peter’s picture

Status: Needs work » Needs review
plach’s picture

Sorry for being vacant, peter. I'll give this another pass tonight.

das-peter’s picture

@plach No problem :)
I've fixed wrong conditions, let's see if this works better.

das-peter’s picture

Removed debugging artefacts.

das-peter’s picture

I moved storing the data properties into the postSave hook. At least the ugliness is centralized now :P
Further I added the test class NodePropertyMultilingualTestCase to test the multilingual properties. The tests were extended too.

fago’s picture

Gábor Hojtsy’s picture

Views just landed. I don't yet see what this means for us, but we are likely considerably set back and need more help to convert everything in views as well, which will result in a mega-mega-patch and definitely needs more eyeballs :/ Will send for a retest first and foremost.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-content

@dawehner offered to take a look tonight at the views integration for this issue. Thanks a lot in advance :)

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -89,6 +89,13 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+   * The table that stores properties, if the entity has multilingual support.
+   *
+   * @var string
+   */
+  protected $dataTable;

We should start calling them fields or base fields. See #1798140: Agree upon terminology for configurable fields. So even the table name 'node_property_data' should be updated then. Maybe go with 'node_field_data' ?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -510,6 +550,11 @@ public function save(EntityInterface $entity) {
+        if ($this->dataTable) {
+          $entity->default_langcode = $entity->langcode;
+          $entity->langcode = $langcode;
+        }

Hm, I don't like how the langcode is copied around here. $entity->langcode should always be its default langcode, everything else is confusing and wrong.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -550,6 +602,18 @@ public function save(EntityInterface $entity) {
+    // Ensure when saving properties in a new language a new revision is created.
+    // @todo try to find a nicer/unified way to do this.
+    if ($this->dataTable && !$entity->isNewRevision()) {
+      $query = db_select($this->revisionTable)
+        ->condition($this->idKey, $entity->id())
+        ->condition('langcode', $entity->langcode);
+      $query->addExpression('1');
+      if (!$query->execute()->fetchField()) {
+        $entity->setNewRevision();
+      }
+    }

So we auto-enable revisions here? Is that a technical requirement somehow? If not, I think it's something that should happen outside of the controller. What if revisions are disabled?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -593,11 +657,32 @@ protected function preSave(EntityInterface $entity) { }
-  protected function postSave(EntityInterface $entity, $update) { }
+  protected function postSave(EntityInterface $entity, $update) {

Shouldn't that loop over all available languages of the node?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -593,11 +657,32 @@ protected function preSave(EntityInterface $entity) { }
+      // @todo try to find a better way to deal with updates.
+      $query = db_select($this->entityInfo['data table'])

Could we just use db_merge() here? See #1774104: Replace all trivial drupal_write_record() calls with db_merge().

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -95,32 +94,19 @@ protected function invokeHook($hook, EntityInterface $node) {
+    // Make sure the default language is set.
+    if (empty($node->default_langcode)) {
+      $node->default_langcode = $node->langcode;
+    }

default_langcode is no valid property. It's langcode.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodePropertyMultilingualTestCase.php
@@ -0,0 +1,196 @@
+      foreach ($property_langcodes as $langcode) {
+        $property_count++;
+        $expected_languages[] = $langcode;
+        $title_values[$langcode] = $this->randomName(8);
+
+        // Save properties in a different language.
+        $node->title = $title_values[$langcode];
+        $node->langcode = $langcode;
+        $node->save();

Oh, looks like the API is used wrongly here. You have to save the values of *all* languages at once. Yep, without entityNG you cannot do this for title yet. So maybe we should do the entityNG conversion first??

dawehner’s picture

We had some discussion about skype. One main point was the decision between using EntityNG or not.
Our conclusion was that EntityNG would fit better for various reasons. In order to not require to change each $node->property access
we had the idea to enable the compability mode by default, so the magic getters/setters will automatically take care about it.
@fago
Sound this like a doable plan?

The reasons for this suggestion:

  • Keep the patch as small and reviewable as possible
  • Don't work on stuff which will have to be reverted in the future (change the default storage controller to support the other tables).
  • Profit from EntityNG in general

Once a route is set (for example the table names), the views parts is not a big issue in terms of complexity.

Gábor Hojtsy’s picture

Well, our earlier problem with that was that it required hundreds of Ks of unrelated changes, distracting from the problem on this issue. Is compatibility mode working around that, so the patch will be smaller instead of significantly bigger?

das-peter’s picture

I've to clarify this with fago, but as far as I understand the compatibility mode will help to keep the patch small.
I'm optimistic about that, but what I'm afraid of is that the compatibility mode could be a dead end somewhere else where it is about using the new multilingual capabilities.
I hope I'm able to clarify this for the D8MI meeting today.

fago’s picture

In order to not require to change each $node->property access
we had the idea to enable the compability mode by default, so the magic getters/setters will automatically take care about it.

omg, yes! Why did I not think of that!? ;-) This should help us doing conversion faster in general.

With compatibility mode on, you still can access the EntityNG stuff by using the getters; e.g.

$field_value = $entity->get('field')->value;
$field = $entity->get('field');
$field[$delta]->value;
$field_translated_value = $entity->getTranslation($langcode)->field->value;

Problem is only code that enables and disables compatibility mode right now, e.g. as needed for invoking field API attachers. Easiest fix would be to override setCompatibilityMode() to force keeping it enabled.

Next problem is, that with compatibility mode properties all look like field API fields look in D7 on the entity, i.e. $entity->field[langcode][0]['value']. One could override the magic getters/setters to forward everything that is not a field API field directly to the value.

dawehner’s picture

FileSize
10 KB

I just started with working on updating the views integration, so just posting an interdiff to the patch in #77.
More work will come tomorrow.

dawehner’s picture

Status: Needs work » Needs review
FileSize
147.13 KB
49.81 KB

This should fix a lot of the failing tests now, i runned the most obvious ones locally.

das-peter’s picture

Status: Needs review » Needs work

As I'm not able to continue the work over the weekend I've posted what I've done related to #86 so far here: #1818556: Convert nodes to the new Entity Field API

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.41 KB
156.59 KB

Things done by that patch:

  • Update the hook_update_N in the .install file
  • Fix a bunch of views tests/views integrations

Let's see how many tests are still broken.

dawehner’s picture

Issue tags: +VDC-integration

Add tag

dawehner’s picture

FileSize
1.16 KB
171.94 KB

Fixed one test and lost hours in trying to debug the other one.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +feature freeze

Add feature freeze tag.

Schnitzel’s picture

Status: Needs work » Needs review
Issue tags: -feature freeze
FileSize
148.75 KB

did a reroll of the patch in #94. Also found an issue where:
core/modules/views/views_ui/lib/Drupal/views_ui/ViewAddFormController.php
core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.php
core/modules/views/views_ui/lib/Drupal/views_ui/ViewPreviewFormController.php

are deleted in patch #94, which probably was a mistake ;)

let's see what happens now with the tests

Schnitzel’s picture

Issue tags: +feature freeze

to stay tag has to be

Carsten Müller’s picture

FileSize
148.75 KB

just an error with the hook_update_n() numbers

fixed and requeued for testing

Schnitzel’s picture

found some issues with the whole new plugin system.
fixxed them, let's see what the testbot means.

Schnitzel’s picture

fixxed two of the failed tests

Schnitzel’s picture

Status: Needs review » Needs work
FileSize
149.89 KB

mhh these two fails should not happen, well at least they don't happen on my local.

let's make some ugly debugging.

Schnitzel’s picture

Status: Needs work » Needs review
Schnitzel’s picture

FileSize
150.8 KB

more debugging

Schnitzel’s picture

Berdir’s picture

Assigned: Unassigned » Berdir
Issue tags: +D8MI, +sprint, +language-content, +feature freeze, +VDC-integration

I'll work on a re-roll and have a look at those test faliures.

Berdir’s picture

Ok. Fixed two of the three failing test classes.

- NodeAdmin list is doing a sort on a list of nodes, but all of them have the same changed date. So it actually depends on the fact that mysql is hopefully going to return them in the correct order. The problem is that the distinct() messes up that default order. Fixed that by adding an explicit secondary sort order on the node id. We should probably actually directly update the table to have some useful changed dates that do *not* match the nid's.
- The settings tests did not pass anymore because it was looking for node.status but this was now called node_property_data.status.
- The FieldEntityTest is failing because we want a join like this: comment -> node -> node_property_data -> user but user is joined on node, probably because we specify the relationship explicitly? That is one for dereine to figure out :)

dawehner’s picture

Status: Needs review » Needs work

Even if it is not trivial to see, but this is fixed by the bug in #1822384: Don't replace $this->extra on the JoinPlugin with adjusted..

Schnitzel’s picture

Status: Needs work » Needs review

#114: node-property-1498674-115.patch queued for re-testing.

Gábor Hojtsy’s picture

Priority: Major » Critical
Schnitzel’s picture

something is strange with the Testbot, reupload to force a new test.

Schnitzel’s picture

mhh NodeAdministration Test is still not fixed, trying new approach with using innerJoin, so no Distinct anymore.

Schnitzel’s picture

YAY! So here the patch withouth debug() (which I needed in case Testbot again does not like it =) )

plach’s picture

#122: node-property-1498674-122.patch queued for re-testing.

das-peter’s picture

Assigned: Berdir » das-peter
Priority: Critical » Major

Even if the tests pass, this still needs work. I introduced several bugs and built up code on wrong assumptions.
So I take over here as I now know where I've to do the cleanup.

das-peter’s picture

Priority: Major » Critical
Status: Needs review » Needs work
Issue tags: -sprint, -language-content, -feature freeze, -VDC-integration

Fix tagging

Fabianx’s picture

Dear Testbot, it is not okay to remove tags ...

das-peter’s picture

Current state from my perspective - IRC-Log:

[16:48] <das-peter> GaborHojtsy: I nag berdir from time to time already. But my current perspective is that node multilingual properties, without EntityNG (at least with the Compatibility Decorator), is a dead end.
[16:50] <das-peter> GaborHojtsy: To have multilingual properties I need to store the properties as array which breaks the compatibility of $node->title. And to use the refactored node entity you've to a) insert magic methods (entityNG) or b) rewrite all locations of $node->title to $node->get('title')
[16:51] <das-peter> GaborHojtsy: b) is what I did weeks ago - with the result that the patch was ~ 400kb . And to build a) on my own is silly because that's exactly the goal of the entityNG Decorator.
[16:52] <YesCT> wow.
[16:52] <GaborHojtsy> das-peter: Jesus
[16:52] <das-peter> GaborHojtsy: My current conclusion is: Stop wasting time with multilingual properties and try to push the entityNG Decorator instead.

My personal conclusion is that we should focus on #1818556: Convert nodes to the new Entity Field API - especially the decorator part. This will allow us to base the multilingual property code on a shared code base and we don't have to re-invent the wheel. Further this will help the entity to entityNG conversion in general.

Fabianx’s picture

Issue tags: +Twig

#129: That is fantastic. I really like the decorator part and especially for theming with Twig we would also love if all entities were available via easy getter / setter functions:

In Twig btw. {{ node.title }} will actually try $node->get('title'), so at least within twig templates it is compatible. Other code has been changed to use $node->label() anyway, so I don't see compatibility of $node->title as a big issue.

Huge +1 in whatever direction this goes to allowing something like {{ node.field_name.0 }} for getting the content of a field instead of {{ node.field_name.und.0.safe_value }}.

This is one of the missing building blocks within our cleanups.

Tentatively marking with Twig tag, so I can find this later ...

das-peter’s picture

Gábor Hojtsy’s picture

FileSize
31.66 KB

@das-peter created this dependency drawing of the related issues:

image003.jpeg

Issues linked are:

#1778178: Convert comments to the new Entity Field API
#1831444: EntityNG: Support for revisions for entity save and delete operations
#1833334: EntityNG: integrate a dynamic property data table handling
#1818556: Convert nodes to the new Entity Field API
#1498674: Refactor node properties to multilingual

Given that #1831444: EntityNG: Support for revisions for entity save and delete operations is RTBC for a while, that looks like still some steps before we get back here. I don't think this will be done before December 1st, however, it flips over to a regression if/once we remove the content translation module, so it will need to get done either way. It is out of question IMHO that it needs to be done in Drupal 8. (As well as other types like comments, taxonomy, etc. to be converted).

Gábor Hojtsy’s picture

Status: Needs work » Postponed
Issue tags: -sprint

Remove from sprint, marked postponed.

plach’s picture

It is out of question IMHO that it needs to be done in Drupal 8. (As well as other types like comments, taxonomy, etc. to be converted).

Yes, they should have a fairly balanced +/- ratio :)

Also, this is categorized as critical task, so I guess there is no risk to see a patch rejected if it is RTBC after feature freeze.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary to highlight the issues this is postponed on in the remaining tasks, and link to the drawing of the issue dependencies.

Gábor Hojtsy’s picture

plach’s picture

Yep, I think the plan we agreed upon is doing the minimal needed to switch to NG, i.e just make the storage controller return BC-decorated entities. I hope it won't be as hard as the comment issue. Having the storage controller switched to NG will make the data table available for us to exploit here :)

I'll start to work on the NG conversion tonight.

YesCT’s picture

Issue tags: +Usability, +budapest2012

plach, you are amazing.

this was mentioned as a big problem (we knew that :) ) in response to the user testing. tagging.
Budapest Usability Testing Results
http://groups.drupal.org/node/271918

plach’s picture

Issue tags: +Post NG clean-up
plach’s picture

Status: Postponed » Active
Issue tags: -Post NG clean-up

We can start again with this.

plach’s picture

Issue summary: View changes

Updated issue summary take out repeat (reference to this issue itself)

plach’s picture

I'm going to start working on this as soon as I've provided an inital stab for #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable, since that one is blocking the node type to configurable conversion. Hopefully tonight or tomorrow.

podarok’s picture

Issue tags: +sprint

due to #141 updated summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

@podarok:

This is not actually blocked on that one, simply I ain't sure I'll be able to write both an initial patch for the other one and start working on this tonight. Reverted the issue summary change.

plach’s picture

Assigned: das-peter » plach
Status: Active » Needs review
FileSize
19.09 KB

Here is a first stab: it allows to save/load a node. I think we should definitely revisit the NG storage controller since it's not working exactly as I would expect wrt the data/revision tables handling. I'll post a proof of concept tomorrow with more details on this and I'll see if I can merge in the latest patches.

plach’s picture

Here is a new attempt still working on save/load. This heavily refactors entity loading to take into account base/mul/rev/mulrev scenarios. In the latter case, that is when we have both the data and the revision table, the latter holds a record for each revision in each available language, hence we should not perform a join with the base table, as we might get multiple records for each entity. Moreover in the new standard the revision table fully replicates the data table schema with the addition of revision-specific metadata (the log column) that should be loaded anyway, thus we don't need to query the data table in this case.

The attached patch keeps the base + revision join for monolingual entities. In multilingual scenarios it attaches property data from either the revision table or the data table depending on the entity revisability.

There is also a bit of futzing with the isDefaultRevision property which has not been converted to a field yet.

Overall this does not look very clean to me. I think we should provide different storage helper classes for the various base/mul/rev/mulrev scenarios in #1497374: Switch from Field-based storage to Entity-based storage.

Gábor Hojtsy’s picture

plach’s picture

Ok, this one should fix the node admin page. Let's start testing this (not that I expect this to come out green ;).

plach’s picture

Fixed comment installation.

plach’s picture

The next step will be merging the latest pre-NG patch (#122), if possible.

plach’s picture

In #147 I moved the node admin code from DBTNG to EFQ2 without big efforts. I'm wondeing whether it would make sense doing it for every failing query and avoiding the double step of first adapting the SQL queries to the new schema and then porting everything to EFQ2 in a follow-up of #1497374: Switch from Field-based storage to Entity-based storage.

amateescu’s picture

EFQ2 was specifically designed to be an easy transition from DBTNG, so I'd say yes, switch to it.

plach’s picture

I know that ease in transition was one of the design goals, the main reason of my doubt is benchmarking: switching to the new schema is likely to introduce a performance regression, moving to EFQ2 too. But they are two (semi) unrelated changes, hence it might be useful to be able to quantify the perfomance penalty each one implies.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

FileSize
126.03 KB

Here is an attempt to (manually) merge #122. Hopefully we'll have less failures.

Anonymous’s picture

Issue summary: View changes

remaining tasks to clarify that the dependencies are done

plach’s picture

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-157.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
132.84 KB

Fixed a leftover with revision handling in the node storage controller. Let's see how many failures were caused by this (we should have at very least one less failing test). Still at work to fixing more tests.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-159.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
133.48 KB

More fixes :)

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-161.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
137.29 KB

This one provides a (supposedly) working upgrade path plus additonal test fixes.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-163.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
140.42 KB

Fixed NG storage controller revision handling plus additional tests.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-165.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
145.49 KB

Fixed more tests.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-165.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
164.94 KB

Fixed some node_revision occurences.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-167.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
163.97 KB

This might break a few more things, but contains a necessary refactoring to revision handling plus a couple of test fixes. Let's see how it performs.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-171.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
171.45 KB

More test fixes.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-172.patch, failed testing.

dsnopek’s picture

I have big plans to test this and do some code review before the next D8MI meeting... I'm super excited about this patch! :-)

plach’s picture

Status: Needs work » Needs review
FileSize
171.45 KB

Fixed tracker tests. The remaining failures are due to the current messy handling of data and revision tables together, which we are real-life testing for the first time with nodes. The attached patch fixes revision translation handling. Not sure whether more things will break. Let's see.

plach’s picture

FileSize
175.64 KB

Wrong patch...

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-176.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
177.75 KB

This should fix most failures. Still fighting against the last ones.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-179.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.6 KB
177.15 KB

This is just a little clean-up and some questions. Patch attached for these small nits. I'll do another read through after posting it.

+++ b/core/includes/schema.incundefined
@@ -522,5 +510,33 @@ function drupal_write_record($table, &$record, $primary_keys = array()) {
+ * Type cast to proper datatype.
+ *
+ * MySQL PDO silently casts e.g. FALSE and '' to 0 when inserting the value
+ * into an integer column, but PostgreSQL PDO does not. Also type cast NULL
+ * when the column does not allow this.
+ *
+ * @param array $info
+ *   An array describing the schema field info.
...
+function drupal_schema_get_field_value($info, $value) {

Typecasts...

I think the for example phrase is in commas, and the line wrapping could be tighter.

and the type hinting can be done for array $info.

I dont understand "Also type cast NULL
+ * when the column does not allow this." This function does not appear to do anything special for NULL.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -220,39 +270,58 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+          // Get revision ID's.

I think this is plural and not possessive. (I did a grep to compare to what is usual in core, and it appears that IDs is almost always used.)

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -220,39 +270,58 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+          // Unless a revision id was specified we are dealing with the default
+          // revision.

here id is lowercase. Let us be consistant, at least within this patch, either id or ID. Some of the code was moved... and had "id" previously. maybe I shouldn't bother with it.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -353,32 +422,52 @@ public function save(EntityInterface $entity) {
+      // When saving a new revision, set any existing revision ID to NULL so as to
+      // ensure that a new revision will actually be created.

wrap to 80 chars.

+++ b/core/modules/comment/comment.installundefined
@@ -37,7 +37,8 @@ function comment_uninstall() {
+  // TODO Add support for multilingual properties.

@todo http://drupal.org/node/1354#todo

Wait.. isn't that what this issue is doing?
Maybe this needs a follow-up that can be linked.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
@@ -84,11 +84,19 @@ public function setUp() {
+      // Ensure comments are sorted in ascending order.
+      $time = REQUEST_TIME + ($this->masterDisplayResults - $i);
+      $comment->created->value = $time;
+      $comment->changed->value = $time;
+
       comment_save($comment);
     }
 
     // Store all the nodes just created to access their properties on the tests.
     $this->commentsCreated = entity_load_multiple('comment');
+
+    // Sort created comments in ascending order.
+    ksort($this->commentsCreated, SORT_NUMERIC);

seems strange to sort comments, save them, then.. get the comments created and sort them again. This is in a test setUp(). Maybe this is giving the comments created and changed time because otherwise, the test does not differentiate the time enough.

Maybe the first part is storing information that is used later to let them be sorted. "ensure"

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -129,6 +114,16 @@ protected function invokeHook($hook, EntityInterface $node) {
+    // @todo Remove this once comment is a regular entity field.

maybe this has an issue?
was it #1778178: Convert comments to the new Entity Field API and is actually already done?

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -142,30 +137,23 @@ protected function preSave(EntityInterface $node) {
+      // @todo: Make the {node_property_revision}.log column nullable so that we
+      //   can remove this check.

just @todo, not @todo:

+++ b/core/modules/node/node.installundefined
@@ -47,6 +47,69 @@ function node_schema() {
+      // @todo Remove the following columns when removing the legacy Content
+      //   Translation module.

http://drupal.org/node/1952062

+++ b/core/modules/node/node.installundefined
@@ -62,236 +125,252 @@ function node_schema() {
+  // Node property revision storage.
...
 
-  $schema['node_revision'] = array(
-    'description' => 'Stores information about each saved version of a {node}.',
+  $schema['node_access'] = array(
+    'description' => 'Identifies which realm/grant pairs a user must possess in order to view, update, or delete specific nodes.',

Store the node property revision. (It should be a sentence...)

Check this in context, maybe just delete comment.

The next similar section does not have such a comment. I just removed it.

+++ b/core/modules/node/node.moduleundefined
@@ -1465,11 +1467,13 @@ function node_ranking() {
+  // TODO Consider multilingual properties.

todo format again.
also, again, isn't that what this issue is doing. Maybe it's just not done yet, otherwise will need a follow-up issue.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -274,10 +274,10 @@ protected function drupalCreateNode(array $settings = array()) {
-    db_update('node_revision')
-      ->fields(array('uid' => $node->uid))
-      ->condition('vid', $node->vid)
-      ->execute();
+//     db_update('node_revision')
+//       ->fields(array('uid' => $node->uid))
+//       ->condition('vid', $node->vid)
+//       ->execute();

looks like you are still thinking about this. Maybe how to make it pass a test?

Perhaps a @todo here, or these can just be removed?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/join/JoinPluginBase.phpundefined
@@ -157,6 +157,7 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
     $this->leftTable = $configuration['left_table'];
     $this->leftField = $configuration['left_field'];
+
     $this->field = $configuration['field'];

unrelated change.

Also, what is a "translation set id" I think it's just translation ID really?

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-181.patch, failed testing.

Gábor Hojtsy’s picture

Translation set id is from the old translation module in core. It should be removed with the migration.

Gábor Hojtsy’s picture

Issue summary: View changes

added section for issues blocked by this one. there might be more to add.

YesCT’s picture

updated issue summary. I'm not 100% sure if this actually still needs a follow-up as mentioned in #60 for upgrade tests.

@Gábor Hojtsy Ah, you mean #1952062: Remove legacy translation module in favor of content translation will take care of that, so we dont have to here?

YesCT’s picture

Status: Needs work » Needs review
FileSize
177.14 KB

oops. I forgot to do a git pull when I made the patch. This should be better (it's the same interdiff as 181, just a better patch that should apply.)

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-185.patch, failed testing.

YesCT’s picture

A single node has been unpublished. Other BulkFormTest.php 76 Drupal\action\Tests\BulkFormTest->testBulkForm()
failed in #179 but passes for me locally.

--

While trying to track down the exceptions from the image sync test, I wanted to look up the allowed indexes to the $values array used to create an Entity.
http://api.drupal.org/api/drupal/core%21modules%21translation_entity%21l...
says: "array $values: An array of initial values for the entity."
But does not tell me where I can find out what that array can hold.
How can I look up that info?
Ah, it probably depends on the EntityType
How can I see what $this->entityType is?
Is it just a TestEntity?

EntityTestTranslationUITest
uses

      'name' => $this->randomName(),
      'user_id' => mt_rand(1, 128),

also, but there is an undefined index in that test fail also..

Locally, I changed 'name' to 'title' and 'user_id' to 'uid' and that helped with the exceptions, but I dont see where that might be documented...

[edit:]
http://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modul...
has name and user_id

plach’s picture

Status: Needs work » Needs review
FileSize
178.3 KB
3.45 KB

(Hopefully) fixed a couple of failures and incorporated #185, except TODOs fixes: they need to be resolved before commit, I intentionally used a non-standard format to find them later. Too late for more answers now :)

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-188.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
178.34 KB
1.31 KB

This should (hopefully!) fix that ton of new failures (silly me) and leave just a couple of lingering ones, which I'll try to fix asap.

@YesCT:

I dont understand "Also type cast NULL
+ * when the column does not allow this." This function does not appear to do anything special for NULL.

Honestly I didn't even read that docs, I just copypasted them from above while factoring out those lines in a standalone function :) However I think the doc is referring to MySQL/PgSQL PDO drivers. This function is filling the gaps between the two.

I think this is plural and not possessive.

Yep

here id is lowercase. Let us be consistant, at least within this patch, either id or ID. Some of the code was moved... and had "id" previously. maybe I shouldn't bother with it.

Agreed. I think we should do what core does most often for every added line in the patch.

seems strange to sort comments, save them, then.. get the comments created and sort them again. This is in a test setUp(). Maybe this is giving the comments created and changed time because otherwise, the test does not differentiate the time enough.

Yep, all comments share the same creation/modification date (REQUEST_TIME) so we need to ensure they are sorted as intended (by id) as the added join seems to alter the natural order.

maybe this has an issue?

Probably #1939994: Complete conversion of nodes to the new Entity Field API.

looks like you are still thinking about this. Maybe how to make it pass a test?

I think the changes introduced in the storage controller should make this unnecessary but I ain't sure. The patch is not any close to being complete so I didn't bother to remove those lines. Overall I think it's not nitpick time yet as many lines of this patch could still change before it's ready :)

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-190.patch, failed testing.

Gábor Hojtsy’s picture

Tagging for D8MI.

plach’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

[wrong patch]

plach’s picture

FileSize
178.14 KB

This is going to fix one test but break others. Just a temporay upload for the bot as I don't remember what the lines I just removed were supposed to fix.

plach’s picture

Fixed the other failure meanwhile.

plach’s picture

The last submitted patch, et-node-ml-1498674-196.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#196: et-node-ml-1498674-196.patch queued for re-testing.

The last submitted patch, et-node-ml-1498674-196.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Obviously such an issue could not come without a test failing only online :(

plach’s picture

An attempt to fix the failing test + some debug lines.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-201.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
179.25 KB
2 KB

This removes the debug lines and should fix the entity_query() leftovers. Any remaining failure should just be caused by some new commit.

Interdiff is with #196.

The last submitted patch, et-node-ml-1498674-203.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#203: et-node-ml-1498674-203.patch queued for re-testing.

The last submitted patch, et-node-ml-1498674-203.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
180.44 KB
1.37 KB

This should hopefully fix the last failure.

Berdir’s picture

Haven't found much to complain...

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -220,43 +270,67 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+
+        if ($this->revisionTable) {
+          // Unless a revision ID was specified we are dealing with the default
+          // revision.
+          $entities[$id]->getBCEntity()->isDefaultRevision = intval(empty($revision_id));

$entity->isDefaultRevision($new_value) should work fine...

+++ b/core/modules/book/book.moduleundefined
@@ -289,21 +289,23 @@ function book_get_books() {
       $query = db_select('book', 'b', array('fetch' => PDO::FETCH_ASSOC));
-      $query->join('node', 'n', 'b.nid = n.nid');
+      $query->join('node', 'nb', 'b.nid = nb.nid');
+      $query->join('node_property_data', 'n', 'nb.nid = n.nid');
       $query->join('menu_links', 'ml', 'b.mlid = ml.mlid');
-      $query->addField('n', 'type', 'type');
-      $query->addField('n', 'title', 'title');
+      $query->addField('nb', 'type', 'type');
       $query->fields('b');
       $query->fields('ml');
-      $query->condition('n.nid', $nids, 'IN');
+      $query->condition('nb.nid', $nids, 'IN');
       $query->condition('n.status', 1);
       $query->orderBy('ml.weight');
       $query->orderBy('ml.link_title');
       $query->addTag('node_access');
       $result2 = $query->execute();
       foreach ($result2 as $link) {
+        $node = node_load($link['nid']);
         $link['href'] = $link['link_path'];
         $link['options'] = unserialize($link['options']);
+        $link['title'] = $node->label();
         $all_books[$link['bid']] = $link;

That's.... ugly :(

Maybe just do a node_load_multiple($nids) and then remove the node joins completely and just rely on the loaded node?

+++ b/core/modules/comment/comment.admin.incundefined
@@ -82,11 +82,10 @@ function comment_admin_overview($form, &$form_state, $arg) {
-  $query->join('node', 'n', 'n.nid = c.nid');
-  $query->addField('n', 'title', 'node_title');
+  $query->join('node_property_data', 'n', 'n.nid = c.nid');
   $query->addTag('node_access');
   $result = $query
-    ->fields('c', array('cid', 'subject', 'name', 'changed'))
+    ->fields('c', array('cid', 'nid', 'subject', 'name', 'changed'))
     ->condition('c.status', $status)
     ->limit(50)
     ->orderByHeader($header)
@@ -97,8 +96,9 @@ function comment_admin_overview($form, &$form_state, $arg) {

@@ -97,8 +96,9 @@ function comment_admin_overview($form, &$form_state, $arg) {
   // We collect a sorted list of node_titles during the query to attach to the
   // comments later.
   foreach ($result as $row) {
+    $node = node_load($row->nid);
     $cids[] = $row->cid;
-    $node_titles[] = $row->node_title;
+    $node_titles[] = $node->label();

This is overlapping with the comment as field issue :(

+++ b/core/modules/node/node.installundefined
@@ -752,6 +830,66 @@ function node_update_8015() {
+    $indexes = array('node_changed', 'node_created', 'node_frontpage', 'node_status_type', 'node_title_type', 'uid');
+    foreach ($indexes as $index) {
+      db_drop_index('node', $index);
+    }
+    $fields = array('title', 'uid', 'status', 'created', 'changed', 'comment', 'promote', 'sticky');
+    foreach ($fields as $field) {
+      db_drop_field('node', $field);

This is a bit tricky, as we decided to not remove tables during the upgrade path. Not sure how to deal with this as we're removing a lot here.

One idea would be to rename node to _d7_node first and then create a new node table. No idea if this is necessary :)

+++ b/core/modules/search/search.api.phpundefined
@@ -205,17 +205,17 @@ function hook_search_execute($keys = NULL, $conditions = NULL) {
-  $query->join('node', 'n', 'n.nid = i.sid');
+  $query->join('example', 'e', 'e.id = i.sid');
   $query
-    ->condition('n.status', 1)
-    ->addTag('node_access')
-    ->searchExpression($keys, 'node');
+    ->condition('e.status', 1)
+    ->addTag('example_access')
+    ->searchExpression($keys, 'example');
 
   // Insert special keywords.
-  $query->setOption('type', 'n.type');
-  $query->setOption('langcode', 'n.langcode');
+  $query->setOption('type', 'e.type');
+  $query->setOption('langcode', 'e.langcode');
   if ($query->setOption('term', 'ti.tid')) {
-    $query->join('taxonomy_index', 'ti', 'n.nid = ti.nid');
+    $query->join('taxonomy_index', 'ti', 'e.id = ti.id');

Changing this to example seems unecessary given that we keep the original and just change a few things?

+++ b/core/modules/search/search.api.phpundefined
@@ -232,29 +232,29 @@ function hook_search_execute($keys = NULL, $conditions = NULL) {
-    $uri = $node->uri();
+    $uri = $example->uri();
     $results[] = array(
       'link' => url($uri['path'], array_merge($uri['options'], array('absolute' => TRUE, 'language' => $language))),
-      'type' => check_plain(node_get_type_label($node)),
-      'title' => $node->label($item->langcode),
-      'user' => theme('username', array('account' => $node)),
-      'date' => $node->get('changed', $item->langcode),
-      'node' => $node,
+      'type' => check_plain(node_get_type_label($example)),

Especially because it starts to get weird here ;) Why would example use node types :p

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -274,10 +274,10 @@ protected function drupalCreateNode(array $settings = array()) {
     // Small hack to link revisions to our test user.
-    db_update('node_revision')
-      ->fields(array('uid' => $node->uid))
-      ->condition('vid', $node->vid)
-      ->execute();
+//     db_update('node_revision')
+//       ->fields(array('uid' => $node->uid))
+//       ->condition('vid', $node->vid)
+//       ->execute();

I guess this should either be fixed or removed if it's not necessary?

Berdir’s picture

I have an 8.x database with 600k real nodes, I'll try the upgrade path there.

Do we need additional upgrade path tests?

plach’s picture

That's.... ugly :( Maybe just do a node_load_multiple($nids) and then remove the node joins completely and just rely on the loaded node?

This should go away as soon as we switch to efq2: can we just do the minimal work needed to keep things working?

This is a bit tricky, as we decided to not remove tables during the upgrade path. Not sure how to deal with this as we're removing a lot here.

That code is only moving around columns from node to node_property_data: no data is actually altered/removed, AAMOF node_revision is retained.

I have an 8.x database with 600k real nodes, I'll try the upgrade path there.

Sounds cool :)

Do we need additional upgrade path tests?

I don't think so: tests were failing until I wrote the update function, obviously unless your tests reveal some bug in the upgrade. One issue with the current code is it's not updating foreign keys definitions, I forgot about that.

Berdir’s picture

Hm, the book query won't work with EFQ unless we make a book an entity or a field on entities. Haven't seen anyone trying to do that :) Given that it's book module, there's a likely chance that it won't get changed unless it breaks tests, so I'd like to have something in that's partially sane :)

Shouldn't be hard to convert, just remove the node table joins, change the nids condition to use the book table directly and then do something like $nodes[$nid]->label() to get the node-related stuff.

plach’s picture

so I'd like to have something in that's partially sane :)

Yeah, makes sense, I was taking for granted that it would be converted to something more D8-like ;)

plach’s picture

This should address #207.

$entity->isDefaultRevision($new_value) should work fine...

Hm, we should really split this into a setter and a getter. I didn't notice this could be used also as a setter...

Shouldn't be hard to convert, just remove the node table joins, change the nids condition to use the book table directly and then do something like $nodes[$nid]->label() to get the node-related stuff.

It was a bit trickier than that: the body of the function was almost replicated in the BookManager (which apparently has no test coverage). Moreover by removing the joins we had no status to filter on: I think we can just check the node status while iterating on the result since unpublished books are not supposed to be a huge number, however we have no indication of which language use while checking $node->status and retrieving $node->label(). I think this exemplifies well one of the remaining tasks of this issue: we need to figure out how to deal with multilingual scenarios for each of the updated queries. As a side issue we need to understand how to update node's views integration (hope @dawehner can help on this): as soon as we will start translating nodes, we will have a row for each translation in the result for every view sorting on title or created. I think it would be sensible to have an implicit filter selecting only rows from the data table with default_langcode = 1, if no explicit language filter is defined. This would mimick the current node access "fallback" behavior, and would look as a sensible default behavior to me.

Moreover we should consider the possibility of making the data table the primary table when querying, as most of the time this will allow us to exploit indexes when sorting. Not sure whether this is feasible without messing too much with Views. More generally If we are able to use the data table as primary table, the performance hit will be limited to the need of joining on the base table to filter the result by node type. AAMOF the only indexes that got lost in the conversion are the 'node_status_type' and 'node_title_type' ones. Not sure whether it would make sense to move the type column in the data table to allow for more performant queries. I guess this logic would be a bit tricky to automate when moving to the entity storage, but probably it could be addressed together with indexes which are going to be tricky anyway.

plach’s picture

This should handle node access better in the book manager.

Gábor Hojtsy’s picture

I admit I don't have a good overview of what would be required to fully support say multilingual node titles on the UI / as part of the translation workflow. Eg. what other issues do we need to resolve for these properties to show up in the content translation config page in Drupal 8 and then for the node forms to respect multilingual properties? Do we have a list of things so we have a better overview. Especially interested due to the fact we want to do this for taxonomy terms and menu titles at least. (At least those have less properties, no author or creation info or status flag to deal with). Thanks!

plach’s picture

I spent the last couple of days playing with ET and nodes: basic translation more or less works, but there is lot of stuff still breaking due to the current mess with partial node NG conversion, BC decorator quirks and NG storage controller fragility (we definitely need separate storage helpers for base/rev/mul/mulrev scenarios). However the main issue is with the current Entity Translation API that simply does not work both functionally and DX-wise: there are many places where we'd need an EntityTranslation object for things to work properly but we can't pass it because only objects implementing EntityInterface are accepted.

The attached patch + interdiff can be considered a proof of concept to show that this stuff is going to work properly once all the missing pieces are in place. We are simply not there yet.

Given this, I changed my mind about how we should proceed with this issue: since multilingual property storage is actually working, I think we should fix queries to account for multilingual properties and get this in ASAP. Then we would need to get the following stuff done:

  1. #1810370: Entity Translation API improvements
  2. Make property translatability configurable in ET
  3. #1939994: Complete conversion of nodes to the new Entity Field API (optional, but probably needed to have everything working the right way)

I guess properly taking care of multingual properties in Views can be done while improving integration with EFQ2, this would have the advantage of dealing with all entity types just once. The conversion of node queries to EFQ2 can be done as a follow-up of #1497374: Switch from Field-based storage to Entity-based storage.

@Gabor:

I admit I don't have a good overview of what would be required to fully support say multilingual node titles on the UI / as part of the translation workflow. Eg. what other issues do we need to resolve for these properties to show up in the content translation config page in Drupal 8 and then for the node forms to respect multilingual properties?

This should not be much hard to do (it's the second bullet above), you can see a very raw version in the attached interdiff (the full patch can be applied to manually test this). Obviously we need to make it configurable, but it should not be a great deal. I think I can post a patch in the next few days.

plach’s picture

Especially interested due to the fact we want to do this for taxonomy terms and menu titles at least.

If we focus on #1497374: Switch from Field-based storage to Entity-based storage, then every content entity type out there will natively support data tables. And I'd like to explore the possibility to switch between the various base/mul/rev/mulrev modes if there is enough time.

fago’s picture

+  // Node property storage.
+  $schema['node_property_data'] = array(
+    'description' => 'Base table for node properties.',

We call all entity properties fields now, so I guess we should use this terminology here as well?

dawehner’s picture

Issue tags: +Needs manual testing

I went through the views related parts and it looks pretty solid so far. For sure this patch also needs manual testing.

+++ b/core/modules/views/config/views.view.tracker.ymlundefined
@@ -61,14 +61,6 @@ display:
-        timestamp:
-          id: timestamp
-          table: history
-          field: timestamp
-          label: ''
-          link_to_node: '0'
-          comments: '1'
-          plugin_id: node_history_user_timestamp

Is there a reason why this piece of configuration is removed?

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/ItemsPerPageTest.phpundefined
@@ -45,7 +45,7 @@ function testItemsPerPage() {
-    $view['show[sort]'] = 'created:DESC';
+    $view['show[sort]'] = 'node_property_data-created:DESC';

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/SortingTest.phpundefined
@@ -35,7 +35,7 @@ function testSorting() {
-    $view1['show[sort]'] = 'created:ASC';
+    $view1['show[sort]'] = 'node_property_data-created:ASC';

@@ -60,7 +60,7 @@ function testSorting() {
-    $view2['show[sort]'] = 'created:DESC';
+    $view2['show[sort]'] = 'node_property_data-created:DESC';

This lines sort of look wrong, but I didn't researched that.

plach’s picture

Here is a reroll.

The attached patch should address #218 (we'll have to update the change notice with the new table names) and #219, except for the last remark: after manually testing the wizard it seems the current code is correct. Let's see if there is any issue with the first remark.

A note: the interdiff does not include #218 as it was pretty meaningless.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-220.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
181.58 KB
1.04 KB

This should fix the failure in #220.

plach’s picture

FileSize
180.55 KB

Rerolled after the latest commits.

plach’s picture

This is replicating the type column in the node_field_data table to restore the lost indexes, as proposed in #213 and approved by @catch.

I'm expecting some failure in views tests but identifying all of them would require too much time on my box.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-224.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
188.53 KB
1.11 KB

This should pass tests again. We should be missing only a final pass to the ported queries to ensure they will behave correctly when dealing with multilingual nodes.

plach’s picture

I started reviewing all the updated queries, ensuring that explained queries are not worst than the original ones, wherever possibile. Meanwhile I'm performing some microbenchmarks and I'm collecting some interesting numbers. I'll post them when I'm done. The attached patch covers book, comment and forum queries. As you can see from the interdiff in most cases I'm just enforcing the default language as language condition. The main reason behind this is that the proper logic depends on use cases. I think the conversion to EFQ2 should let us provide automatic language conditions when none is explicitly defined. We could start doing this right know but it would mean doing it for each query in a possibly unreliable way, whereas after the conversion we would could implement this logic in a single place and more reliably. The current conditions help with benchmarking, given that they implement one of the possible use cases, and that explict language conditions seem to perform more or less the same.

plach’s picture

Other query updates + reverted conversion to EFQ2 for the node admin page.

plach’s picture

Updated/adjusted the node module queries. One last pass (hopefully) tomorrow and we should be done here. There are some cases to discuss but I don't think they are real blockers.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-229.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
193.6 KB
7.24 KB

This should fix failing tests and the last queries. We should be ready for review now. Tomorrow I'll post some benchmarks and write an issue summary.

YesCT’s picture

FileSize
193.61 KB

#1862352: Move Views UI tests to Views UI module. moved core/modules/views_ui/lib/Drupal/views_ui/Tests/SettingsTest.php

reroll for that.

No other changes yet.
Some coming though.

YesCT’s picture

+++ b/core/includes/form.incundefined
@@ -4919,26 +4919,25 @@ function _form_set_attributes(&$element, $class = array()) {
+ * // More advanced example: multi-step operation - load all rows, five by five

since changing this line, let's make it a sentence.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -152,6 +153,55 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
+   * Overrides \Drupal\Core\Entity\DatabaseStorageController::buildQuery().

{@inheritdoc}
#1906474: [policy adopted] Stop documenting methods with "Overrides …" is fixed now.

Q1:

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -217,43 +267,67 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+          // Set only translatable properties, unless we are dealing with a
+          // revisable entity, in which case we did not load the untranslatable
+          // data before.

before?

how about:

+ // Set only translatable properties, unless we are dealing with a
+ // revisable entity, in which case we did not load the untranslatable
+ // data before, so set it now also.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -350,32 +424,53 @@ public function save(EntityInterface $entity) {
+    if (!$entity->isNewRevision()) {
+      // Delete and insert to handle added/removed values.
+      db_delete($this->revisionTable)

this looks like just delete. So:

// Delete to handle removed values.

+++ b/core/modules/node/node.installundefined
@@ -752,6 +837,69 @@ function node_update_8015() {
+    // TODO Handle foreign keys.

use the standard for @todo
http://drupal.org/node/1354#todo

+++ b/core/modules/node/node.moduleundefined
@@ -1838,12 +1842,17 @@ function node_page_title(EntityInterface $node) {
+ * @param $langcode
+ *   (optional) The language the node has been last modified in. Defaults to the
+ *   node language.
  *
  * @return
  *   A unix timestamp indicating the last time the node was changed.
  */
-function node_last_changed($nid) {
-  return db_query('SELECT changed FROM {node} WHERE nid = :nid', array(':nid' => $nid))->fetch()->changed;
+function node_last_changed($nid, $langcode = NULL) {
+  $language_clause = isset($langcode) ? 'langcode = :langcode' : 'default_langcode = 1';
+  $result = db_query('SELECT changed FROM {node_field_data} WHERE nid = :nid AND ' . $language_clause, array(':nid' => $nid, ':langcode' => $langcode))->fetch();
+  return is_object($result) ? $result->changed : FALSE;
 }

add type. string. in the new @param line.

since return line is new, add type to @return also.

plach’s picture

Thanks, Cathy. Anyway, those TODOs are meant to be discussed/resolved here :)

fago’s picture

Impressive work!

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -152,6 +153,55 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
+    $is_revision_query = $this->revisionKey && ($revision_id || !$this->dataTable);

Why do we need to query revisions if we do not have a datatable?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php
@@ -152,6 +153,55 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
+      $entity_revision_fields = drupal_map_assoc(drupal_schema_fields_sql($this->entityInfo['revision_table']));

The d7 controllers were optimized to not load the db schema into memory and therefore cached it into $this->entityInfo.

Thus I think we should either do the same or try to move to entity field definitions instead.

Would a "revisionable" (besides the "translatable") key help here? "revisionable" is right now missing and needs to be added anyway I think.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -387,7 +387,8 @@ public function updateOriginalValues() {
-          $this->values[$name][$langcode] = $field->getValue();
+          $value = $field->getValue();
+          $this->values[$name][$langcode] = is_array($value) ? array_filter($value) : $value;

We've got a fixes for that in other issues as welkl, see #1957888: Exception when a field is empty on programmatic creation

YesCT’s picture

FileSize
836.72 KB

@plach, yeah, I thought of that right after I posted the patch.
At least it will make grepping for the ones we need to find easier. *wink*
============

If I understand correctly, this patch by itself does not make the title (and other properties) translatable via the UI.
Is that right?

So, as a way to do a manual check, I did, both with and without the patch:

  1. install d8
  2. enable content translation (under extend, admin/modules)
  3. add a language (admin/config/regional/language)
  4. configure content language settings to enable translation on article (and it's fields) (admin/config/regional/content-language)
  5. create content of type article
  6. translate the article (enter different body)
  7. check the database

Should the timestamps be different for each language? (I was careful to wait a couple minutes between creation in en and adding the translation of af)

compare.png

=====
I think we should list the issues that are blocked by this, so as to add to the motivation to get this in.
Also, I think we should list what is blocking this commit.
@plach has already said in #231 that some benchmarks are coming. So performance evaluation is one thing blocking this that we already know about.
Also, fixing the @todo's that were added here will need to be fixed as part of this issue (taking into account the criteria in http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule)

Anything else? Let's list it in the summary so we stay on track.

Gábor Hojtsy’s picture

@YesCT: none of the properties (non-configurable fields) have a UI to mark as translatable, so they so far store the same value for each language. As per http://drupal.org/node/1498674#comment-7318886 above, there does not seem to be an issue to have a UI for this.

plach’s picture

Issue summary: View changes

updated issue summary after re-reading through old comments. added remaining task about upgrade tests and explaination for why so many node-> example changes are in this issue.

plach’s picture

FileSize
4.26 KB
4.26 KB
1.55 MB
193.86 KB

Updated the issue summary, sorry for taking so long, but I performed some additional benchmarks and tests. Attached you can find the ab logs and the two DBs (actually the dev one is the head one + the patch db update). I summarized the numbers in the OP.

Attached a reroll + revert of the TODO > @todo changes from #233.

Answers coming in the next comment.

plach’s picture

FileSize
1.58 MB

(attaching the other db dump here due to upload limits)

@fago:

Why do we need to query revisions if we do not have a datatable?

The current code in the database storage controller always joins on the revison table if the eneity is revisable when performing the initial query. When we have a data table the revision table may hold more than one record per entity hence in the latter case, to ensure we have just one one record for each entity, we just skip the join and pick data from the revion table while wattaching field values. This actually mimics field_attach_load().

The d7 controllers were optimized to not load the db schema into memory and therefore cached it into $this->entityInfo.
Thus I think we should either do the same or try to move to entity field definitions instead.
Would a "revisionable" (besides the "translatable") key help here? "revisionable" is right now missing and needs to be added anyway I think.

Makes sense, but perhaps this is not the ideal place to address these issues: we definitely need to provide separate SQL storage helpers for the base/mul/rev/mulrev scenarios (the current state is quite unmanageable). If we do this we might be able to skip all of this schema futzing or at least part of it.

We've got a fixes for that in other issues

Ok, but can we keep the fix here until the other issue is committed? Otherwise tests won't pass.

@YesCT:

Also, fixing the @todo's that were added here will need to be fixed as part of this issue (taking into account the criteria in http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule)

Thanks for pointing this out. However despite the directives outlined in Dries' article, I don't think we can fix those @todo here now. We can either postpone this again to #1810370: Entity Translation API improvements and #1497374: Switch from Field-based storage to Entity-based storage or just go ahead.

plach’s picture

FileSize
3.75 KB

Here's the module I used to microbenchmark multilingual queries. Results are in the code comments.

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

@Gabor:

+++ b/core/modules/node/node.module
@@ -2066,10 +2076,12 @@ function node_feed($nids = FALSE, $channel = array()) {
+      // TODO Should we filter this by language (more performant)?
+      ->groupBy('n.nid')

These issue is still on the table: how should we proceed with it? Basically I'm not sure how we should deal with node_feed(): do we want to filter the list by node language (multilingual list) or pick any node and rely on translation (translated list)?

Gábor Hojtsy’s picture

@plach: as a general rule of thumb, I would not build language filtering into baked in queries, like feeds. If people want per language feeds, they can create them with views? Anyone else sees this differently?

plach’s picture

Makes sense to me, here is a new patch.

plach’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Based on the ab tests, this looks like not necessarily a performance degradation? Eg. comparing the 3rd vs. 3rd results in the two tests.

plach’s picture

Assigned: Unassigned » plach

Well, in many case the explained query plan looks worse after the converison to multilingual. You can also check the microbenchmark results in #240: in most cases there actually is a performance degradation. However, as I wrote in the issue summary, I think we can make it affect only multilingual sistes if we get entity storage right.

plach’s picture

Assigned: plach » Unassigned
catch’s picture

@plach the performance numbers are a bit concerning, could you post some of the EXPLAIN results here?

catch’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

@catch:

I'll do ASAP. However do you think the outlined plan to limit the perfomance degradation to multilingual environments is viable?

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

FileSize
19.49 KB
13.4 KB

@catch:

Here are some explained queries. In the third sample the query plan is not very informative, being the same for all variants; however the actual performance is very different. In the other samples instead actual numbers reflect pretty closely the query plans.

Attached you can find also the full mb sessions results.

Berdir’s picture

The last submitted patch, et-node-ml-1498674-243.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
193.98 KB

Here's a reroll.

plach’s picture

FileSize
13.21 KB

Sorry, the last query in the fourth sample was wrong. Here is the correct version.

plach’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -387,7 +387,8 @@ public function updateOriginalValues() {
-          $this->values[$name][$langcode] = $field->getValue();
+          $value = $field->getValue();
+          $this->values[$name][$langcode] = is_array($value) ? array_filter($value) : $value;

We have various fixes for this problem now in different issues. I guess fago's in the default value issue is the most advanced, I'd suggest to extract that and fix the major bug (that we might want to make critical) first, so that this and the user ng conversion can go on.

+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined
@@ -57,24 +66,28 @@ public function getAllBooks() {
       $query->orderBy('ml.link_title');
-      $query->addTag('node_access');
       $book_links = $query->execute();

We still need node access here I think, you just need to specify the table which should be used: "->addMetaData('base_table', 'book')"

+++ b/core/modules/comment/comment.installundefined
index 6d0d003..93dbee8 100644
--- a/core/modules/comment/comment.module

--- a/core/modules/comment/comment.module
+++ b/core/modules/comment/comment.moduleundefined

Changes in comment.module are going to conflict a lot with the comment field issue. Which would likely also resolve all problems that we have here.

+++ b/core/modules/node/node.installundefined
@@ -752,6 +838,69 @@ function node_update_8015() {
+  $schema = node_schema();
+  $tables = array('node_field_data', 'node_field_revision');

You must not rely on the definitions in node_schema(). The problem is that we might change the schema again and then the hook will reflect the new state, this update function will already create the tables like that and the next update function will try to alter it again and fail. Needs to be copied, as ugly as it is.

+++ b/core/modules/node/node.installundefined
@@ -752,6 +838,69 @@ function node_update_8015() {
+  if (!isset($sandbox['progress'])) {
+    // Create the new tables.
+    foreach ($tables as $table) {
+      db_create_table($table, $schema[$table]);

I would recommend to split it up into three different update functions. 1. create new structure. 2. migrate data. 3. delete old fields. Especially together with the full schema definition, this will otherwise get quite long and is harder to maintain.

+++ b/core/modules/node/node.installundefined
@@ -752,6 +838,69 @@ function node_update_8015() {
+  $result = db_query_range('SELECT nr.*, nr.timestamp AS revision_timestamp, nr.uid as revision_uid, 1 AS default_langcode, n.langcode, n.vid = nr.vid AS default_revision, n.uid, n.changed, n.created, n.type FROM {node_revision} nr JOIN {node} n ON nr.nid = n.nid ORDER BY nr.nid ASC, nr.vid ASC', $sandbox['progress'], 10);

I think we can increase the 10 here easily to 50 or so. That should speed this up quite a bit as we can multi-insert bigger groups.

+++ b/core/modules/search/search.api.phpundefined
@@ -250,18 +249,18 @@ function hook_search_execute($keys = NULL, $conditions = NULL) {
+      'type' => check_plain(node_get_type_label($example)),
+      'title' => $example->label($item->langcode),
+      'user' => theme('username', array('account' => $example)),

Everything else was changed to $example but this still mentions node_get_type_label().

plach’s picture

Status: Needs work » Needs review
FileSize
201.06 KB
16.07 KB

@Berdir:

Thanks! This should take care of #254.

I'd suggest to extract that and fix the major bug (that we might want to make critical) first, so that this and the user ng conversion can go on.

Ok, reverted that hunk. Tests are going to fail now and this is blocked on #1957888: Exception when a field is empty on programmatic creation, which I agree should be marked critical.

Changes in comment.module are going to conflict a lot with the comment field issue.

I'll be happy to revert those two lines if the comment field issue goes in first, but meanwhile I think we should keep them, otherwise comment stuff is going to break. OTOH if this goes in first rerolling the other issue should be pretty easy.

You must not rely on the definitions in node_schema(). [...]

Well, I thought that relying on the declared schema would make sense until "upgrade path freeze", since this way tracking changes like the removal of the tnid column would be easier. Anyway, this should make the upgrade path code more reliable, so I implemented all of your suggestions.

Everything else was changed to $example but this still mentions node_get_type_label().

Since all the examples were very node-focused, I felt it wouldn't make sense to keep the "example" approach here. Instead I just updated them with the actual node code.

plach’s picture

Mmh, weird, it seems this no longer needs the EntityNG fix...

das-peter’s picture

I feel not qualified at all to say whether this is RTBC or not, but to me the patch looks quite good.
There are only some minor cleanup things I've found - besides the more critical search and replacement artifacts:

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/UidRevision.php
@@ -21,7 +21,7 @@ class UidRevision extends Uid {
-    $this->query->add_where_expression(0, "$this->tableAlias.uid = $placeholder OR ((SELECT COUNT(*) FROM {node_revision} nr WHERE nr.uid = $placeholder AND nr.nid = $this->tableAlias.nid) > 0)", array($placeholder => $this->argument));
+    $this->query->add_where_expression(0, "$this->tableAlias.revision_uid = $placeholder OR ((SELECT COUNT(DISTINCT vid) FROM {node_field_revision} npr WHERE npr.revision_uid = $placeholder AND npr.nid = $this->tableAlias.nid) > 0)", array($placeholder => $this->argument));

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument/Vid.php
@@ -25,9 +25,9 @@ class Vid extends Numeric {
-    $results = db_select('node_revision', 'nr')
-      ->fields('nr', array('vid', 'nid', 'title'))
-      ->condition('nr.vid', $this->value)
+    $results = db_select('node_field_revision', 'npr')
+      ->fields('npr', array('vid', 'nid', 'title'))

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/filter/UidRevision.php
@@ -27,7 +27,7 @@ public function query($group_by = FALSE) {
-      ((SELECT COUNT(*) FROM {node_revision} nr WHERE nr.uid IN($placeholder) AND nr.nid = $this->tableAlias.nid) > 0)", array($placeholder => $args),
+      ((SELECT COUNT(DISTINCT vid) FROM {node_field_revision} npr WHERE npr.revision_uid IN($placeholder) AND npr.nid = $this->tableAlias.nid) > 0)", array($placeholder => $args),

I guess the "npr" shortcut is a legacy from node_property_data times. Should we clean this up to "nfr"?

+++ b/core/modules/node/node.module
@@ -1831,7 +1840,7 @@ function node_last_changed($nid) {
-  $result = db_query('SELECT r.vid, r.title, r.log, r.uid, n.vid AS current_vid, r.timestamp, u.name FROM {node_revision} r LEFT JOIN {node} n ON n.vid = r.vid INNER JOIN {users} u ON u.uid = r.uid WHERE r.nid = :nid ORDER BY r.vid DESC', array(':nid' => $node->nid));
+  $result = db_query('SELECT nr.vid, nr.title, nr.log, nr.revision_uid AS uid, n.vid AS current_vid, nr.revision_timestamp, u.name FROM {node_field_revision} nr LEFT JOIN {node} n ON n.vid = nr.vid INNER JOIN {users} u ON u.uid = nr.revision_uid WHERE nr.nid = :nid AND nr.default_langcode = 1 ORDER BY nr.vid DESC', array(':nid' => $node->nid));

How about using also "nfr" instead "nr" as shortcut here? Just for the sake of consistency.

+++ b/core/modules/views/config/views.view.archive.yml
@@ -70,7 +70,7 @@ display:
       arguments:
         created_year_month:
           id: created_year_month
-          table: node
+          table: node_property_date
           field: created_year_month
           default_action: summary
           exception:
@@ -89,7 +89,7 @@ display:

@@ -89,7 +89,7 @@ display:
       filters:
         status:
           id: status
-          table: node
+          table: node_property_date
           field: status
           value: '1'

+++ b/core/modules/views/config/views.view.taxonomy_term.yml
@@ -55,7 +55,7 @@ display:

@@ -55,7 +55,7 @@ display:
       sorts:
         sticky:
           id: sticky
-          table: node
+          table: node_property_date
           field: sticky
           order: DESC
           plugin_id: standard
@@ -67,7 +67,7 @@ display:

@@ -67,7 +67,7 @@ display:
             label: ''
         created:
           id: created
-          table: node
+          table: node_property_date
           field: created
           order: DESC

Looks like search and replacement artifacts. I guess that should be node_field_data and not node_property_date

plach’s picture

This takes care of #257, thanks!

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Patch is looking really good, I had a quick look at the @todo's added and I think we better create follow-ups to address them, so we no longer block everybody else.

List of newly added @todo's, added a link for the 'important' once

  1. @todo This should be actually filtering on the desired node status field language and just fall back to the default language. #1998416: Filter on the desired node status field language and just fall back to the default language.
  2. @todo Use multiple insertions to improve performance.
  3. @todo Remove this once comment is a regular entity field.
  4. @todo Make the {node_field_revision}.log column nullable so that we can remove this check.
  5. @todo Introduce node_mass_delete() or make node_mass_update() more flexible
  6. @todo: Wouldn't it be possible to use $this->base_table and no if here?
attiks’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC, unless somebody thinks some of the @todo's should be addressed in this issue

dawehner’s picture

I manually tested the views bits of the patch and just spotted two problems, which clearly shows some lack of test coverage in views, see #1998488: [views tests] Missing test coverage for node integration.

+++ b/core/modules/node/node.views.incundefined
@@ -411,15 +419,15 @@ function node_views_data() {
-  $data['node_revision']['table']['wizard_id'] = 'node_revision';
...
+  $data['node_field_revision']['table']['wizard_id'] = 'node_field_revision';

Let's not change the plugin id.

+++ b/core/modules/node/node.views.incundefined
@@ -518,12 +526,12 @@ function node_views_data() {
-      'id' => 'node_revision',
+      'id' => 'node_field_revision',

The plugin_id should not change.

alexpott’s picture

Issue tags: +needs profiling

We need to extensively profile this patch. Quoting the issue summary

multilingual queries introduce performance penalties varying from nearly 0 to 15%.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1005 bytes

Not sure if it's related, but one thing that I noticed is that this again produced debug messages like "'Missing handler: node sticky sort'" and a few others as well during upgrade path and also on the frontpage. that might be related to what @dawehner said in #261 As that results in watchdog messages, that's probably one reason for the slowdown.

Another problem that's quite obvious when you load 100 nodes is that the magic in attachPropertyData() is rather slow (note that the CPU xhprof flag makes it slower than it actually is) but it might still be worthwhile to avoid the magic there, see attached interdiff.

Setting to needs work for now because of the views stuff.

attiks’s picture

first improvement (just posting for reference)

alter table node_field_data drop key node_frontpage;
alter table node_field_data add KEY `node_various` (`promote`,`status`,`sticky`,`created`, `default_langcode`);
attiks’s picture

Some more tests (2500 nodes in 2 languages) using

  • alter table node_field_data add KEY `node_frontpage_created` (`created`);
  • alter table node_field_data add KEY `node_various` (`promote`,`status`,`sticky`,`created`, `default_langcode`);

100 runs each

0::0.215742s - SELECT COUNT(*) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1
1::0.224756s - SELECT COUNT(DISTINCT nid) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1
2::0.184260s - SELECT COUNT(nid) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1
3::0.191342s - SELECT COUNT(promote) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1
4::0.166229s - SELECT COUNT(status) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1
5::0.226076s - SELECT COUNT(*) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1 AND default_langcode = 1
6::0.206801s - SELECT COUNT(nid) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 1 AND default_langcode = 1
plach’s picture

Status: Needs work » Needs review
FileSize
200.99 KB
1.87 KB

Thanks everybody for reviewing. Here is a new one incorporating suggestions from #261 and #263.

Berdir’s picture

Just discussed with fago, we should try to build up $values for all languages and then instantiate the entity class with that at once for all languages.

plach’s picture

Wrt #259 I think it's fair to introduce these @todos because most of them would be OT here. The only exception is:

@todo This should be actually filtering on the desired node status field language and just fall back to the default language.

OTOH at the moment these queries are working exactly as without the patch, and allowing for more multilingual flexibility is something that we will be able to address only after getting (at least) #1810370: Entity Translation API improvements.

attiks’s picture

FileSize
201.2 KB
890 bytes

Most SQL differences are solved by adding indexes.
alter table node_field_data add KEY `node_default_langcode` (`default_langcode`);
alter table node_field_data add KEY `node_langcode` (`langcode`);
alter table node_field_revision add KEY `node_default_langcode` (`default_langcode`);
alter table node_field_revision add KEY `node_langcode` (`langcode`);

attiks’s picture

Included profiled runs of views_page('frontpage', 'page_1'); the content was not 100% identical, both sites had +100 nodes with comments in 2 languages.

attiks’s picture

Run #without-patch Run #with-patch Diff Diff%
Number of function xhprof_Calls 34,247 35,824 1,577 4.6%
Incl. Wall Diff (microsec) 378,553 356,581 -21,972 -5.8%
Incl. CPU Diff (microsec) 352,021 348,022 -3,999 -1.1%
Incl. MemUse Diff (bytes) 9,722,864 10,902,464 1,179,600 12.1%
Incl. PeakMemUse Diff (bytes) 9,721,032 10,909,904 1,188,872 12.2%
Function Name Calls Diff Calls Diff% Incl. Wall Diff (microsec) IWall Diff% Excl. Wall Diff (microsec) EWall Diff% Incl. CPU Diff (microsec) ICpu Diff% Excl. CPU Diff (microsec) ECpu Diff% Incl. MemUse Diff (bytes) IMemUse Diff% Excl. MemUse Diff (bytes) EMemUse Diff% Incl. PeakMemUse Diff (bytes) IPeakMemUse Diff% Excl. PeakMemUse Diff (bytes) EPeakMemUse Diff%
Drupal\Core\Entity\DatabaseStorageControllerNG::attachPropertyData 0 0.0% 42,732 194.5% 4,282 19.5% 40,003 1000.3% 8,000 200.1% 1,280,632 108.6% 13,616 1.2% 1,325,464 111.5% 48,248 4.1%
Drupal\Core\Entity\DatabaseStorageControllerNG::mapFromStorageRecords 0 0.0% 42,425 193.1% -188 -0.9% 40,003 1000.3% 0 0.0% 1,210,120 102.6% -52,800 -4.5% 1,254,416 105.5% -34,616 -2.9%
Drupal\node\NodeStorageController::attachLoad 0 0.0% 40,268 183.3% -112 -0.5% 40,002 1000.3% 0 0.0% 832,136 70.5% 18,000 1.5% 850,336 71.5% -2,280 -0.2%
entity_load_multiple 0 0.0% 37,424 170.3% -52 -0.2% 56,003 1400.4% 0 0.0% 1,021,568 86.6% 0 0.0% 1,039,720 87.5% 280 0.0%
Drupal\views\Plugin\views\query\Sql::execute 0 0.0% 36,727 167.2% -60 -0.3% 40,001 1000.3% 0 0.0% 922,280 78.2% -1,352 -0.1% 949,088 79.8% -880 -0.1%
Drupal\views\Plugin\views\query\Sql::load_entities 0 0.0% 36,708 167.1% -30 -0.1% 40,002 1000.3% 0 0.0% 881,064 74.7% 0 0.0% 904,624 76.1% 1,520 0.1%
Drupal\views\ViewExecutable::execute 0 0.0% 32,282 146.9% -29 -0.1% 36,002 900.3% 0 0.0% 921,376 78.1% 0 0.0% 949,608 79.9% 360 0.0%
Drupal\Core\Entity\DatabaseStorageController::load 0 0.0% 32,164 146.4% -73 -0.3% 44,002 1100.3% 4,000 100.0% 789,464 66.9% 16,008 1.4% 807,888 68.0% -72 -0.0%
Drupal\Core\Extension\ModuleHandler::getImplementationInfo -30 -1.9% -27,648 -125.8% -10,210 -46.5% -20,002 -500.2% -4,000 -100.0% -70,016 -5.9% 19,680 1.7% -33,480 -2.8% -14,400 -1.2%
Drupal\Core\Extension\ModuleHandler::getImplementations 2 0.1% -27,545 -125.4% -55 -0.3% -20,002 -500.2% 0 0.0% -55,112 -4.7% 1,320 0.1% -16,368 -1.4% 352 0.0%
Drupal\Core\Extension\CachedModuleHandler::getImplementationInfo 2 0.1% -27,488 -125.1% -279 -1.3% -20,002 -500.2% 0 0.0% -57,168 -4.8% -3,112 -0.3% -18,072 -1.5% -1,520 -0.1%
main() 0 0.0% -21,972 -100.0% -10 -0.0% -3,999 -100.0% 0 0.0% 1,179,600 100.0% 0 0.0% 1,188,872 100.0% 0 0.0%
views_page 0 0.0% -21,914 -99.7% 4 0.0% -3,999 -100.0% 0 0.0% 1,179,584 100.0% 0 0.0% 1,189,016 100.0% 0 0.0%
Drupal\Core\Extension\ModuleHandler::invokeAll 0 0.0% -21,661 -98.6% -71 -0.3% -12,003 -300.2% 0 0.0% -42,600 -3.6% 0 0.0% -1,376 -0.1% 16 0.0%
Drupal\views\Plugin\views\display\PathPluginBase::execute 0 0.0% -20,289 -92.3% -62 -0.3% -8,001 -200.1% 0 0.0% 293,896 24.9% 0 0.0% 293,656 24.7% 0 0.0%
Drupal\views\ViewExecutable::build 0 0.0% -20,227 -92.1% -120 -0.5% -8,001 -200.1% 0 0.0% 293,896 24.9% 0 0.0% 293,656 24.7% 328 0.0%
Drupal\Core\Entity\EntityNG::getTranslatedField 110 7.0% 20,123 91.6% 2,386 10.9% 12,003 300.2% 0 0.0% 571,488 48.4% 56,952 4.8% 605,256 50.9% 44,136 3.7%
Drupal\views\ViewExecutable::executeDisplay 0 0.0% -19,668 -89.5% -8 -0.0% 0 0.0% 0 0.0% 1,171,344 99.3% 0 0.0% 1,180,344 99.3% 0 0.0%
PDOStatement::execute 3 0.2% -19,167 -87.2% -19,167 -87.2% -4,000 -100.0% -4,000 -100.0% 21,608 1.8% 21,608 1.8% 10,688 0.9% 10,688 0.9%
Drupal\Core\Database\Statement::execute 3 0.2% -19,142 -87.1% 57 0.3% -8,000 -200.1% 0 0.0% 21,608 1.8% 0 0.0% 10,688 0.9% 0 0.0%
Drupal\Component\Plugin\PluginManagerBase::createInstance 1 0.1% -18,287 -83.2% -6 -0.0% -8,002 -200.1% 0 0.0% 41,464 3.5% 1,168 0.1% 43,064 3.6% 1,168 0.1%
Drupal\Core\Database\Connection::query 3 0.2% -18,049 -82.1% 169 0.8% -16,002 -400.2% 4,000 100.0% 27,616 2.3% -5,136 -0.4% 30,640 2.6% 5,312 0.4%
Drupal\Core\Plugin\Factory\ContainerFactory::createInstance 0 0.0% -16,557 -75.4% -13 -0.1% -16,001 -400.1% 0 0.0% -16 -0.0% 0 0.0% -2,560 -0.2% 0 0.0%
Drupal\Core\Entity\EntityNG::__get 150 9.5% 16,199 73.7% -82 -0.4% 28,002 700.2% 12,000 300.1% 586,272 49.7% 9,128 0.8% 617,560 51.9% 3,872 0.3%
Drupal\views\Plugin\views\display\DisplayPluginBase::getPlugin 0 0.0% -14,832 -67.5% -152 -0.7% -12,000 -300.1% 0 0.0% 4,112 0.3% 0 0.0% -1,432 -0.1% -544 -0.0%
Drupal\Core\TypedData\TypedDataManager::getPropertyInstance 110 7.0% 14,398 65.5% 3,642 16.6% 8,002 200.1% 8,001 200.1% 446,288 37.8% 87,064 7.4% 476,760 40.1% 92,336 7.8%
Drupal\Core\Cache\DatabaseBackend::getMultiple 2 0.1% -13,467 -61.3% -401 -1.8% -24,002 -600.2% 0 0.0% 81,920 6.9% 3,536 0.3% 96,848 8.1% 5,840 0.5%
Drupal\Core\Plugin\Discovery\CacheDecorator::getDefinitions 1 0.1% -13,249 -60.3% -6 -0.0% -12,001 -300.1% 0 0.0% 9,320 0.8% 0 0.0% 22,744 1.9% 128 0.0%
Drupal\Core\Plugin\Discovery\CacheDecorator::getCachedDefinitions 1 0.1% -13,243 -60.3% -20 -0.1% -12,001 -300.1% 0 0.0% 9,320 0.8% -2,864 -0.2% 22,616 1.9% 0 0.0%
Drupal\Core\Plugin\Discovery\CacheDecorator::getDefinition 26 1.6% -13,162 -59.9% 92 0.4% -8,001 -200.1% 4,000 100.0% 9,320 0.8% 0 0.0% 22,864 1.9% 120 0.0%
Drupal\views\ViewExecutable::render 0 0.0% 12,562 57.2% -598 -2.7% 20,002 500.2% 4,001 100.1% 924,376 78.4% -48 -0.0% 925,648 77.9% 2,096 0.2%
Drupal\Core\Cache\DatabaseBackend::get 2 0.1% -12,365 -56.3% 15 0.1% -20,001 -500.2% 0 0.0% 80,368 6.8% -784 -0.1% 93,232 7.8% 312 0.0%
Drupal\Core\Extension\ModuleHandler::loadInclude -420 -26.6% -12,360 -56.3% -7,699 -35.0% -8,000 -200.1% -4,000 -100.0% -37,848 -3.2% -1,144 -0.1% 10,168 0.9% -2,936 -0.2%
Drupal\Component\Plugin\PluginManagerBase::getDefinition 1 0.1% -12,355 -56.2% 20 0.1% -12,001 -300.1% 0 0.0% 6,432 0.5% 0 0.0% 1,080 0.1% 360 0.0%
Drupal\views\Plugin\views\style\StylePluginBase::pre_render 0 0.0% -12,108 -55.1% -3 -0.0% -8,000 -200.1% 0 0.0% 20,408 1.7% 0 0.0% 14,984 1.3% 0 0.0%
Drupal\system\Plugin\views\row\EntityRow::pre_render 0 0.0% -12,105 -55.1% -42 -0.2% -8,000 -200.1% 0 0.0% 20,408 1.7% 0 0.0% 14,984 1.3% 0 0.0%
entity_view_multiple 0 0.0% -11,661 -53.1% -24 -0.1% -12,001 -300.1% 0 0.0% 21,208 1.8% 0 0.0% 15,784 1.3% 0 0.0%
Drupal\Core\Entity\EntityRenderController::viewMultiple 0 0.0% -11,125 -50.6% 10 0.0% -12,001 -300.1% 0 0.0% 21,208 1.8% -3,056 -0.3% 15,784 1.3% -264 -0.0%
Drupal\views\ViewExecutable::initQuery 0 0.0% -10,797 -49.1% -1 -0.0% 3,999 100.0% 0 0.0% 0 0.0% 0 0.0% 224 0.0% 0 0.0%
Drupal\node\NodeRenderController::buildContent 0 0.0% -9,559 -43.5% -104 -0.5% -7,999 -200.0% 0 0.0% -95,368 -8.1% -48 -0.0% -98,104 -8.3% -2,896 -0.2%
Drupal\views\Plugin\views\display\Page::execute 0 0.0% -7,971 -36.3% -16 -0.1% 8,001 200.1% 0 0.0% 1,218,304 103.3% 0 0.0% 1,227,304 103.2% 592 0.0%
Drupal\field\Plugin\Core\Entity\FieldInstance::unserialize 0 0.0% 7,752 35.3% 17 0.1% 8,000 200.1% 4,000 100.0% -9,264 -0.8% 512 0.0% -6,752 -0.6% 80 0.0%
Drupal\field\Plugin\Core\Entity\FieldInstance::__construct 0 0.0% 7,747 35.3% 14 0.1% 4,000 100.0% 4,001 100.1% 24 0.0% 0 0.0% -2,736 -0.2% 0 0.0%
Drupal\field\FieldInfo::getFieldById 6 0.4% 7,627 34.7% 60 0.3% -4,001 -100.1% -4,000 -100.0% 24 0.0% 0 0.0% -2,608 -0.2% 0 0.0%
Drupal\field\FieldInfo::getBundleInstances 0 0.0% 7,618 34.7% -10 -0.0% 8,001 200.1% 4,001 100.1% -12,488 -1.1% -16 -0.0% -17,376 -1.5% 0 0.0%
field_info_field_by_id 6 0.4% 7,585 34.5% 12 0.1% 0 0.0% 4,001 100.1% 24 0.0% 0 0.0% -1,792 -0.2% 584 0.0%
field_attach_load 0 0.0% -7,553 -34.4% -27 -0.1% -8,001 -200.1% 0 0.0% -467,776 -39.7% 2,216 0.2% -468,752 -39.4% -3,032 -0.3%
field_info_instances 0 0.0% 7,479 34.0% -57 -0.3% 8,001 200.1% 0 0.0% -12,488 -1.1% 0 0.0% -16,960 -1.4% 584 0.0%
Drupal\Core\Entity\EntityBCDecorator::__set 2 0.1% -7,463 -34.0% -542 -2.5% 4,001 100.1% 4,001 100.1% -470,360 -39.9% -2,608 -0.2% -470,216 -39.6% -2,600 -0.2%
Drupal\Core\TypedData\TypedDataManager::create 11 0.7% 6,959 31.7% 117 0.5% 1 0.0% 0 0.0% 130,832 11.1% 0 0.0% 125,496 10.6% 0 0.0%
field_read_fields 0 0.0% 6,879 31.3% 75 0.3% 8,000 200.1% 0 0.0% 24 0.0% 0 0.0% -2,728 -0.2% 0 0.0%
Drupal\Core\TypedData\TypedDataFactory::createInstance 11 0.7% 6,842 31.1% 279 1.3% 1 0.0% 0 0.0% 130,832 11.1% 9,984 0.8% 125,496 10.6% 9,320 0.8%
unserialize 3 0.2% 6,670 30.4% -1,208 -5.5% -4,000 -100.0% -8,000 -200.1% 72,344 6.1% 83,760 7.1% 87,856 7.4% 101,080 8.5%
Drupal\Core\Cache\DatabaseBackend::prepareItem 3 0.2% 6,670 30.4% -36 -0.2% -8,000 -200.1% -4,000 -100.0% 69,800 5.9% -4,776 -0.4% 86,680 7.3% 72 0.0%
field_attach_prepare_view 0 0.0% -6,005 -27.3% -20 -0.1% -4,000 -100.0% 0 0.0% -41,056 -3.5% 0 0.0% -43,720 -3.7% -624 -0.1%
Drupal\views\ViewExecutable::preExecute 0 0.0% -5,997 -27.3% 9 0.0% -4,000 -100.0% 0 0.0% -47,032 -4.0% 0 0.0% -46,848 -3.9% -2,160 -0.2%
field_invoke_method_multiple 0 0.0% -5,960 -27.1% -106 -0.5% -4,000 -100.0% -4,001 -100.1% -43,256 -3.7% -2,600 -0.2% -41,816 -3.5% -3,968 -0.3%
Drupal\Core\Entity\EntityNG::language 21 1.3% -5,670 -25.8% 161 0.7% -3,999 -100.0% 0 0.0% -453,072 -38.4% 0 0.0% -470,832 -39.6% -856 -0.1%
Drupal\Core\Entity\Field\Type\Field::__construct 11 0.7% 5,507 25.1% 231 1.1% 1 0.0% 0 0.0% 120,800 10.2% 5,632 0.5% 114,872 9.7% 720 0.1%
Drupal\Core\Entity\EntityRenderController::buildContent 0 0.0% -5,491 -25.0% 52 0.2% 2 0.1% 0 0.0% -22,224 -1.9% 0 0.0% -26,896 -2.3% -496 -0.0%
Drupal\Core\TypedData\ItemList::createItem 11 0.7% 5,211 23.7% 198 0.9% 1 0.0% -4,000 -100.0% 115,168 9.8% -880 -0.1% 114,152 9.6% 1,856 0.2%
Drupal\entity\Plugin\Core\Entity\EntityDisplay::getFormatter 2 0.1% -5,089 -23.2% -49 -0.2% 0 0.0% 0 0.0% -43,256 -3.7% -832 -0.1% -34,240 -2.9% -1,112 -0.1%
{closure} 2 0.1% -5,080 -23.1% 50 0.2% 0 0.0% 0 0.0% -43,256 -3.7% 0 0.0% -34,792 -2.9% -312 -0.0%
Drupal\Core\TypedData\TypedDataManager::getPropertyInstance@1 11 0.7% 4,959 22.6% 822 3.7% 4,001 100.1% 0 0.0% 115,168 9.8% 11,208 1.0% 111,576 9.4% 13,616 1.1%
Drupal\Core\Config\Entity\ConfigStorageController::buildQuery 0 0.0% 4,924 22.4% 245 1.1% 8,000 200.1% 0 0.0% 121,392 10.3% 1,456 0.1% 115,624 9.7% -584 -0.0%
_field_invoke_multiple 0 0.0% -4,856 -22.1% -258 -1.2% -8,001 -200.1% 4,000 100.0% 5,312 0.5% -2,224 -0.2% 38,864 3.3% 7,128 0.6%
translation_entity_entity_load 0 0.0% 4,732 21.5% 183 0.8% 4,001 100.1% 0 0.0% 72,400 6.1% 448 0.0% 81,640 6.9% 624 0.1%
field_language 0 0.0% 4,696 21.4% 31 0.1% 16,001 400.1% 0 0.0% -14,704 -1.2% -728 -0.1% -40,192 -3.4% -912 -0.1%
Drupal\Core\Entity\EntityNG::get 151 9.6% 4,645 21.1% 803 3.7% 1 0.0% 4,000 100.0% -4,496 -0.4% 1,160 0.1% 24 0.0% 8,456 0.7%
Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass 1 0.1% -4,585 -20.9% -14 -0.1% -1 -0.0% 0 0.0% 29,056 2.5% 0 0.0% 13,352 1.1% 0 0.0%
call_user_func 2 0.1% -4,486 -20.4% 67 0.3% 11,999 300.1% 4,000 100.0% -43,272 -3.7% 0 0.0% -36,608 -3.1% 0 0.0%
Drupal\views\ViewExecutable::initDisplay 0 0.0% -4,413 -20.1% 9 0.0% -1 -0.0% 0 0.0% 64 0.0% 0 0.0% -344 -0.0% -784 -0.1%
Drupal\views\ViewExecutable::setDisplay 0 0.0% -4,384 -20.0% 33 0.2% -8,001 -200.1% 0 0.0% 72 0.0% 0 0.0% 56 0.0% 0 0.0%
Drupal\Core\Extension\ModuleHandler::alter -17 -1.1% -4,230 -19.3% -327 -1.5% -8,001 -200.1% -4,000 -100.0% -15,392 -1.3% -2,160 -0.2% -19,840 -1.7% -3,448 -0.3%
Drupal\Core\Config\Entity\ConfigStorageController::load 0 0.0% 3,897 17.7% 4 0.0% 8,001 200.1% 0 0.0% 122,760 10.4% 0 0.0% 117,232 9.9% 88 0.0%
drupal_alter 0 0.0% -3,690 -16.8% -18 -0.1% -12,001 -300.1% 0 0.0% -2,584 -0.2% 16 0.0% -15,640 -1.3% -512 -0.0%
is_file -420 -26.6% -3,601 -16.4% -3,601 -16.4% -4,000 -100.0% -4,000 -100.0% -8 -0.0% -8 -0.0% 1,248 0.1% 1,248 0.1%
Drupal\Core\TypedData\TypedDataManager::create@1 11 0.7% 3,411 15.5% 127 0.6% 4,001 100.1% 0 0.0% 87,320 7.4% 0 0.0% 79,176 6.7% 0 0.0%
Drupal\entity\Plugin\Core\Entity\EntityDisplay::getComponent -14 -0.9% -3,292 -15.0% -264 -1.2% 0 0.0% 0 0.0% -17,968 -1.5% 2,592 0.2% -39,976 -3.4% -312 -0.0%
Drupal\Core\TypedData\TypedDataFactory::createInstance@1 11 0.7% 3,284 14.9% 258 1.2% 4,001 100.1% 0 0.0% 87,320 7.4% 11,752 1.0% 79,176 6.7% 9,104 0.8%
Drupal\views\ViewStorageController::load 0 0.0% -3,129 -14.2% -11 -0.1% 1 0.0% 0 0.0% 1,880 0.2% 0 0.0% 1,880 0.2% 0 0.0%
image_field_prepare_view 0 0.0% -2,992 -13.6% -11 -0.1% 0 0.0% 0 0.0% 6,400 0.5% 0 0.0% 6,496 0.5% -160 -0.0%
file_field_prepare_view 0 0.0% -2,981 -13.6% -60 -0.3% 0 0.0% 0 0.0% 6,400 0.5% -1,424 -0.1% 6,656 0.6% -176 -0.0%
Drupal\views\ViewExecutable::initStyle 0 0.0% -2,940 -13.4% -51 -0.2% 0 0.0% 0 0.0% -32 -0.0% 0 0.0% 2,264 0.2% 0 0.0%
function_exists -1,395 -88.5% -2,900 -13.2% -2,900 -13.2% -12,002 -300.1% -12,002 -300.1% -1,160 -0.1% -1,160 -0.1% -1,016 -0.1% -1,016 -0.1%
file_load_multiple 0 0.0% -2,886 -13.1% -4 -0.0% 0 0.0% 0 0.0% 3,528 0.3% 0 0.0% 2,888 0.2% 0 0.0%
Drupal\views\Plugin\views\display\DisplayPluginBase::preExecute 0 0.0% -2,884 -13.1% -27 -0.1% -4,000 -100.0% 0 0.0% 2,896 0.2% 0 0.0% -31,728 -2.7% -1,792 -0.2%
Drupal\views\DisplayBag::initializePlugin 0 0.0% -2,787 -12.7% 16 0.1% -8,001 -200.1% 0 0.0% 40 0.0% 0 0.0% -16 -0.0% 0 0.0%
Drupal\views\DisplayBag::__construct 0 0.0% -2,787 -12.7% 24 0.1% -1 -0.0% 0 0.0% 32 0.0% 0 0.0% 408 0.0% 0 0.0%
Drupal\Core\Entity\DatabaseStorageController::buildQuery -1 -0.1% -2,746 -12.5% -143 -0.7% 0 0.0% 0 0.0% -234,208 -19.9% 5,976 0.5% -240,992 -20.3% -1,192 -0.1%
Drupal\Core\TypedData\ItemList::__clone 110 7.0% 2,567 11.7% 1,810 8.2% 0 0.0% 0 0.0% 218,136 18.5% 174,080 14.8% 253,240 21.3% 185,408 15.6%
Drupal\Core\Config\Config::get -1 -0.1% 2,525 11.5% 119 0.5% 12,000 300.1% 0 0.0% 6,216 0.5% 1,664 0.1% 3,192 0.3% -24 -0.0%
Drupal\Core\Entity\EntityNG::__isset 0 0.0% -2,524 -11.5% -3 -0.0% -8,001 -200.1% -4,000 -100.0% -70,096 -5.9% 0 0.0% -71,400 -6.0% 160 0.0%
Drupal\views\Plugin\views\query\Sql::alter 0 0.0% -2,519 -11.5% -11 -0.1% -4,000 -100.0% 0 0.0% -456 -0.0% 0 0.0% -1,112 -0.1% 320 0.0%
Drupal\node\NodeStorageController::buildQuery -1 -0.1% -2,493 -11.3% -32 -0.1% 0 0.0% 0 0.0% -248,448 -21.1% -2,432 -0.2% -251,328 -21.1% -1,088 -0.1%
field_language_fallback 0 0.0% -2,473 -11.3% -44 -0.2% 4,001 100.1% 4,000 100.0% -472 -0.0% -160 -0.0% -5,544 -0.5% -336 -0.0%
Drupal\Core\Entity\EntityNG::label 0 0.0% -2,393 -10.9% 105 0.5% -8,001 -200.1% 0 0.0% -71,936 -6.1% -1,840 -0.2% -72,904 -6.1% -1,392 -0.1%
Drupal\Core\Entity\EntityBCDecorator::label 0 0.0% -2,386 -10.9% 7 0.0% -8,001 -200.1% 0 0.0% -71,936 -6.1% 0 0.0% -72,904 -6.1% 0 0.0%
Drupal\field\Plugin\Type\Formatter\FormatterPluginManager::getInstance 0 0.0% -2,311 -10.5% -49 -0.2% 0 0.0% 0 0.0% 24 0.0% -32 -0.0% -656 -0.1% 496 0.0%
views_get_view 0 0.0% -2,258 -10.3% -8 -0.0% -3,999 -100.0% 0 0.0% 8,240 0.7% 0 0.0% 8,672 0.7% 0 0.0%
plach’s picture

Those numbers look a bit weird to me. How about performing the patch test with an upgraded db, i.e. with the very same data.

attiks’s picture

I'm going to add a D8 -> D8 upgrade today to test with the same data.

attiks’s picture

FileSize
1001.76 KB
982.76 KB

New results using the same data on a standard Drupal 8 install with 2 languages, 50 nodes (5 runs):

  drupal_flush_all_caches();
  views_page('frontpage', 'page_1');

In short: wall time +1%

Details (# calls):

  • Drupal\Core\Entity\EntityNG::getTranslatedField: +500 (3.4%)
  • Drupal\Core\Plugin\Discovery\CacheDecorator::getDefinition: +123 (0.8%)
  • Drupal\Core\Entity\EntityNG::get: +1080 (7.4%)
  • Drupal\Core\Entity\EntityBCDecorator::__get: +480 (3.3%)
  • Drupal\Core\Entity\EntityNG::__get: +3745 (25.7%)

Details (mem use):

  • Drupal\Core\Entity\DatabaseStorageControllerNG::attachPropertyData: +117.4%
  • Drupal\Core\Entity\DatabaseStorageControllerNG::mapFromStorageRecords: +107.2%
  • entity_load_multiple: +77%
  • Drupal\Core\Plugin\Discovery\CacheDecorator::getDefinition: +25.5%)
Run #without Run #with Diff Diff%
Number of function xhprof_Calls 5,724,057 5,738,624 14,567 0.3%
Incl. Wall Diff (microsec) 70,051,823 70,770,096 718,273 1.0%
Incl. CPU Diff (microsec) 62,207,888 62,779,924 572,036 0.9%
Incl. MemUse Diff (bytes) 40,924,416 45,039,864 4,115,448 10.1%
Incl. PeakMemUse Diff (bytes) 55,558,464 59,748,704 4,190,240 7.5%
Gábor Hojtsy’s picture

I talked to @attiks and @berdir about the idea to load all $values as per above in #267. @attiks said that would be a memory problem but a performance improvement. @berdir says we already load the same data but ALSO instantiate the objects. So by loading up a $values array, we avoid the instantiation as late as possible, and only have some memory footprint (also have smaller memory footprint hopefully due to the need for no instantiation).

plach’s picture

Assigned: Unassigned » plach

Working on #275.

Berdir’s picture

One problem with that is that we need a way to know which column (value vs target_id) a value belongs to. If that doesn't work based on the bundle (as we might not know that yet, not sure), @fago suggested change the column of everything that doesn't use value to contain it, e.g. uid__target_id.

Berdir’s picture

To explain what I meant, some examples:

Column "status" maps to "$entity->status->value", if nothing is specified, we default to ->value.

To use something else, name your database column something like "uid__target_id", that maps to "$entity->uid->target_id".

That also means we automatically support multiple primitive values for a field if we have "reference__entity_type" and "reference__entity_id", we can map that to "$entity->reference->entity_type" and "$entity->reference->entity_id".

plach’s picture

Well, #268 makes sense to me but I don't think it belongs to this issue. Anyway, it probably would make more sense to just use "->value" when a field has a single column. Or we could have a 'default column' property in the field definition that defaults to 'value'. The above proposal makes sense to me for multicolumned non-configurable fields.

plach’s picture

Ok, let's see how many things break :)

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-280.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
202.53 KB
2.93 KB

This might fix all failures.

attiks’s picture

Good news and bad news

It's faster but with more memory overhead

Run #282without Run #282with Diff Diff%
Number of function xhprof_Calls 5,024,636 5,058,860 34,224 0.7%
Incl. Wall Diff (microsec) 66,723,621 62,927,135 -3,796,486 -5.7%
Incl. CPU Diff (microsec) 57,787,611 54,707,419 -3,080,192 -5.3%
Incl. MemUse Diff (bytes) 38,473,984 43,364,504 4,890,520 12.7%
Incl. PeakMemUse Diff (bytes) 49,666,512 55,366,456 5,699,944 11.5%

I'll upload the xhprofs as well

attiks’s picture

FileSize
835.2 KB
866.21 KB

xhprofs

attiks’s picture

memory mystery solved, was caused by the in between cache clears

this is the same test with only a cc all before testing

Run #nocache Run #nocachepatched2 Diff Diff%
Number of function xhprof_Calls 458,604 454,428 -4,176 -0.9%
Incl. Wall Diff (microsec) 5,317,032 5,313,479 -3,553 -0.1%
Incl. CPU Diff (microsec) 4,932,308 4,884,305 -48,003 -1.0%
Incl. MemUse Diff (bytes) 24,689,584 24,095,528 -594,056 -2.4%
Incl. PeakMemUse Diff (bytes) 24,803,200 24,195,624 -607,576 -2.4%
plach’s picture

So this is making the front page faster? Hard to believe :)

attiks’s picture

#286 I double checked the profile files and it might be that something went wrong, I would love somebody else to profile this as well (preferably with and without cache clears in between)

plach’s picture

I will tonight

Gábor Hojtsy’s picture

Amazing! we would ideally have data ASAP, so we can look art it and figure out next steps while we are on the same space-time continuum.

Gábor Hojtsy’s picture

Amazing! we would ideally have data ASAP, so we can look art it and figure out next steps while we are on the same space-time continuum.

carsonevans’s picture

bbranches 519ffa9a30e84 et-node-ml-1498674-282.patch
=== 8.x..8.x compared (519ffa9a30e84..519ffafe96886):

ct : 40,386|40,386|0|0.0%
wt : 447,597|447,202|-395|-0.1%
cpu : 432,655|432,636|-19|-0.0%
mu : 40,772,776|40,759,592|-13,184|-0.0%
pmu : 40,905,480|40,892,112|-13,368|-0.0%

Profiler output

=== 8.x..et-node-ml-1498674-282.patch compared (519ffa9a30e84..519ffb355f5ce):

ct : 40,386|41,335|949|2.3%
wt : 447,597|459,557|11,960|2.7%
cpu : 432,655|439,488|6,833|1.6%
mu : 40,772,776|40,722,768|-50,008|-0.1%
pmu : 40,905,480|40,857,112|-48,368|-0.1%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

carsonevans’s picture

May profiling of this may not have been ideal, so setting it back to needs profiling so someone else can give it a shot.

plach’s picture

Here are my numbers: I run several xhprof sessions on a plain/patched 8.x front page with 50 nodes and 1000 nodes in the db. The difference was pretty consistently around 1.5% wall time and ~0.5% mem use. However looking at the single functions, many differences varied from session to session, although the highest exclusive time diff was always in PDOStatement::execute(), which is varying between 18% and 45% (around 20ms). As you can see the variance is very high wrt to the reported difference, hence I'd say that we do have a performance penalty, but not reliably measurable. Microbenchmark results were a bit more consistent on this side.

To sum up the most relevant difference is, as expected, in the query time, for whose optimizaton we have a plan in the OP. It should also be noted that Views is not using node_field_data as base table yet, hence the resulting query is not ideal.

Run                              #head      #patch     Diff  Diff%
Number of Function Calls       271,728     272,204      476   0.2%
Incl. Wall Time (microsec)   3,205,856   3,252,692   46,836   1.5%
Incl. CPU (microsecs)        3,104,420   3,182,420   78,000   2.5%
Incl. MemUse (bytes)        20,334,376  20,462,616  128,240   0.6%
Incl. PeakMemUse (bytes)    20,848,096  20,976,352  128,256   0.6%
plach’s picture

Issue tags: -needs profiling

I think we are done with profiling here, unless someone else wishes to give this a last shot.

plach’s picture

I run another mb session with the following queries (the roughly optimized versions of the views frontpage query) on the same dbs, 10000 runs:

HEAD:

SELECT title FROM {node} WHERE status = 1 AND promote = 1 ORDER BY created -- 49.100055s

PATCH:

SELECT title FROM {node_field_data} WHERE status = 1 AND promote = 1 ORDER BY created -- 48.774051s

No measurable difference.

fago’s picture

@plach: amazing work, ++!

It's great to see that the numbers are finally better with the updated patch. I think the storage code could use some more clean-up, variable name & code-flow improvements etc., but that a) does not need to hold up this great work in any way b) that clean-up will need to go beyond of what this patch introduces - thus deserves its own issue anyway. -> followup.

das-peter’s picture

FileSize
202.5 KB

#1620010: Move LANGUAGE constants to the Language class was committed and interferes with #282 -> re-roll.
And here's an overview over all the introduced @todos:

  • Multiple queries: @todo This should be actually filtering on the desired node status field language and just fall back to the default language.
  • NodeStorageController::mapToDataStorageRecord(): @todo Remove this once comment is a regular entity field.
  • node_schema() @todo Remove the following columns when removing the legacy Content Translation module. See http://drupal.org/node/1952062.

Are there already existing follow-up issues for these todos?

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-296.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
202.58 KB

*blargh* not good. Here are (hopefully all) the necessary adjustments.

das-peter’s picture

Issue summary: View changes

Link added to 1998416

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-299.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
203.61 KB

Here we go - fixed views table configuration.

Status: Needs review » Needs work

The last submitted patch, et-node-ml-1498674-301.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
203.73 KB

Another reroll. Tomorrow I'll post some updated benchmarks in the OP. I think we should more or less ready for RTBC afterwards.

@fago:

I think the storage code could use some more clean-up, variable name & code-flow improvements etc., but that a) does not need to hold up this great work in any way b) that clean-up will need to go beyond of what this patch introduces - thus deserves its own issue anyway. -> followup.

I agree. I'd definitely wish to clean this stuff up while working on entity storage :)

PS: Can a d.o. power user delete the bot comments so we get back under the 300 threshold? Thanks :)

Gábor Hojtsy’s picture

Issue summary: View changes

I was confused for a second was talking about revisions of accounts

chrishks’s picture

Issue summary: View changes

Updated issue summary with link to 2004330

chrishks’s picture

Issue summary: View changes

Added links to follow-up issues

YesCT’s picture

Issue summary: View changes

move followup section next to issues blocked by this on

chrishks’s picture

Issue summary: View changes

Updated issue summary.

chrishks’s picture

Issue summary: View changes

Updated issue summary with additional follow-ups from to-dos

chrishks’s picture

Issue summary: View changes

Fix closing

  • tag
  • chrishks’s picture

    Issue summary: View changes

    Fix closing

  • tag
  • chrishks’s picture

    Issue summary: View changes

    Updated issue summary with follow-up from code to-do

    chrishks’s picture

    Issue summary: View changes

    Updated issue summary with follow-ups from to-dos

    YesCT’s picture

    @chrishks and @Ryan Weal and @hardik.patel99 helped to open followups, and update the issue summary.

    We covered some follow-ups that were listed in the issue summary,
    some from recent comments from reviewers,
    and all the @todo's in the code.

    Follow-ups done!

    Some of these might be duplicates and could use more detail. We can sort that out later. Some of them make very nice novice issues with a little bit more detail and guidance. (... so ... dont just do them when adding that guidance) :)

    das-peter’s picture

    @YesCT Awesome! Thank you guys!
    @plach Thanks for the re-roll!

    das-peter’s picture

    Issue summary: View changes

    Updated issue summary with follow-up from code to-do

    YesCT’s picture

    Issue summary: View changes

    add the followup to make properties translatable via the ui

    plach’s picture

    Issue summary: View changes

    Updated issue summary.

    fago’s picture

    Status: Needs review » Reviewed & tested by the community

    I had another look at it, so here is a nitpick:

    +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -22,6 +23,13 @@ class BookManager {
    +   * Entity manager Service Object.
    

    service object should be lowercase I guess.

    Overall, I think the patch is ready, performance concerns have been addressed and everything left has follow-ups. So let's move on with this!

    alexpott’s picture

    Title: Refactor node properties to multilingual » Change notice: Refactor node properties to multilingual
    Status: Reviewed & tested by the community » Active
    Issue tags: +Needs change record, +needs celebration video
     _________________________________________
    / Committed and pushed to 8.x. Thanks!    \
    | Thanks plach, das-peter, Schnitzel,     |
    | dawehner, YesCT, attiks, Berdir, Gábor |
    | Hojtsy, Soul88, Carsten Müller. You    |
    \ are awesome!                            /
     -----------------------------------------
            \   ^__^
             \  (oo)\_______
                (__)\       )\/\
                    ||----w |
                    ||     ||
    

    Committed 477c06c and pushed to 8.x. Thanks!

    plach’s picture

    Issue tags: -needs celebration video
    Schnitzel’s picture

    w000t! Awesome. Super happy :) thanks. To all who helped :)

    plach’s picture

    Issue tags: -sprint

    Happily no longer on sprint :)

    plach’s picture

    Status: Active » Needs review

    I don't think this needs an explicit change notice: there is no developer-facing API change. This is just a conversion to the standard announced in Added multilingual support to the standard entity schema. I mentioned this issue there.

    Gábor Hojtsy’s picture

    @plach: I think this is a HUGE change in itself that should be documented to communicate it. There are likely people directly querying things still.

    plach’s picture

    Right, we probably want to add a paragraph about querying in the generic change notice.

    Anonymous’s picture

     __________________________________
    / This is amazing! You guys are so \
    \ awesome.                         /
     ----------------------------------
            \   ^__^
             \  (**)\_______
                (__)\       )\/\
                 U  ||----w |
                    ||     ||
    
    Gábor Hojtsy’s picture

    Added http://drupal.org/node/2004750 as a short change notice for this. We should add some node query examples in the generic one I think. That should cover it I believe.

    jibran’s picture

    Status: Needs review » Needs work

    I think change notice should explain node_field_data and node_field_revision.

    plach’s picture

    Do you mean the new one?

    jibran’s picture

    Yes

    Gábor Hojtsy’s picture

    @jobran: the specific change notice for this issue links to the generic change notice at https://drupal.org/node/1722906 which explains these tables. What kind of explanation do you miss?

    jibran’s picture

    Status: Needs work » Needs review

    How about something like this?

    • Similarly to {entity} base table {node} base table only holds
      | nid*| uuid | vid | type | langcode | tnid | translate
      Primary key: nid
    • New {node_field_data} and {node_field_revision} are introduced for property data per language and for node revision property data per language similar to {entity_field_data} and {entity_field_revision}.
      | nid* | vid* | langcode*| default_langcode | title| uid | status | created | changed | comment | promote | sticky
      Primary key: nid + vid + langcode

    For further details read Added multilingual support to the standard entity schema.

    jibran’s picture

    Title: Change notice: Refactor node properties to multilingual » Refactor node properties to multilingual
    Status: Needs review » Fixed
    Issue tags: -Needs change record

    Thanks for the update :).

    plach’s picture

    I updated the general change notice with a paragraph on querying. We should be done now.

    Thanks everyone here!

    dcrocks’s picture

    There were a number of recent problems that prevented D8 being installed on SQLite. This patch introduced what is hopefully the last one. The 2 new tables, node_field_data and node_field_revisions have both a 'type' => 'serial' column and composite primary keys, a combination that can't be handled by the SQLite driver. #1998366: [meta] SQLite is broken identified and evolved thru a number of problems while installing D8 on SQLite and discusses the issues introduced by this patch towards the end. It is easily fixed to work in a 'monolingual' environment, but that's not the desired result. Any insight would be appreciated.

    Berdir’s picture

    Berdir’s picture

    Berdir’s picture

    Issue summary: View changes

    update api changes section.

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

    Anonymous’s picture

    Issue summary: View changes

    added follow ups from aboutn #326

    plach’s picture