Problem/Motivation
While writing the mongodb content entity storage passing back stored data to ContentEntityBase was the biggest mystery. I have figured it out, I think.
Proposed resolution
Document the array. Add a todo.
Remaining tasks
The whole loadFromMongo method really badly wants to be documentation; this is the first half, up to creating the entity. Where do we put the second half?
Get someone (plach/berdir/fago) to verify the @todo . If correct then #2137801: Refactor entity storage to load field values before instantiating entity objects needs an issue summary update, I think. Ie. I know loadFromMongo works because Drupal\node\Tests\NodeViewLanguageTest and Drupal\node\Tests\NodeFieldMultilingualTest pass since I changed it to do what core does, I think at least -- but the question is, is this the only way?
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff-5-12.txt | 468 bytes | snehi |
#12 | interdiff-5-12.patch | 468 bytes | snehi |
| |||
#12 | entity-ceb_values-2430673-12.patch | 1.44 KB | snehi |
#5 | entity-ceb_values-2430673-5.patch | 1.45 KB | plach |
ceb_values.patch | 1.59 KB | chx | |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
BerdirThe referenced issue wouldn't change anything as far as how this is stored. What it would change that we would pass a $data array into loadFields() or how it is called again, and then collect the field values in an array instead of passing in an entity object and assign the f ield values to the entity object through the (slow) entity field API.
And it should be possible to already pass in an array with more than one translation directly to $values. I'll check the sql implementation again but I'm pretty sure we already do that for base fields, just not configurable fields (and none of those, default language or not), @plach might be better at answering this.
In your method, shouldn't the break on http://cgit.drupalcode.org/mongodb/tree/src/Entity/ContentEntityStorage.... be *inside* the if, otherwise it would stop after the first loop, wheter it found the default language or not?
Comment #3
chx CreditAttribution: chx commented> In your method, shouldn't the break [snip]
Yes. It works because save already moves the default to be the first and it's just a safety against manual manipulation but thanks I fixed it.
Comment #4
plachWhat about something like this? I documented all the constructor parameters instead, which should be more interesting for API consumers :)
Btw, I think the
langcode
level was added to$values
after the$translations
parameter was added. I think the latter is useless now and could be derived by the$values
langcode
keys. However that's a separate issue, I guess.Comment #5
plachBetter docs for the last param.
Comment #6
chx CreditAttribution: chx commentedSure, but I am 95% sure you can't pass translations in values, check the code in the constructor:
I tried to pass in non-default langcodes -- but perhaps when I tested that I still used en instead of LANGCODE_DEFAULT ? Perhaps. I will re-test, nonetheless, this is patch is a big step forwards, thanks. But I still would like to see some code which passes translated values in $values...
Comment #7
plachIt might be a good idea to add a test here.
Comment #8
jhodgdonI have not comment on the accuracy of the docs - will let others review that.
A few typos:
a)
"by" should be dropped.
Also Please Do Not Use "e.g.", unless you want to punctuate it properly. Better not to use it, as half of the readers do not understand the difference between e.g. and i.e. If you insist on using it, the correct punctuation is:
(e.g., ...)
b)
This last sentence is a comma splice. Should be "... field values; for instance, the default...".
Comment #9
Berdir@chx: #2137801: Refactor entity storage to load field values before instantiating entity objects has a patch now, note that it doesn't change anything about the constructor arguments, but will hopefully make it easier to understand how $values is built.
I think the issue that would be more interesting for you would be #2142885: Simplify ContentEntityBase internal field storage by removing special treatment for LANGCODE_DEFAULT, which would change $values from being first keyed by field name and then language to first language, second field name. Which I think would be a lot easier to create for you. That would be an API change, but I think you would be the only person affected by that and like +1 as it would simplify your implementation.
Comment #10
chx CreditAttribution: chx commentedTest posted in #2436209: Test ContentEntityBase constructor called with multilanguage values
Comment #11
xjmCame here from #2436209: Test ContentEntityBase constructor called with multilanguage values.
This is hard to understand; an example would go a long way. Also at first glance it was unclear to me why this would be so drastically different from the parameter in
Entity::__construct()
until I realized that it's still keyed by field name with just a different/specific structure for the "values". So it might be good to mention the differences?Comment #12
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone as suggested in #8
Comment #14
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAttaching diff
Comment #15
jhodgdonThanks, but it doesn't look like #11 was addressed at all?
Comment #26
mimpro CreditAttribution: mimpro as a volunteer commentedI am changing this category to TASK because this is more likely a task rather than bug, the code works well. If anyone disagree, feel free to change back to bug.