Work is ongoing in the D8MI - Entity Translation sandbox.
Problem/Motivation
Currently node non-configurable fields are not translatable, since the node schema only allows for a single record for each node. This is a big blocker for the D8MI initiative since we want to make all entities translatable through the upcoming #1188388: Entity translation UI in core and remove the node translation model from core.
Proposed resolution
Refactor the node schema to match the new standard described in Added multilingual support to the standard entity schema.
Some documentation examples used {node}
but were really generic, so we use {example}
instead. See @Berdir's feedback in #49.
Implementation details
Adopting the new schema standard required, among the rest, to refactor the DatabaseStorageController
to properly take into consideration revision translation. On node load we no longer join on the revision table, instead we attach all the field data by subsequently querying it, since field values may be multilingual and thus involve multiple records.
The (internal) refactoring of the storage controller allows for splitting the node table into two separate {node}
and {node_field_data}
tables. All the field values have been moved into the latter, thus gaining the ability to have a different value per language. Complying to the standard required to make all the field values revisable: the revision table has been renamed to {node_field_revision}
and all the missing columns were added to it. For instance now we can track the node authoring date per revision and have a different value per-language.
All the queries on the {node}
table has been updated to act (generally) on the {node_field_data}
table instead. We needed also to adapt the various queries to multilingual scenarios: in some cases language-agnostic queries were fine (e.g. count queries). In other cases it was ok to just take the default language into account (e.g. when retrieving a set of nids to load the corresponding nodes subsequently). In many cases a contextual information about the language to filter on would be needed, but such information is not available yet so these queries were just adapted to use the entity language. We will fix them after #1810370: Entity Translation API improvements is done. Updated queries were analyzed through MySQL explain to find out the most efficient alternative. Also microbenchmarks results were used to confirm the choice.
Lastly all views definitions were updated to use the {node_field_data}
table where necessary.
An (apparently) working upgrade path has also been provided. Manual tests were performed and automated tests are green.
Performance analysis
A series of benchmarks on a fresh installation with 1000 nodes and ~500 comments was run on a sample of three common pages: the front page (50 nodes shown), the node administration page and the comment administration page (where node titles are shown). We had ~6%, ~9% and ~2% of performance penalty, respectively (head logs, patch logs | head db dump, patch db dump).
Microbenchmarking sessions were run for each updated query, showing analogous results: multilingual queries introduce performance penalties varying from nearly 0 to 15%. All the mb sessions were run to determine the most performant multilingual query, but it should be noted that the simple switch to the new schema introduces a performance penalty. This happens even for "monolingual" queries, i.e. considering the scenario of monolingual sites, where we have exactly one row for each node in the data table. For instance:
SELECT title FROM {node_field_data} WHERE status = 1 AND promote = 1 ORDER BY created
was ~10% slower than
SELECT title FROM {node} WHERE status = 1 AND promote = 1 ORDER BY created
The main culprit here seems to be the composite primary key, in fact dropping it and setting just nid
as primary key makes the monolingual query above as performant as in the current HEAD.
Remaining tasks
(done) resolve dependencies this is postponed on. See drawing in #132Redefine the business logic to take into account multilingual properties and adjust the related queries.Review/test the current patch
User interface changes
None
API changes
No API changes. There are changes to queries and now it is even more important that others us EFQ and not direct database queries. We have to use EFQ to ensure translatability.
Issues blocked by this one
- #1952044: Migration path for legacy content translation module data
- #1952062: Remove legacy translation module in favor of content translation
- others?
Follow-ups
- #2004626: Make non-configurable field translation settings available in the content language settings
- #2004302: Automaticly generate Performance improvements needed after Refactor node properties to multilingual
- #2004330: [META] Convert queries to entity field query v2
- #2004328: [Meta] Views integration with EFQ2
- #2004352: Tidy up code following multilingual node properties patch
- #2004362: Remove special property language code to handle comments once comments are field
- #2004358: Use multiple language insertions in drupal_write_record to revisionTabe to improve performance.
- #2004356: Revisit node status condition in statistics_title_list()
- #2004368: Introduce node_mass_delete() or make node_mass_update() more flexible
- #2004376: use $this->base_table and remove an if
- #2004372: Refactor node query to use desired language rather than default language while providing fallback to default
- #2004378: Refactor query to use desired language rather than default language while providing fallback to default
- #1998416: Filter on the desired node status field language and just fall back to the default language.
- #1998366: [meta] SQLite is broken
- #2015277: Reduce the number of indexes on the node_field_* tables
- #2015303: node_update_8017() can loop forever.
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.
Comment | File | Size | Author |
---|---|---|---|
#303 | et-node-ml-1498674-303.patch | 203.73 KB | plach |
#301 | et-node-ml-1498674-301.patch | 203.61 KB | das-peter |
#301 | interdiff-1498674-299-301-do-not-test.diff | 1.04 KB | das-peter |
#299 | et-node-ml-1498674-299.patch | 202.58 KB | das-peter |
#299 | interdiff-1498674-296-299-do-not-test.diff | 1.35 KB | das-peter |
Comments
Comment #1
klonos...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)
Comment #2
Gábor HojtsyAs 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.
Comment #3
andypostWhat to do with title? should this issue postponed for #1188394: Make title behave as a configurable, translatable field (again)
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedIMO, we should not postpone this on #1188394: Make title behave as a configurable, translatable field (again). {node} and {node_revision} have title at this time. Therefore, {node_translation} and {node_revision_translation} should have title in any patch posted here. If the other issue ends up getting committed first and removes title from {node} and {node_revision}, then patches here can be rerolled accordingly. OTOH, if this issue lands first, then patches on that issue can be rerolled to remove title from 4 tables instead of 2. Seems to me that the work needed on either issue to adjust for the other one is tiny compared to the benefit of allowing both to proceed independently.
Comment #5
catchOne issue that will definitely conflict with this one is #1528028: Add tests for reverting revisions where revision_uid and uid differ, I'm not sure if I can get away with it but it'd be really nice to fix the node_revisions schema before rather than after duplicating it (/me ducks).
Comment #6
Gábor Hojtsy@catch: I don't think we even remotely have the bandwith to take on #1528028: Add tests for reverting revisions where revision_uid and uid differ to move this one forward as we are pretty much behind on property/entity translation work.
@all: Here is a quick patch to get us started. I think this is pretty simple and easily updated later if the node or revisions schema changes. The regular schema has the schemas as copies, just the description for the table is different. The update functions need to replicate the whole thing. I looked at the properties, and we plan to loose tnid on nodes later (but we have a long way to go to be able to replicate that functionality first) and changing the type for translation sounds very dangerous. Otherwise all properties on node and node_revision seemed to make complete sense to allow for translation, such as the author or title or sticky but.
Comment #7
Soul88Hello, 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:
maybe `node_translation`should point on `node_revision_translation`?
Comment #8
andypostAnother issue with this approach - schema could be altered but current implementation relay onto un-altered schema values
Comment #9
Gábor Hojtsy@andypost: can you elaborate on that?
Comment #10
Soul88I've slightly changed the patch
Comment #11
Gábor HojtsyThere 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.
Comment #12
Gábor HojtsyThere 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.
Comment #13
plachComment #14
plachRetitling
Comment #15
effulgentsia CreditAttribution: effulgentsia commented#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".
Comment #16
Gábor HojtsyWe 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
Comment #17
plachI'll start working on this soon. If anyone else wishes to take it meanwhile, feel free.
Comment #18
das-peter CreditAttribution: das-peter commentedAttached 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.
Comment #19
das-peter CreditAttribution: das-peter commentedOh, 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
Comment #20
plachAccording to #1498634: [meta] Define revision/translation SQL schema for core entities (no patch)
uuid
andtype
should not be repeated in the data and revision tables. Probably alsotnid
andtranslate
should be left out of the revision table: they aren't there right now and are likely to be dropped if we will be able to remove node translation from core.Comment #21
das-peter CreditAttribution: das-peter commentedOkay, I'll move
uuid
,type
,tnid
andtranslate
.What about the
created
field? This seems to be stateless too.Further I forgot to add the field
timestamp
tonode_property_data_revision
in 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_revision
tonode_property_data_revision
?Comment #22
plachHonestly 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?
Comment #23
das-peter CreditAttribution: das-peter commentedI'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.Comment #24
das-peter CreditAttribution: das-peter commentedBerdir just told me that the handling of the
data table
is 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
Comment #25
das-peter CreditAttribution: das-peter commentedHmm, the new Entity Property API is interesting. Let's see what fails...
If this patch really should go in, I'll split it in smaller junks so a review is possible!
Currently it contains parts of #1778178: Convert comments to the new Entity Field API and some custom adjustments make
DatabaseStorageControllerNG
working with the data table.Comment #27
das-peter CreditAttribution: das-peter commentedDamn, silly copy paste error. Let's try again.
Comment #29
das-peter CreditAttribution: das-peter commentedSilly me, I should commit before I create the patch :|
Comment #31
das-peter CreditAttribution: das-peter commentedI'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...
Comment #32
Gábor Hojtsy@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 :/
Comment #33
das-peter CreditAttribution: das-peter commentedOkay, then
creation
goes innode_property_data
and<code>node_property_data_revision
.Yes and yes.
$node->nid
becomes$node->nid->value
with EntityNG so why not switch to<code>$node->id()
right away?And it will become a monster, but I think I'll be able to split it into reviewable junks.
Comment #34
Gábor HojtsyIf 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.
Comment #35
das-peter CreditAttribution: das-peter commentedAs with Gabor discussed in IRC here's a patch that just covers the absolute minimum of changes. Attention: This still includes changes in the
Drupal\Core\Entity\DatabaseStorageController
andcore/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?
Comment #37
effulgentsia CreditAttribution: effulgentsia commented@das-peter: I don't think anyone's opened an issue for converting Node to EntityNG yet. Go for it.
Comment #38
das-peter CreditAttribution: das-peter commentedHere comes the next version.
Interdiff needed?
Comment #39
plachWill have a look to this later today.
Comment #40
plachSorry, 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_langcode
column. 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 = 1
condition, in other cases an explicit language condition would be needed.Comment #41
das-peter CreditAttribution: das-peter commented@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.
Comment #42
BerdirAgreed 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?)
Comment #43
plachI 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.
Comment #44
das-peter CreditAttribution: das-peter commentedHad the same discussion with berdir in IRC. Let's wait with the EFQ change - this will keep the review a bit easier too.
Comment #44.0
plachUpdated issue summary
Comment #44.1
plachUpdated issue summary.
Comment #44.2
plachUpdated issue summary.
Comment #44.3
plachUpdated issue summary.
Comment #45
plachUpdated the issue summary.
Comment #46
das-peter CreditAttribution: das-peter commentedUpdated patch:
ToDo: Update query logic where necessary.
Comment #47
plachDidn't review the patch yet. However rules of thumb to convert queries:
Comment #48
das-peter CreditAttribution: das-peter commentedAdded "handling" of
default_langcode
property.Comment #49
Berdiris there a reason you didn't send the patches to the testbot?
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.
Same thing here.
Unrelated changes?
When fixing it, fix it correctly, these should be protected. Or leave it alone ;)
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.
Comment #50
das-peter CreditAttribution: das-peter commentedI would be happy to change them. And I think
example
would be a good table name. If there are no objections I'll change that.*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.
Comment #52
das-peter CreditAttribution: das-peter commentedFixed issue with storing revisions - someone should take a look at this :|
Hope we can switch to the revision enhancement by berdir soon #1723892: Support for revisions for entity save and delete operations
I started to convert queries, but it doesn't feel right. I guess I make more mess than solving a single thing.
The file
query_diff-1498674-50-52.txt
shows which queries I changed after simply changing the table names / adding necessary joins.ReExp-Patterns I used to search for related queries:
(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)
Comment #54
das-peter CreditAttribution: das-peter commentedOuch, 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. :|
Comment #56
das-peter CreditAttribution: das-peter commentedBooks fixed.
Comment #57
plachYay, will look into this as soon as I can!
Comment #58
das-peter CreditAttribution: das-peter commented#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 :)
Comment #59
das-peter CreditAttribution: das-peter commentedAnd here's the re-roll.
Comment #60
plachI couldn't review every single query yet, but it seems to me they are not dealing with multilingual properties yet. It might make sense to leav the tougher ones to dedicated follow-ups but I don't think every query here deserves this. Also we need new or updated upgrade path tests, because the current update functions are broken but tests pass.
Moreover I think a bit more consistency with the
EntityTestStorageController
will help the future merge of all the property stuff into the base storage controller.These examples are node-related: node-specific queries should be kept or the textual description updated as well.
Can we make this example even less nodesque? Title and body smell bad.
Double space before 'has'. Also: there is nothing in code that explicitly refers to multilingual stuff. Not sure whether the comment should mention it.
The current
EntityTestStorageController
attaches data values in a second pass avoiding the join. Whether this approach is more performant is debatable, but it mimicks the current field attach process, and lets the door open for more unification at storage level.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.Can we use
node_load_multiple()
here?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?
Unrelated?
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.
Why do we need this change?
Missing space after comma.
Aligning stuff vertically is discouraged AFAIK.
Should be {node_property_revision}.
Useless leftover?
Missing table prefixing braces.
Normally we use a step of 10 or so. Five thousands sounds definitely too high.
Only one?
Can we call these just old instead of deprecated? They are going to be dropped after all.
I guess we should fail and notify if things go wrong here.
Should be $deprecated_index
Comment #61
das-peter CreditAttribution: das-peter commentedI 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()
!Comment #62
plachThanks, can you post an interdiff?
Comment #64
das-peter CreditAttribution: das-peter commentedSure :)
Comment #65
das-peter CreditAttribution: das-peter commentedI just tried to fix the
drupal_write_record()
inDatabaseStorageController::save()
Doing so I realized that we don't have
language
asentity key
in 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:First I don't know if there's really a language and second if there's one I don't know if it's really called langcode.
Looks like we should add language to
hook_entity_info()
and mark it as optional - similar to uuid.Then we handle it like
idKey
orrevisionKey
.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 :)
Comment #66
das-peter CreditAttribution: das-peter commentedAnother thing that will be ugly to handle is the
langcode
/default_langcode
naming.Since the base table uses
langcode
as name for the default language, while the property tables usedefault_langcode
we've to switch the data in the record we want to save between saving it into the base table and saving it into the property table.Result is something like:
This looks very ugly :| Was there a reason why we don't use
default_langcode
in the base table as well?That said, how is
default_langcode
handled in the entity_info? Will the decision for the possible 'language' entity key apply to this naming too?Comment #67
das-peter CreditAttribution: das-peter commentedBecause of lack of feedback I just try to go on with the ugly workaround for the
langcode
/default_langcode
issue.Further I updated the storing mechanism of revisions and added some tests to check if properties / revisions are created language dependent.
Comment #69
das-peter CreditAttribution: das-peter commented*blargh* note for myself: I've to check the code more careful after doing a merge. :D
Reordered and renamed the update hooks.
Comment #70
das-peter CreditAttribution: das-peter commentedComment #71
plachSorry for being vacant, peter. I'll give this another pass tonight.
Comment #73
das-peter CreditAttribution: das-peter commented@plach No problem :)
I've fixed wrong conditions, let's see if this works better.
Comment #75
das-peter CreditAttribution: das-peter commentedRemoved debugging artefacts.
Comment #76
das-peter CreditAttribution: das-peter commentedI moved storing the data properties into the postSave hook. At least the ugliness is centralized now :P
Further I added the test class
NodePropertyMultilingualTestCase
to test the multilingual properties. The tests were extended too.Comment #77
fagoFYI - I created #1818556: Convert nodes to the new Entity Field API
Comment #78
Gábor HojtsyViews 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.
Comment #79
Gábor Hojtsy#76: drupal-node-properties-multilingual-1498674-76.patch queued for re-testing.
Comment #81
Gábor Hojtsy@dawehner offered to take a look tonight at the views integration for this issue. Thanks a lot in advance :)
Comment #82
fagoWe 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' ?
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.
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?
Shouldn't that loop over all available languages of the node?
Could we just use db_merge() here? See #1774104: Replace all trivial drupal_write_record() calls with db_merge().
default_langcode is no valid property. It's langcode.
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??
Comment #83
dawehnerWe 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.
Comment #84
Gábor HojtsyWell, 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?
Comment #85
das-peter CreditAttribution: das-peter commentedI'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.
Comment #86
fagoomg, 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.
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.
Comment #87
dawehnerI just started with working on updating the views integration, so just posting an interdiff to the patch in #77.
More work will come tomorrow.
Comment #88
dawehnerThis should fix a lot of the failing tests now, i runned the most obvious ones locally.
Comment #90
das-peter CreditAttribution: das-peter commentedAs 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
Comment #91
dawehnerThings done by that patch:
Let's see how many tests are still broken.
Comment #92
dawehnerAdd tag
Comment #94
dawehnerFixed one test and lost hours in trying to debug the other one.
Comment #96
Gábor HojtsyAdd feature freeze tag.
Comment #97
Schnitzel CreditAttribution: Schnitzel commenteddid 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
Comment #98
Schnitzel CreditAttribution: Schnitzel commentedto stay tag has to be
Comment #100
Carsten Müller CreditAttribution: Carsten Müller commentedjust an error with the hook_update_n() numbers
fixed and requeued for testing
Comment #102
Schnitzel CreditAttribution: Schnitzel commentedfound some issues with the whole new plugin system.
fixxed them, let's see what the testbot means.
Comment #104
Schnitzel CreditAttribution: Schnitzel commentedfixxed two of the failed tests
Comment #106
Schnitzel CreditAttribution: Schnitzel commentedmhh these two fails should not happen, well at least they don't happen on my local.
let's make some ugly debugging.
Comment #107
Schnitzel CreditAttribution: Schnitzel commentedComment #109
Schnitzel CreditAttribution: Schnitzel commentedmore debugging
Comment #111
Schnitzel CreditAttribution: Schnitzel commented#109: drupal-1498674-109.patch queued for re-testing.
Comment #113
BerdirI'll work on a re-roll and have a look at those test faliures.
Comment #114
BerdirOk. 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 :)
Comment #116
dawehnerEven 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..
Comment #117
Schnitzel CreditAttribution: Schnitzel commented#114: node-property-1498674-115.patch queued for re-testing.
Comment #118
Gábor HojtsyElevating to critical as per #1831530: Entity translation UI in core (part 2).
Comment #119
Schnitzel CreditAttribution: Schnitzel commentedsomething is strange with the Testbot, reupload to force a new test.
Comment #121
Schnitzel CreditAttribution: Schnitzel commentedmhh NodeAdministration Test is still not fixed, trying new approach with using innerJoin, so no Distinct anymore.
Comment #122
Schnitzel CreditAttribution: Schnitzel commentedYAY! So here the patch withouth debug() (which I needed in case Testbot again does not like it =) )
Comment #124
plach#122: node-property-1498674-122.patch queued for re-testing.
Comment #125
das-peter CreditAttribution: das-peter commentedEven 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.
Comment #127
das-peter CreditAttribution: das-peter commentedFix tagging
Comment #128
Fabianx CreditAttribution: Fabianx commentedDear Testbot, it is not okay to remove tags ...
Comment #129
das-peter CreditAttribution: das-peter commentedCurrent state from my perspective - IRC-Log:
My personal conclusion is that we should focus on #1818556: Convert nodes to the new Entity Field API - especially the decorator part. This will allow us to base the multilingual property code on a shared code base and we don't have to re-invent the wheel. Further this will help the
entity
toentityNG
conversion in general.Comment #130
Fabianx CreditAttribution: Fabianx commented#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 ...
Comment #131
das-peter CreditAttribution: das-peter commentedOkay, next step is done.
To be able to do #1818556: Convert nodes to the new Entity Field API, we need:
Comment #132
Gábor Hojtsy@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).
Comment #133
Gábor HojtsyRemove from sprint, marked postponed.
Comment #134
plachYes, 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.
Comment #135
YesCT CreditAttribution: YesCT commentedthis 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)
Comment #135.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #135.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary to highlight the issues this is postponed on in the remaining tasks, and link to the drawing of the issue dependencies.
Comment #136
Gábor HojtsyNow only postponed on #1818556: Convert nodes to the new Entity Field API, right?
Comment #137
plachYep, 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.
Comment #138
YesCT CreditAttribution: YesCT commentedplach, 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
Comment #139
plachComment #140
plachWe can start again with this.
Comment #140.0
plachUpdated issue summary take out repeat (reference to this issue itself)
Comment #141
plachI'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.
Comment #142
podarokdue to #141 updated summary
Comment #142.0
plachUpdated issue summary.
Comment #143
plach@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.
Comment #144
plachHere 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.
Comment #145
plachHere is a new attempt still working on save/load. This heavily refactors entity loading to take into account base/mul/rev/mulrev scenarios. In the latter case, that is when we have both the data and the revision table, the latter holds a record for each revision in each available language, hence we should not perform a join with the base table, as we might get multiple records for each entity. Moreover in the new standard the revision table fully replicates the data table schema with the addition of revision-specific metadata (the
log
column) that should be loaded anyway, thus we don't need to query the data table in this case.The attached patch keeps the base + revision join for monolingual entities. In multilingual scenarios it attaches property data from either the revision table or the data table depending on the entity revisability.
There is also a bit of futzing with the
isDefaultRevision
property which has not been converted to a field yet.Overall this does not look very clean to me. I think we should provide different storage helper classes for the various base/mul/rev/mulrev scenarios in #1497374: Switch from Field-based storage to Entity-based storage.
Comment #146
Gábor HojtsyAdded #1952044: Migration path for legacy content translation module data, marked postponed on this.
Comment #147
plachOk, this one should fix the node admin page. Let's start testing this (not that I expect this to come out green ;).
Comment #149
plachFixed comment installation.
Comment #151
plachThe next step will be merging the latest pre-NG patch (#122), if possible.
Comment #152
plachIn #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.
Comment #153
amateescu CreditAttribution: amateescu commentedEFQ2 was specifically designed to be an easy transition from DBTNG, so I'd say yes, switch to it.
Comment #154
plachI 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.
Comment #154.0
plachUpdated issue summary.
Comment #155
plachHere is an attempt to (manually) merge #122. Hopefully we'll have less failures.
Comment #155.0
(not verified) CreditAttribution: commentedremaining tasks to clarify that the dependencies are done
Comment #157
plachFixed some tests.
Comment #159
plachFixed 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.
Comment #161
plachMore fixes :)
Comment #163
plachThis one provides a (supposedly) working upgrade path plus additonal test fixes.
Comment #165
plachFixed NG storage controller revision handling plus additional tests.
Comment #167
plachFixed more tests.
Comment #169
plachFixed some node_revision occurences.
Comment #171
plachThis 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.
Comment #173
plachMore test fixes.
Comment #175
dsnopekI have big plans to test this and do some code review before the next D8MI meeting... I'm super excited about this patch! :-)
Comment #176
plachFixed 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.
Comment #177
plachWrong patch...
Comment #179
plachThis should fix most failures. Still fighting against the last ones.
Comment #181
YesCT CreditAttribution: YesCT commentedThis is just a little clean-up and some questions. Patch attached for these small nits. I'll do another read through after posting it.
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.
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.)
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.
wrap to 80 chars.
@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.
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"
maybe this has an issue?
was it #1778178: Convert comments to the new Entity Field API and is actually already done?
just @todo, not @todo:
http://drupal.org/node/1952062
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.
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.
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?
unrelated change.
Also, what is a "translation set id" I think it's just translation ID really?
Comment #183
Gábor HojtsyTranslation set id is from the old translation module in core. It should be removed with the migration.
Comment #183.0
Gábor Hojtsyadded section for issues blocked by this one. there might be more to add.
Comment #184
YesCT CreditAttribution: YesCT commentedupdated 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?
Comment #185
YesCT CreditAttribution: YesCT commentedoops. 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.)
Comment #187
YesCT CreditAttribution: YesCT commentedA 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
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
Comment #188
plach(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 :)
Comment #190
plachThis 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: 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 :)
Comment #192
Gábor HojtsyTagging for D8MI.
Comment #193
plach[wrong patch]
Comment #194
plachThis 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.
Comment #195
plachFixed the other failure meanwhile.
Comment #196
plachLast try for tonight.
Comment #198
plach#196: et-node-ml-1498674-196.patch queued for re-testing.
Comment #200
plachObviously such an issue could not come without a test failing only online :(
Comment #201
plachAn attempt to fix the failing test + some debug lines.
Comment #203
plachThis 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.
Comment #205
plach#203: et-node-ml-1498674-203.patch queued for re-testing.
Comment #207
plachThis should hopefully fix the last failure.
Comment #208
BerdirHaven't found much to complain...
$entity->isDefaultRevision($new_value) should work fine...
That's.... ugly :(
Maybe just do a node_load_multiple($nids) and then remove the node joins completely and just rely on the loaded node?
This is overlapping with the comment as field issue :(
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 :)
Changing this to example seems unecessary given that we keep the original and just change a few things?
Especially because it starts to get weird here ;) Why would example use node types :p
I guess this should either be fixed or removed if it's not necessary?
Comment #209
BerdirI have an 8.x database with 600k real nodes, I'll try the upgrade path there.
Do we need additional upgrade path tests?
Comment #210
plachThis 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
node
tonode_property_data
: no data is actually altered/removed, AAMOFnode_revision
is 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.
Comment #211
BerdirHm, 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.
Comment #212
plachYeah, makes sense, I was taking for granted that it would be converted to something more D8-like ;)
Comment #213
plachThis 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->status
and retrieving$node->label()
. I think this exemplifies well one of the remaining tasks of this issue: we need to figure out how to deal with multilingual scenarios for each of the updated queries. As a side issue we need to understand how to update node's views integration (hope @dawehner can help on this): as soon as we will start translating nodes, we will have a row for each translation in the result for every view sorting ontitle
orcreated
. 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 thetype
column in the data table to allow for more performant queries. I guess this logic would be a bit tricky to automate when moving to the entity storage, but probably it could be addressed together with indexes which are going to be tricky anyway.Comment #214
plachThis should handle node access better in the book manager.
Comment #215
Gábor HojtsyI 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!
Comment #216
plachI spent the last couple of days playing with ET and nodes: basic translation more or less works, but there is lot of stuff still breaking due to the current mess with partial node NG conversion, BC decorator quirks and NG storage controller fragility (we definitely need separate storage helpers for base/rev/mul/mulrev scenarios). However the main issue is with the current Entity Translation API that simply does not work both functionally and DX-wise: there are many places where we'd need an
EntityTranslation
object for things to work properly but we can't pass it because only objects implementingEntityInterface
are accepted.The attached patch + interdiff can be considered a proof of concept to show that this stuff is going to work properly once all the missing pieces are in place. We are simply not there yet.
Given this, I changed my mind about how we should proceed with this issue: since multilingual property storage is actually working, I think we should fix queries to account for multilingual properties and get this in ASAP. Then we would need to get the following stuff done:
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.
Comment #217
plachIf 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.
Comment #218
fagoWe call all entity properties fields now, so I guess we should use this terminology here as well?
Comment #219
dawehnerI went through the views related parts and it looks pretty solid so far. For sure this patch also needs manual testing.
Is there a reason why this piece of configuration is removed?
This lines sort of look wrong, but I didn't researched that.
Comment #220
plachHere 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.
Comment #222
plachThis should fix the failure in #220.
Comment #223
plachRerolled after the latest commits.
Comment #224
plachThis is replicating the
type
column in thenode_field_data
table to restore the lost indexes, as proposed in #213 and approved by @catch.I'm expecting some failure in views tests but identifying all of them would require too much time on my box.
Comment #226
plachThis 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.
Comment #227
plachI 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.
Comment #228
plachOther query updates + reverted conversion to EFQ2 for the node admin page.
Comment #229
plachUpdated/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.
Comment #231
plachThis 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.
Comment #232
YesCT CreditAttribution: YesCT commented#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.
Comment #233
YesCT CreditAttribution: YesCT commentedsince changing this line, let's make it a sentence.
{@inheritdoc}
#1906474: [policy adopted] Stop documenting methods with "Overrides …" is fixed now.
Q1:
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.
this looks like just delete. So:
// Delete to handle removed values.
use the standard for @todo
http://drupal.org/node/1354#todo
add type. string. in the new @param line.
since return line is new, add type to @return also.
Comment #234
plachThanks, Cathy. Anyway, those TODOs are meant to be discussed/resolved here :)
Comment #235
fagoImpressive work!
Why do we need to query revisions if we do not have a datatable?
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.
We've got a fixes for that in other issues as welkl, see #1957888: Exception when a field is empty on programmatic creation
Comment #236
YesCT CreditAttribution: YesCT commented@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.
Comment #237
Gábor Hojtsy@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.
Comment #237.0
plachupdated issue summary after re-reading through old comments. added remaining task about upgrade tests and explaination for why so many node-> example changes are in this issue.
Comment #238
plachUpdated 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.
Comment #239
plach(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.
Comment #240
plachHere's the module I used to microbenchmark multilingual queries. Results are in the code comments.
Comment #240.0
plachUpdated issue summary
Comment #240.1
plachUpdated issue summary.
Comment #240.2
plachUpdated issue summary.
Comment #240.3
plachUpdated issue summary.
Comment #240.4
plachUpdated issue summary.
Comment #240.5
plachUpdated issue summary.
Comment #240.6
plachUpdated issue summary.
Comment #240.7
plachUpdated issue summary.
Comment #241
plach@Gabor:
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)?Comment #242
Gábor Hojtsy@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?
Comment #243
plachMakes sense to me, here is a new patch.
Comment #243.0
plachUpdated issue summary.
Comment #244
Gábor HojtsyBased on the ab tests, this looks like not necessarily a performance degradation? Eg. comparing the 3rd vs. 3rd results in the two tests.
Comment #245
plachWell, 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.
Comment #246
plachComment #247
catch@plach the performance numbers are a bit concerning, could you post some of the EXPLAIN results here?
Comment #247.0
catchUpdated issue summary.
Comment #248
plach@catch:
I'll do ASAP. However do you think the outlined plan to limit the perfomance degradation to multilingual environments is viable?
Comment #248.0
plachUpdated issue summary.
Comment #249
plach@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.
Comment #250
Berdir#243: et-node-ml-1498674-243.patch queued for re-testing.
Comment #252
plachHere's a reroll.
Comment #253
plachSorry, the last query in the fourth sample was wrong. Here is the correct version.
Comment #253.0
plachUpdated issue summary.
Comment #254
BerdirWe 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.
We still need node access here I think, you just need to specify the table which should be used: "->addMetaData('base_table', 'book')"
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.
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.
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.
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.
Everything else was changed to $example but this still mentions node_get_type_label().
Comment #255
plach@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.
Comment #256
plachMmh, weird, it seems this no longer needs the EntityNG fix...
Comment #257
das-peter CreditAttribution: das-peter commentedI 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:
I guess the "npr" shortcut is a legacy from
node_property_data
times. Should we clean this up to "nfr"?How about using also "nfr" instead "nr" as shortcut here? Just for the sake of consistency.
Looks like search and replacement artifacts. I guess that should be
node_field_data
and notnode_property_date
Comment #258
plachThis takes care of #257, thanks!
Comment #258.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #259
attiks CreditAttribution: attiks commentedPatch 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
Comment #260
attiks CreditAttribution: attiks commentedMarking as RTBC, unless somebody thinks some of the @todo's should be addressed in this issue
Comment #261
dawehnerI 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.
Let's not change the plugin id.
The plugin_id should not change.
Comment #262
alexpottWe need to extensively profile this patch. Quoting the issue summary
Comment #263
BerdirNot 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.
Comment #264
attiks CreditAttribution: attiks commentedfirst improvement (just posting for reference)
Comment #265
attiks CreditAttribution: attiks commentedSome more tests (2500 nodes in 2 languages) using
100 runs each
Comment #266
plachThanks everybody for reviewing. Here is a new one incorporating suggestions from #261 and #263.
Comment #267
BerdirJust 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.
Comment #268
plachWrt #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.
Comment #269
attiks CreditAttribution: attiks commentedMost 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`);
Comment #270
attiks CreditAttribution: attiks commentedIncluded profiled runs of views_page('frontpage', 'page_1'); the content was not 100% identical, both sites had +100 nodes with comments in 2 languages.
Comment #271
attiks CreditAttribution: attiks commentedComment #272
plachThose numbers look a bit weird to me. How about performing the patch test with an upgraded db, i.e. with the very same data.
Comment #273
attiks CreditAttribution: attiks commentedI'm going to add a D8 -> D8 upgrade today to test with the same data.
Comment #274
attiks CreditAttribution: attiks commentedNew results using the same data on a standard Drupal 8 install with 2 languages, 50 nodes (5 runs):
In short: wall time +1%
Details (# calls):
Details (mem use):
Comment #275
Gábor HojtsyI 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).
Comment #276
plachWorking on #275.
Comment #277
BerdirOne 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.
Comment #278
BerdirTo explain what I meant, some examples:
Column "status" maps to "$entity->status->value", if nothing is specified, we default to ->value.
To use something else, name your database column something like "uid__target_id", that maps to "$entity->uid->target_id".
That also means we automatically support multiple primitive values for a field if we have "reference__entity_type" and "reference__entity_id", we can map that to "$entity->reference->entity_type" and "$entity->reference->entity_id".
Comment #279
plachWell, #268 makes sense to me but I don't think it belongs to this issue. Anyway, it probably would make more sense to just use "->value" when a field has a single column. Or we could have a 'default column' property in the field definition that defaults to
'value'
. The above proposal makes sense to me for multicolumned non-configurable fields.Comment #280
plachOk, let's see how many things break :)
Comment #282
plachThis might fix all failures.
Comment #283
attiks CreditAttribution: attiks commentedGood news and bad news
It's faster but with more memory overhead
I'll upload the xhprofs as well
Comment #284
attiks CreditAttribution: attiks commentedxhprofs
Comment #285
attiks CreditAttribution: attiks commentedmemory mystery solved, was caused by the in between cache clears
this is the same test with only a cc all before testing
Comment #286
plachSo this is making the front page faster? Hard to believe :)
Comment #287
attiks CreditAttribution: attiks commented#286 I double checked the profile files and it might be that something went wrong, I would love somebody else to profile this as well (preferably with and without cache clears in between)
Comment #288
plachI will tonight
Comment #289
Gábor HojtsyAmazing! we would ideally have data ASAP, so we can look art it and figure out next steps while we are on the same space-time continuum.
Comment #290
Gábor HojtsyAmazing! we would ideally have data ASAP, so we can look art it and figure out next steps while we are on the same space-time continuum.
Comment #291
carsonevans CreditAttribution: carsonevans commentedbbranches 519ffa9a30e84 et-node-ml-1498674-282.patch
=== 8.x..8.x compared (519ffa9a30e84..519ffafe96886):
ct : 40,386|40,386|0|0.0%
wt : 447,597|447,202|-395|-0.1%
cpu : 432,655|432,636|-19|-0.0%
mu : 40,772,776|40,759,592|-13,184|-0.0%
pmu : 40,905,480|40,892,112|-13,368|-0.0%
Profiler output
=== 8.x..et-node-ml-1498674-282.patch compared (519ffa9a30e84..519ffb355f5ce):
ct : 40,386|41,335|949|2.3%
wt : 447,597|459,557|11,960|2.7%
cpu : 432,655|439,488|6,833|1.6%
mu : 40,772,776|40,722,768|-50,008|-0.1%
pmu : 40,905,480|40,857,112|-48,368|-0.1%
Profiler output
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
Comment #292
carsonevans CreditAttribution: carsonevans commentedMay profiling of this may not have been ideal, so setting it back to needs profiling so someone else can give it a shot.
Comment #293
plachHere are my numbers: I run several xhprof sessions on a plain/patched 8.x front page with 50 nodes and 1000 nodes in the db. The difference was pretty consistently around 1.5% wall time and ~0.5% mem use. However looking at the single functions, many differences varied from session to session, although the highest exclusive time diff was always in
PDOStatement::execute()
, which is varying between 18% and 45% (around 20ms). As you can see the variance is very high wrt to the reported difference, hence I'd say that we do have a performance penalty, but not reliably measurable. Microbenchmark results were a bit more consistent on this side.To sum up the most relevant difference is, as expected, in the query time, for whose optimizaton we have a plan in the OP. It should also be noted that Views is not using
node_field_data
as base table yet, hence the resulting query is not ideal.Comment #294
plachI think we are done with profiling here, unless someone else wishes to give this a last shot.
Comment #295
plachI run another mb session with the following queries (the roughly optimized versions of the views frontpage query) on the same dbs, 10000 runs:
HEAD:
PATCH:
No measurable difference.
Comment #296
fago@plach: amazing work, ++!
It's great to see that the numbers are finally better with the updated patch. I think the storage code could use some more clean-up, variable name & code-flow improvements etc., but that a) does not need to hold up this great work in any way b) that clean-up will need to go beyond of what this patch introduces - thus deserves its own issue anyway. -> followup.
Comment #297
das-peter CreditAttribution: das-peter commented#1620010: Move LANGUAGE constants to the Language class was committed and interferes with #282 -> re-roll.
And here's an overview over all the introduced
@todo
s:NodeStorageController::mapToDataStorageRecord()
: @todo Remove this once comment is a regular entity field.node_schema()
@todo Remove the following columns when removing the legacy Content Translation module. See http://drupal.org/node/1952062.Are there already existing follow-up issues for these todos?
Comment #299
das-peter CreditAttribution: das-peter commented*blargh* not good. Here are (hopefully all) the necessary adjustments.
Comment #299.0
das-peter CreditAttribution: das-peter commentedLink added to 1998416
Comment #301
das-peter CreditAttribution: das-peter commentedHere we go - fixed views table configuration.
Comment #303
plachAnother reroll. Tomorrow I'll post some updated benchmarks in the OP. I think we should more or less ready for RTBC afterwards.
@fago:
I agree. I'd definitely wish to clean this stuff up while working on entity storage :)
PS: Can a d.o. power user delete the bot comments so we get back under the 300 threshold? Thanks :)
Comment #303.0
Gábor HojtsyI was confused for a second was talking about revisions of accounts
Comment #303.1
chrishks CreditAttribution: chrishks commentedUpdated issue summary with link to 2004330
Comment #303.2
chrishks CreditAttribution: chrishks commentedAdded links to follow-up issues
Comment #303.3
YesCT CreditAttribution: YesCT commentedmove followup section next to issues blocked by this on
Comment #303.4
chrishks CreditAttribution: chrishks commentedUpdated issue summary.
Comment #303.5
chrishks CreditAttribution: chrishks commentedUpdated issue summary with additional follow-ups from to-dos
Comment #303.6
chrishks CreditAttribution: chrishks commentedFix closing
Comment #303.7
chrishks CreditAttribution: chrishks commentedFix closing
Comment #303.8
chrishks CreditAttribution: chrishks commentedUpdated issue summary with follow-up from code to-do
Comment #303.9
chrishks CreditAttribution: chrishks commentedUpdated issue summary with follow-ups from to-dos
Comment #304
YesCT CreditAttribution: YesCT commented@chrishks and @Ryan Weal and @hardik.patel99 helped to open followups, and update the issue summary.
We covered some follow-ups that were listed in the issue summary,
some from recent comments from reviewers,
and all the @todo's in the code.
Follow-ups done!
Some of these might be duplicates and could use more detail. We can sort that out later. Some of them make very nice novice issues with a little bit more detail and guidance. (... so ... dont just do them when adding that guidance) :)
Comment #305
das-peter CreditAttribution: das-peter commented@YesCT Awesome! Thank you guys!
@plach Thanks for the re-roll!
Comment #305.0
das-peter CreditAttribution: das-peter commentedUpdated issue summary with follow-up from code to-do
Comment #305.1
YesCT CreditAttribution: YesCT commentedadd the followup to make properties translatable via the ui
Comment #305.2
plachUpdated issue summary.
Comment #306
fagoI had another look at it, so here is a nitpick:
service object should be lowercase I guess.
Overall, I think the patch is ready, performance concerns have been addressed and everything left has follow-ups. So let's move on with this!
Comment #307
alexpottCommitted 477c06c and pushed to 8.x. Thanks!
Comment #308
plachLOL, thanks!
https://www.youtube.com/watch?v=9jK-NcRmVcw
Comment #309
Schnitzel CreditAttribution: Schnitzel commentedw000t! Awesome. Super happy :) thanks. To all who helped :)
Comment #310
plachHappily no longer on sprint :)
Comment #311
plachI don't think this needs an explicit change notice: there is no developer-facing API change. This is just a conversion to the standard announced in Added multilingual support to the standard entity schema. I mentioned this issue there.
Comment #312
Gábor Hojtsy@plach: I think this is a HUGE change in itself that should be documented to communicate it. There are likely people directly querying things still.
Comment #313
plachRight, we probably want to add a paragraph about querying in the generic change notice.
Comment #314
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #315
Gábor HojtsyAdded http://drupal.org/node/2004750 as a short change notice for this. We should add some node query examples in the generic one I think. That should cover it I believe.
Comment #316
jibranI think change notice should explain
node_field_data
andnode_field_revision
.Comment #317
plachDo you mean the new one?
Comment #318
jibranYes
Comment #319
YesCT CreditAttribution: YesCT commentedfollow-up?
#2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations
Comment #320
Gábor Hojtsy@jobran: the specific change notice for this issue links to the generic change notice at https://drupal.org/node/1722906 which explains these tables. What kind of explanation do you miss?
Comment #321
jibranHow about something like this?
{entity}
base table{node}
base table only holds| nid*| uuid | vid | type | langcode | tnid | translate
Primary key: nid
{node_field_data}
and{node_field_revision}
are introduced for property data per language and for node revision property data per language similar to{entity_field_data}
and{entity_field_revision}
.| nid* | vid* | langcode*| default_langcode | title| uid | status | created | changed | comment | promote | sticky
Primary key: nid + vid + langcode
For further details read Added multilingual support to the standard entity schema.
Comment #322
jibranThanks for the update :).
Comment #323
plachI updated the general change notice with a paragraph on querying. We should be done now.
Thanks everyone here!
Comment #324
dcrocks CreditAttribution: dcrocks commentedThere were a number of recent problems that prevented D8 being installed on SQLite. This patch introduced what is hopefully the last one. The 2 new tables, node_field_data and node_field_revisions have both a 'type' => 'serial' column and composite primary keys, a combination that can't be handled by the SQLite driver. #1998366: [meta] SQLite is broken identified and evolved thru a number of problems while installing D8 on SQLite and discusses the issues introduced by this patch towards the end. It is easily fixed to work in a 'monolingual' environment, but that's not the desired result. Any insight would be appreciated.
Comment #325
BerdirAnother follow-up: #2015277: Reduce the number of indexes on the node_field_* tables
Comment #326
BerdirAnother one: #2015303: node_update_8017() can loop forever.
Comment #326.0
Berdirupdate api changes section.
Comment #327.0
(not verified) CreditAttribution: commentedadded follow ups from aboutn #326
Comment #328
plach