| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | node system |
| Category: | task |
| Priority: | critical |
| Assigned: | plach |
| Status: | needs review |
| Issue tags: | budapest2012, D8MI, feature freeze, language-content, language-content-property, Needs manual testing, Needs profiling, sprint, translation editorial workflow, Twig, Usability, VDC-integration |
Issue Summary
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 account 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 createdwas ~10% slower than
SELECT title FROM {node} WHERE status = 1 AND promote = 1 ORDER BY createdThe 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
(done) resolve dependencies this is postponed on. See drawing in #132- #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
- #1998416: Filter on the desired node status field language and just fall back to the default language.
Redefine the business logic to take into account multilingual properties and adjust the related queries.- Review/test the current patch
Follow-ups
- Performance improvements: there is not much room for optimization in this issue, however #1497374: Switch from Field-based storage to Entity-based storage, the contextual conversion of entity queries to EFQ2 and #1498720: Make the default SQL storage controller automatically generate tables for every defined entity type open up for some interesting scenario: once the SQL storage is under the full control of the entity system we can automatically alter primary keys and queries depending on whether we are in a monolingual or multilingual environment. We have a couple of recurring patterns to switch from a monolingual query to a multilingual one, which should not be hard to automate. This would let us confine the performance regression to multilingual environments, which should be a fair compromise.
- Contextual information: some use cases do not support multilingual scenarios yet, as they would require a contextual information about the language to act on. Solving this is out of the scope of this issue and should be done when performing the conversion to EFQ2 exploiting the upcoming #1810370: Entity Translation API improvements.
- Views integration: views are still using
{node}as base table, which is not ideal for performance, since queries cannot use the field data indexes. It should be possible to solve this while working on a proper integration between Views and EFQ2. If Views relies on EFQ2 to query entities then the entity storage controller can automatically generate the most appropriate queries, since EFQ2 already supports data tables.
Aside from the perfomance issues, we need to consider that as soon as we start translating nodes, we will have a row for each translation in the result for every view sorting or joining on node fields. It would probably make sense to have an implicit filter selecting only rows from the data table withdefault_langcode = 1, if no explicit language filter is defined. Again this would be taken care of by EFQ2 if we get proper integration with Views.
User interface changes
None
API changes
None foreseen at the moment. The changes should concern only node storage.
Issues blocked by this one
- #1952044: Upgrade path for legacy content translation module data
- #1952062: Remove legacy translation module in favor of content translation
- others?
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.
Comments
#1
...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)
#2
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.
#3
What to do with title? should this issue postponed for #1188394: Make title behave as a field (again)
#4
IMO, we should not postpone this on #1188394: Make title behave as a 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.
#5
One issue that will definitely conflict with this one is #1528028: Have the node_revision table actually store all the information in the node table, 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).
#6
@catch: I don't think we even remotely have the bandwith to take on #1528028: Have the node_revision table actually store all the information in the node table 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.
#7
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:
<?php'foreign keys' => array(
'node_revision' => array(
'table' => 'node_revision',
'columns' => array('vid' => 'vid'),
),
?>
maybe `node_translation`should point on `node_revision_translation`?
#8
Another issue with this approach - schema could be altered but current implementation relay onto un-altered schema values
#9
@andypost: can you elaborate on that?
#10
I've slightly changed the patch
#11
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.
#12
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.
#13
#14
Retitling
#15
#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".
#16
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
#17
I'll start working on this soon. If anyone else wishes to take it meanwhile, feel free.
#18
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.
#19
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:
nid
uuid
vid
type
langcode
nid
uuid
vid
type
langcode
title
uid
status
created
changed
comment
promote
sticky
tnid
translate
nid
uuid
vid
type
langcode
title
uid
status
created
changed
comment
promote
sticky
tnid
translate
log
#20
According to #1498634: [meta] Define revision/translation SQL schema for core entities (no patch)
uuidandtypeshould not be repeated in the data and revision tables. Probably alsotnidandtranslateshould 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.#21
Okay, I'll move
uuid,type,tnidandtranslate.What about the
createdfield? This seems to be stateless too.Further I forgot to add the field
timestamptonode_property_data_revisionin the above list. And I thought about rename this field tochanged, does that make sense?Another thing I'm unsure about is, if it is okay to rename
node_revisiontonode_property_data_revision?#22
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?
#23
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.#24
Berdir just told me that the handling of the
data tableis covered inDrupal\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
#25
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
DatabaseStorageControllerNGworking with the data table.#26
The last submitted patch, drupal-node-properties-multilingual-1498674-25.patch, failed testing.
#27
Damn, silly copy paste error. Let's try again.
#28
The last submitted patch, drupal-node-properties-multilingual-1498674-27.patch, failed testing.
#29
Silly me, I should commit before I create the patch :|
#30
The last submitted patch, drupal-node-properties-multilingual-1498674-27.patch, failed testing.
#31
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...
#32
@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 :/
#33
Okay, then
creationgoes innode_property_dataand<code>node_property_data_revision.Yes and yes.
$node->nidbecomes$node->nid->valuewith 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.
#34
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.
#35
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\DatabaseStorageControllerandcore/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?
#36
The last submitted patch, drupal-node-properties-multilingual-1498674-35.patch, failed testing.
#37
@das-peter: I don't think anyone's opened an issue for converting Node to EntityNG yet. Go for it.
#38
Here comes the next version.
Interdiff needed?
#39
Will have a look to this later today.
#40
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:
default_langcodecolumn. See the test entity and the related storage controller for details. Moreover the revision table should be namednode_property_revision(see the Field API tables).default_langcode = 1condition, in other cases an explicit language condition would be needed.#41
@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.
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.
#42
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?)
#43
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.
#44
Had the same discussion with berdir in IRC. Let's wait with the EFQ change - this will keep the review a bit easier too.
#45
Updated the issue summary.
#46
Updated patch:
ToDo: Update query logic where necessary.
#47
Didn't review the patch yet. However rules of thumb to convert queries:
#48
Added "handling" of
default_langcodeproperty.#49
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.
#50
I would be happy to change them. And I think
examplewould be a good table name. If there are no objections I'll change that.*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
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.
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.
#51
The last submitted patch, drupal-node-properties-multilingual-1498674-50.patch, failed testing.
#52
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.txtshows which queries I changed after simply changing the table names / adding necessary joins.ReExp-Patterns I used to search for related queries:
(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)))(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)#53
The last submitted patch, drupal-node-properties-multilingual-1498674-52.patch, failed testing.
#54
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. :|
#55
The last submitted patch, drupal-node-properties-multilingual-1498674-54.patch, failed testing.
#56
Books fixed.
#57
Yay, will look into this as soon as I can!
#58
#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 :)
#59
And here's the re-roll.
#60
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
EntityTestStorageControllerwill 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
EntityTestStorageControllerattaches 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 thepostSave()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
#61
I didn't update / extend the tests yet. This is just a small re-roll.
I'm not sure how much of that is applicable without going "NG".
Text adjusted.
Tried to change it to something less nodesque.
Removed space, changed description.
I've no qualified opinion on this.
Primary keys adjusted, but I don't think that will work, a
db_merge()looks more appropriate. What about usingdb_merge()indrupal_write_record()when primary keys are provided?And I've no qualified opinion about where this should be handled.
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.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.
Yes. Is it really too disturbing?
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 :|
Because this is the chance to get rid of this strange construct and the todo that says make log nullable.
Fixed.
Was copy pasted - now adjusted.
Fixed.
Yes, removed.
Fixed.
Changed to 10 - but as this is a purely storage based migration it should be quite performant, thus 10 looks very low to me.
Damn, debugging stuff left. Fixed.
We can. Changed.
Added an Exception with a message indicating what could be wrong.
Indeed, fixed.
I expect failing tests for this, because of the adjusted
drupal_write_record()inDatabaseStorageController::save()!#62
Thanks, can you post an interdiff?
#63
The last submitted patch, drupal-node-properties-multilingual-1498674-61.patch, failed testing.
#64
Sure :)
#65
I just tried to fix the
drupal_write_record()inDatabaseStorageController::save()Doing so I realized that we don't have
languageasentity keyin thehook_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
idKeyorrevisionKey.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 usedb_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 :
db_merge()what means building the whole schema object property mapping on our own (This mapping would be already done in the "NG" controller), ordrupal_write_record()to usedb_merge()Ideas, suggestions, solutions? Everything's welcome :)
#66
Another thing that will be ugly to handle is the
langcode/default_langcodenaming.Since the base table uses
langcodeas name for the default language, while the property tables usedefault_langcodewe'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:
<?php
$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_langcodein the base table as well?That said, how is
default_langcodehandled in the entity_info? Will the decision for the possible 'language' entity key apply to this naming too?#67
Because of lack of feedback I just try to go on with the ugly workaround for the
langcode/default_langcodeissue.Further I updated the storing mechanism of revisions and added some tests to check if properties / revisions are created language dependent.
#68
The last submitted patch, drupal-node-properties-multilingual-1498674-67.patch, failed testing.
#69
*blargh* note for myself: I've to check the code more careful after doing a merge. :D
Reordered and renamed the update hooks.
#70
#71
Sorry for being vacant, peter. I'll give this another pass tonight.
#72
The last submitted patch, drupal-node-properties-multilingual-1498674-69.patch, failed testing.
#73
@plach No problem :)
I've fixed wrong conditions, let's see if this works better.
#74
The last submitted patch, drupal-node-properties-multilingual-1498674-73.patch, failed testing.
#75
Removed debugging artefacts.
#76
I moved storing the data properties into the postSave hook. At least the ugliness is centralized now :P
Further I added the test class
NodePropertyMultilingualTestCaseto test the multilingual properties. The tests were extended too.#77
FYI - I created #1818556: Convert nodes to the new Entity Field API
#78
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.
#79
#76: drupal-node-properties-multilingual-1498674-76.patch queued for re-testing.
#80
The last submitted patch, drupal-node-properties-multilingual-1498674-76.patch, failed testing.
#81
@dawehner offered to take a look tonight at the views integration for this issue. Thanks a lot in advance :)
#82
+++ 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 drupal_write_record() calls with db_merge() and get rid of drupal_write_record().
+++ 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??
#83
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:
Once a route is set (for example the table names), the views parts is not a big issue in terms of complexity.
#84
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?
#85
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.
#86
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.
#87
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.
#88
This should fix a lot of the failing tests now, i runned the most obvious ones locally.
#89
The last submitted patch, core-1498674-88.patch, failed testing.
#90
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
#91
Things done by that patch:
Let's see how many tests are still broken.
#92
Add tag
#93
The last submitted patch, drupal-1498674-91.patch, failed testing.
#94
Fixed one test and lost hours in trying to debug the other one.
#95
The last submitted patch, drupal-1498674-94.patch, failed testing.
#96
Add feature freeze tag.
#97
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
#98
to stay tag has to be
#99
The last submitted patch, drupal-1498674-97.patch, failed testing.
#100
just an error with the hook_update_n() numbers
fixed and requeued for testing
#101
The last submitted patch, drupal-1498674-100.patch, failed testing.
#102
found some issues with the whole new plugin system.
fixxed them, let's see what the testbot means.
#103
The last submitted patch, drupal-1498674-102.patch, failed testing.
#104
fixxed two of the failed tests
#105
The last submitted patch, drupal-1498674-104.patch, failed testing.
#106
mhh these two fails should not happen, well at least they don't happen on my local.
let's make some ugly debugging.
#107
#108
The last submitted patch, drupal-1498674-106.patch, failed testing.
#109
more debugging
#110
The last submitted patch, drupal-1498674-109.patch, failed testing.
#111
#109: drupal-1498674-109.patch queued for re-testing.
#112
The last submitted patch, drupal-1498674-109.patch, failed testing.
#113
I'll work on a re-roll and have a look at those test faliures.
#114
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 :)
#115
The last submitted patch, node-property-1498674-115.patch, failed testing.
#116
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..
#117
#114: node-property-1498674-115.patch queued for re-testing.
#118
Elevating to critical as per #1831530: Entity translation UI in core (part 2).
#119
something is strange with the Testbot, reupload to force a new test.
#120
The last submitted patch, node-property-1498674-119.patch, failed testing.
#121
mhh NodeAdministration Test is still not fixed, trying new approach with using innerJoin, so no Distinct anymore.
#122
YAY! So here the patch withouth debug() (which I needed in case Testbot again does not like it =) )
#123
The last submitted patch, node-property-1498674-122.patch, failed testing.
#124
#122: node-property-1498674-122.patch queued for re-testing.
#125
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.
#126
The last submitted patch, node-property-1498674-122.patch, failed testing.
#127
Fix tagging
#128
Dear Testbot, it is not okay to remove tags ...
#129
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
entitytoentityNGconversion in general.#130
#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 ...
#131
Okay, next step is done.
To be able to do #1818556: Convert nodes to the new Entity Field API, we need:
#132
@das-peter created this dependency drawing of the related issues:
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).
#133
Remove from sprint, marked postponed.
#134
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.
#135
this issue is listed as part of an example of a concrete use case meta issue #1860522: [META] support (basis for a contrib) Translation Editorial Workflow (support for revisions for translations of entities)
#136
Now only postponed on #1818556: Convert nodes to the new Entity Field API, right?
#137
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.
#138
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
#139
#140
We can start again with this.
#141
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.
#142
due to #141 updated summary
#143
@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.
#144
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.
#145
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
logcolumn) 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
isDefaultRevisionproperty 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.
#146
Added #1952044: Upgrade path for legacy content translation module data, marked postponed on this.
#147
Ok, this one should fix the node admin page. Let's start testing this (not that I expect this to come out green ;).
#148
The last submitted patch, et-node-ml-1498674-147.patch, failed testing.
#149
Fixed comment installation.
#150
The last submitted patch, et-node-ml-1498674-149.patch, failed testing.
#151
The next step will be merging the latest pre-NG patch (#122), if possible.
#152
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.
#153
EFQ2 was specifically designed to be an easy transition from DBTNG, so I'd say yes, switch to it.
#154
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.
#155
Here is an attempt to (manually) merge #122. Hopefully we'll have less failures.
#156
The last submitted patch, et-node-ml-1498674-155.patch, failed testing.
#157
Fixed some tests.
#158
The last submitted patch, et-node-ml-1498674-157.patch, failed testing.
#159
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.
#160
The last submitted patch, et-node-ml-1498674-159.patch, failed testing.
#161
More fixes :)
#162
The last submitted patch, et-node-ml-1498674-161.patch, failed testing.
#163
This one provides a (supposedly) working upgrade path plus additonal test fixes.
#164
The last submitted patch, et-node-ml-1498674-163.patch, failed testing.
#165
Fixed NG storage controller revision handling plus additional tests.
#166
The last submitted patch, et-node-ml-1498674-165.patch, failed testing.
#167
Fixed more tests.
#168
The last submitted patch, et-node-ml-1498674-165.patch, failed testing.
#169
Fixed some node_revision occurences.
#170
The last submitted patch, et-node-ml-1498674-167.patch, failed testing.
#171
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.
#172
The last submitted patch, et-node-ml-1498674-171.patch, failed testing.
#173
More test fixes.
#174
The last submitted patch, et-node-ml-1498674-172.patch, failed testing.
#175
I have big plans to test this and do some code review before the next D8MI meeting... I'm super excited about this patch! :-)
#176
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.
#177
Wrong patch...
#178
The last submitted patch, et-node-ml-1498674-176.patch, failed testing.
#179
This should fix most failures. Still fighting against the last ones.
#180
The last submitted patch, et-node-ml-1498674-179.patch, failed testing.
#181
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?
#182
The last submitted patch, et-node-ml-1498674-181.patch, failed testing.
#183
Translation set id is from the old translation module in core. It should be removed with the migration.
#184
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?
#185
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.)
#186
The last submitted patch, et-node-ml-1498674-185.patch, failed testing.
#187
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
#188
(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 :)
#189
The last submitted patch, et-node-ml-1498674-188.patch, failed testing.
#190
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:
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.
Yep
Agreed. I think we should do what core does most often for every added line in the patch.
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.
Probably #1939994: [META] Complete conversion of nodes to the new Entity Field API.
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 :)
#191
The last submitted patch, et-node-ml-1498674-190.patch, failed testing.
#192
Tagging for D8MI.
#193
[wrong patch]
#194
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.
#195
Fixed the other failure meanwhile.
#196
Last try for tonight.
#197
The last submitted patch, et-node-ml-1498674-196.patch, failed testing.
#198
#196: et-node-ml-1498674-196.patch queued for re-testing.
#199
The last submitted patch, et-node-ml-1498674-196.patch, failed testing.
#200
Obviously such an issue could not come without a test failing only online :(
#201
An attempt to fix the failing test + some debug lines.
#202
The last submitted patch, et-node-ml-1498674-201.patch, failed testing.
#203
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.
#204
The last submitted patch, et-node-ml-1498674-203.patch, failed testing.
#205
#203: et-node-ml-1498674-203.patch queued for re-testing.
#206
The last submitted patch, et-node-ml-1498674-203.patch, failed testing.
#207
This should hopefully fix the last failure.
#208
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?
#209
I have an 8.x database with 600k real nodes, I'll try the upgrade path there.
Do we need additional upgrade path tests?
#210
This should go away as soon as we switch to efq2: can we just do the minimal work needed to keep things working?
That code is only moving around columns from
nodetonode_property_data: no data is actually altered/removed, AAMOFnode_revisionis retained.Sounds cool :)
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.
#211
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.
#212
Yeah, makes sense, I was taking for granted that it would be converted to something more D8-like ;)
#213
This should address #207.
Hm, we should really split this into a setter and a getter. I didn't notice this could be used also as a setter...
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->statusand 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 ontitleorcreated. I think it would be sensible to have an implicit filter selecting only rows from the data table withdefault_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 thetypecolumn 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.#214
This should handle node access better in the book manager.
#215
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!
#216
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
EntityTranslationobject for things to work properly but we can't pass it because only objects implementingEntityInterfaceare 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:
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:
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.
#217
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.
#218
+ // 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?
#219
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.
#220
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.
#221
The last submitted patch, et-node-ml-1498674-220.patch, failed testing.
#222
This should fix the failure in #220.
#223
Rerolled after the latest commits.
#224
This is replicating the
typecolumn in thenode_field_datatable 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.
#225
The last submitted patch, et-node-ml-1498674-224.patch, failed testing.
#226
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.
#227
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.
#228
Other query updates + reverted conversion to EFQ2 for the node admin page.
#229
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.
#230
The last submitted patch, et-node-ml-1498674-229.patch, failed testing.
#231
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.
#232
#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.
#233
+++ 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.
#234
Thanks, Cathy. Anyway, those TODOs are meant to be discussed/resolved here :)
#235
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
#236
@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:
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)
=====
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.
#237
@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.
#238
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.
#239
(attaching the other db dump here due to upload limits)
@fago:
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().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.
Ok, but can we keep the fix here until the other issue is committed? Otherwise tests won't pass.
@YesCT:
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.
#240
Here's the module I used to microbenchmark multilingual queries. Results are in the code comments.
#241
@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)?#242
@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?
#243
Makes sense to me, here is a new patch.
#244
Based on the ab tests, this looks like not necessarily a performance degradation? Eg. comparing the 3rd vs. 3rd results in the two tests.
#245
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.
#246
#247
@plach the performance numbers are a bit concerning, could you post some of the EXPLAIN results here?
#248
@catch:
I'll do ASAP. However do you think the outlined plan to limit the perfomance degradation to multilingual environments is viable?
#249
@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.
#250
#243: et-node-ml-1498674-243.patch queued for re-testing.
#251
The last submitted patch, et-node-ml-1498674-243.patch, failed testing.
#252
Here's a reroll.
#253
Sorry, the last query in the fourth sample was wrong. Here is the correct version.
#254
+++ 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().
#255
@Berdir:
Thanks! This should take care of #254.
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.
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.
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.
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.
#256
Mmh, weird, it seems this no longer needs the EntityNG fix...
#257
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_datatimes. 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_dataand notnode_property_date#258
This takes care of #257, thanks!
#259
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
#260
Marking as RTBC, unless somebody thinks some of the @todo's should be addressed in this issue
#261
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.
#262
We need to extensively profile this patch. Quoting the issue summary
#263
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.
#264
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`);
#265
Some more tests (2500 nodes in 2 languages) using
100 runs each
0::0.215742s - SELECT COUNT(*) FROM {node_field_data} n WHERE n.promote = 1 AND n.status = 11::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
#266
Thanks everybody for reviewing. Here is a new one incorporating suggestions from #261 and #263.
#267
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.
#268
Wrt #259 I think it's fair to introduce these @todos because most of them would be OT here. The only exception is:
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.
#269
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`);
#270
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.
#271
#272
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.
#273
I'm going to add a D8 -> D8 upgrade today to test with the same data.
#274
New results using the same data on a standard Drupal 8 install with 2 languages, 50 nodes (5 runs):
<?phpdrupal_flush_all_caches();
views_page('frontpage', 'page_1');
?>
In short: wall time +1%
Details (# calls):
Details (mem use):
#275
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).
#276
Working on #275.
#277
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.