Problem/Motivation
Currently the usage of a property data table to proivide multilingual entity properties has to be implemented by each entity controller on its own.
But with the available meta-data this functionality can be integrated dynamically similar to revisions. See #1831444: EntityNG: Support for revisions for entity save and delete operations
Proposed resolution
Integrate a dynamic property data table handling into entityNG base classes.
The attached patch contains the changes made in #1831444: EntityNG: Support for revisions for entity save and delete operations.
It adds a whole bunch of new test entities. The entities are independent dedicated classes to ensure proper test results.
Every currently possible variation is available:
- Simple entity
EntityTest
- Entity with revision support
EntityTestRevisions
- Entity with multilingual property support
EntityTestData
- Entity with multilingual property and revisions support
EntityTestDataRevisions
The major part of the patch was the refactoring of the existing Entity API tests to test every entity variation.
Locally all Entity API tests pass.
Remaining tasks
Reviews needed.
User interface changes
none.
API changes
none.
Comments
Comment #2
ifux CreditAttribution: ifux commentedentityNG-dynamic-property-data-table-handling.patch queued for re-testing.
Comment #4
das-peter CreditAttribution: das-peter commentedentityNG-dynamic-property-data-table-handling.patch queued for re-testing.
Comment #6
das-peter CreditAttribution: das-peter commentedLooks like something doesn't work around
EntityTestDataTranslationController::removeTranslation()
.Comment #7
andypostI think better to make tests against core entities then create abstract examples. contact category, imagestyles and views should be used in tests...
Suppose some core hooks could be wrongly fired in tests.
Comment #8
Berdir@andypost: This is about generic support for translatable and revisionable property tables. Your examples are config entities, those have nothing to do with this, they work completely different in that regard.
Adding all these different combinations as test entities makes IMHO sense. As a next step, we can get rid of test entities in field_test.module (test_entity_*) and instead use these for the field api and similar tests which currently depend on those.
The main question imho is if we can write the tests in a way that doesn't involve so many changes. As discussed, I like the approach taken by EntityTranslationUITest, where the test code is defined in a base class and each actual entity has it's own small class that overrides it and provides a few customisations and allows for additional type specific validations. Downside is that this multiplies the amount of test classes and is slower, to do more setUp()/tearDown()'s.
Comment #9
das-peter CreditAttribution: das-peter commentedThe easy issue was that the Entity Translation UI used the simplest entity type without property translation.
The hard part was that
entityNG::getTranslationLanguages()
returned all languages of an entity even ifEntityTestDataTranslationController::removeTranslation()
was called for one of the languages.Thus
DatabaseStorageControllerNG::saveData()
tried to store a record in a removed language.The problem is, that
entityNG::$values
contains 'localized' non-translatable property values andEntityTestDataTranslationController::removeTranslation()
can't remove them atm.entityNG::getTranslationLanguages()
iterates over theentityNG::$values
to collect all available languages and thus ends up returning the languages of missed localized non-translatable properties too.I'm not sure why
entityNG::$values
has localized non-translatable values - but this seems to be the root cause.As far as I could see this values get in by using
entityNG::getTranslatedField()
and a later call toentityNG::updateOriginalValues()
.I've tried to adjust
entityNG::updateOriginalValues()
to filter the values to set - without success.Maybe it would be possible to enhance
EntityTestDataTranslationController::removeTranslation()
to clear all items in a specific language ofentityNG::$values
.The only solution I have for the moment is to add a filter to
entityNG::getTranslationLanguages()
which ensures that only translatable properties are evaluated.Comment #10
das-peter CreditAttribution: das-peter commentedResult of a discussion with fago in IRC:
The entity test classes should be renamed to less confusing names.
We introduce following abbreviations:
The entity test classes will be renamed to:
Comment #11
Schnitzel CreditAttribution: Schnitzel commentedI would suggest to use 'mul' instead of 'ml' as we already use 'mul' in core:
const LANGUAGE_MULTIPLE = 'mul';
and 'rev' instead of 're' ?
Comment #12
das-peter CreditAttribution: das-peter commentedSounds fine - I'd like to keep it as consistent as possible and if there's already
mul
in core I'm happy to reuse it.Comment #13
das-peter CreditAttribution: das-peter commentedRe-rolled based on #1778178: Convert comments to the new Entity Field API and #1831444: EntityNG: Support for revisions for entity save and delete operations.
Entity test classes renamed.
Translation workaround still needed :| Definitely needs review, deleting a language from an entity seems a bit unstable atm.
Hope the patches this bases on will be committed soon, so that this becomes reviewable.
Comment #14
das-peter CreditAttribution: das-peter commentedGo testbot go.
Comment #15
das-peter CreditAttribution: das-peter commentederm, what? Why the heck did the status change fail :-O
Comment #17
das-peter CreditAttribution: das-peter commented#13: entityNG-1833334-12-core.patch queued for re-testing.
Comment #19
das-peter CreditAttribution: das-peter commentedRe-rolled against the latest comments patch.
Comment #21
das-peter CreditAttribution: das-peter commentedAdded new test entity types to REST test.
Comment #22
das-peter CreditAttribution: das-peter commentedI'm a bit pessimistic about the comments patch. Thus here's another re-roll of this feature without the stuff from the comments patch.
Comment #23
BerdirMostly nit-pick review of the actual changes, haven't looked at the tests yet.
New code should follow the coding style, so let's add a type to revision_id @param and fix the reference to SelectQuery. That's now \Drupal\Core\Database\...
Should have @params.
Looks like an accidental change?
\Drupal\...
Same :)
I'll try to get plach to have a look at this.
There's a RTBC patch for the rest test that limits those tests to just node, user and entity_test. Not sure if it makes sense to test the new test entities here too but this shouldn't be necessary anymore then unless we explicitly add them: #1847200: Limit the entity types which are used for the rest tests.
Comment #24
das-peter CreditAttribution: das-peter commented@Berdir Thank you very much for the review!
I've adjusted all mentioned points except
RESTTestBase::entityCreate()
. There I've to wait until the related issue is fixed, as otherwise the tests fail. I'll observe the related issue and re-roll once this is committed.Comment #25
das-peter CreditAttribution: das-peter commentedLooks like the related REST issue was fixed as I wrote my post :) Re-rolled patch.
Comment #26
BerdirThinking about ways to make the diffs for these test changes not that huge.
What about adding a helper method that receives an argument for the entity type? Then we only need to change the hardcoded 'entity_test' to $entity_test. Then we just call that once for each type we want to test which should make the diff much smaller. Can you try that for a testclass or two to see the difference?
Comment #27
das-peter CreditAttribution: das-peter commented@Berdir Thank you very much for the review!
I finally found some time to make a re-roll. The new patch uses a helper method to execute the test-set, while the original method simply iterates over the entity types to test. I hope that's what you imagined.
This approach saves about 16kb :)
No interdiff, I'm sure no one would look at it - to many (non essential) changes.
Let's see if I worked well and everything still passes...
Comment #29
BerdirWorking on this a bit.
Comment #30
BerdirOk, this should be green.
Changes:
- Renamed test helpers to assert* to not have them triggered by the test. That's what caused the failures, everything below is just "delete them functions!".
- removed str_replace('_', '-') stuff, this has been removed for node types a while ago
- merged type list functions into a single one with a filter flag, should make changes/renames easier if they would happen at some point.
- Restructured entity_test_menu() a bit to use the same page callbacks for add/delete, deleted all unused function wrappers, as the goal is to get rid of them anyway.
The difference isn't that big, but it does save ~175 lines of unecessary code, mostly in .module.
About the controllers which are added here. I do see the argument of them being separate so they are separate things, but I don't think that's necessary. Might have been easy to copy/paste them, but they also need to be reviewed now and maintained later. So I'd vote for combining them as much as possible:
- I think the overridden translation controllers should go into an EntityTranslationControllerNG. They look generic and identical and can later on be merged back into the main class once everything is NG.
- The form controllers look almost identical too, is the only difference really the redirect path and message? We can make that generic and have a single form controller for all of them then.. Might even want to fill a follow-up issue to add default implementations for save and delete right in the default controller, if there isn't one already.
- storage controller I'm not sure. they look identical right now as well, but e.g. EntityTestStorageController shouldn't have a revision_id. Maybe a base implementation that has subclasses with additional properties for multiple/revisionable? It's not like the order of properties matters much, at least not for these test entities :)
Comment #32
plachI think an awesome work is going on here! The improved test coverage is impressive, although an approach more similar to the entity translation UI tests could save us some headaches, should a particular test entity need any sort of special-casing.
Here is my review: the code looks already pretty good and I performed the smallest changes myself to avoid cluttering this with minor stuff (I hope I didn't break anything).
Actually this is wrong, as it is duplicating the logic in
::attachPropertyData()
: the join will cause multiple entries for each entity to be returned by the main load query. This is not immediately visible since the result is keyed by entity id, however dropping all this code should be safe and correct.I'm not totally convinced this is right. It seems to me that we might end-up including more lines than the ones we want to retrieve. Didn't test this, though.
@Berdir, can you confirm the logic here is correct?
This is pretty weird, but we need to refactor this code anyway (see #1810370: Entity Translation API improvements), hence I think it's a good stop-gap fix.
Here and elsewhere we need to use
format_string()
.This is being used throughout many many core tests, hence should not be marked as private. I'd suggest to remove the leading underscore from the function name.
Aside from the things above I think we should really introduce some intermediate classes all the test entity controllers should inherit from to factor-up the common code. Basically I agree with everything @Berdir is saying in #30 :)
http://drupal.org/project/issues/search/drupal?status[]=Open&version[]=8.x&issue_tags=Entity+forms ;)
Comment #33
plachThis is for the bot, but the status is actually needs work per #32.
Comment #34
plachComment #35
BerdirThat was exactly my initial feedback when das-peter started on this as well. However, there are two important arguments for not doing this:
- In the Translation UI tests, you are testing whith real and existing entity types from different modules which all have their special cases. And contrib modules can also use the pattern to easily add test coverage for their own translatable entity types. This is different, we are just using test entity types which were specifically created for these tests in the first place. The chance for requiring some type-specific special casing is much slimmer and if there is, we can easily ad simple if/else for that. Your tests are integration tests with those different entity types, our entity types are just helpers to test an implementation.
- Secondly, this does only slightly increase test time because it's all within one test method. Doing this your way would add 20+ (wild guess) new test classes, which is quite an overhead due to setUp() and teardown(). I think most entity tests can be converted to DrupalUnitTestBase which would eliminate most of the overhead, but still.
Will look into the other things.
Comment #36
das-peter CreditAttribution: das-peter commentedHere's a re-roll. Changes made:
format_string()
DatabaseStorageControllerNG::buildQuery()
_entity_test_entity_types()
Comment #38
das-peter CreditAttribution: das-peter commentedNo changes, just a re-roll. I'm not sure what went wrong :|
Comment #39
plachThis looks good to me, I performed just some minor clean-up:
DatabaseStorageControllerNG::saveData()
toDatabaseStorageControllerNG::storePropertyData()
for consistency withDatabaseStorageControllerNG::attachPropertyData()
;I'd be tempted to RTBC this, only the code above still looks suspect to me, but I didn't test it yet. Again, I'd be happy if @Berdir could confirm it's good.
Comment #40
plachAlso, this is something we HAVE to do, and ASAP, not a feature.
Comment #41
BerdirWe just discussed the point of @todo's in code on twitter yesterday. I know this is just moved around here, but can we add (if not yet existing) and reference a followup issue here?
I've had to read this twice as well, but I think it's ok.
Wouldn't hurt to add something to the revisions tests specifically for the mulrev entity I guess and create multiple revisions in multiple languages, if we don't do that yet.
Nitpick: id's should be ID's.
Wondering if we want to call a method explicitly here, instead of relying on magic setters, for performance. Might also a be a bit more obvious that we're not just setting public properties here but initialize the field classes.
Not sure how easily that actually is right now.
Comment #42
BerdirCrosspost.
Comment #43
plachI'm not 100% sure, but I think the new EntityQuery has already language parameters...
Might be worth a try.
Comment #44
plachEntity queries do have
langcode
parameters but we needdefault_langcode
parameters here. I created the related follow-up as requested (#1866330: Add a parameter to specify the default langcode value when loading/querying entities) and mentioned it in the todo.I tried to use
::set()
as suggested above, but configurable fields require compatibility mode which makes things even more unreadable. I added a todo instead.This should be ready to go IMO.
Comment #45
plachMmh, this makes no sense: we are attaching properties not fields. Will try to investigate this a bit more.
Comment #46
BerdirYes, we discussed that part yesterday too and figured that it's not easy, so I'm fine with trying to improve that in a follow-up, it's not worth holding up the patch because of this. Once we have a proper solution for this, we probably want to use it in the existing map from storage methods as well anyway.
Comment #47
plachOk, so what are we missing? ;)
Comment #48
BerdirSomeone bold enough to mark this RTBC, I guess :)
About the set part, I think fago is doing all sorts of crazy things to improve the performance of the mapping/object creation, yet another reason for ignoring it here.
We introduce the init() helper method in the base entity class, but the different test classes have not yet been updated to override that one instead of init().
Comment #49
plachIf someone else provides a new patch fixing #48, I'll dare RTBC it :)
Comment #50
BerdirOk, fixed that.
Comment #52
plach#50: entityNG-data-table-1833334-50.patch queued for re-testing.
Comment #54
BerdirFixed the test failures. Those are new tests, which failed because they expected a revision id and default language on the entity_test entity type and also expected a given order of the properties. Changed that to use array_diff() so that the order doesn't matter.
Comment #55
plachI only performed some minor clean-ups while reviewing the latest iterations so I feel confident about RTBC-ing this.
Let's get it in.
Comment #56
Gábor HojtsyPut on D8MI sprint to track closely.
Comment #57
fagoSo finally I had a look at this. Patch generally looks good + great work on the tests.
However, I'm worried as this will quite conflict with the comment-issue improvements. I've started separating the API improvements from the comment issue, so we can build from there in other reviews and get it committed faster. See and review #1869250: Various EntityNG and TypedData API improvements. Not sure which patch is better re-rolled on top of which one?
Well, with the new entity field API everything on an entity is called a field. Just, if you look at an entity from the typed data api perspective it's some complex data having properties, which are the entity's fields. In the entity api though, we should call them "fields" only though.
This actually means we should even change the "data" table name, e.g. from entity_test_property_data to entity_test_field_data. I think we also have some property wording left in the databasestoragecontroller, thus I guess we should do that renaming in its own issue.
The function signature is wrong. It does not get entities passed, but storage records which get mapped to entities later on.
Consequently this todo is also wrong, no entities at this stage.
This sounds odd. Before committing a workaround we should at least know what is causing this. Perhaps it's even a non-issue with #1869250: Various EntityNG and TypedData API improvements thanks to the new BC-mode?
Comment #58
das-peter CreditAttribution: das-peter commentedChanged method signature and removed todo as mentioned by fago.
I'm sure this will blow! First I tried to base the patch on the entity enhancements (comments patch) but eventually I decided that I don't want to wait for the big bang to happen and go for small iterative changes (Kaizen styles! :P ). So whatever patch goes in first, I'm ok with that. If the API improvements goes in first I'll happily re-roll this patch, if this patch goes in first I'll happily re-roll / extend the API patch. Only thing that matters to me is progress of some sort.
"No decision is worse than the wrong decision" - I think that's what Sun Tzu would say ;)
Comment #59
fagoThanks - changes look good! What about the non-translatable properties todos? For which entity type and property did the problem arise?
Yeah, it's time to get all those issue fixed!
Comment #60
BerdirI don't know, but let's see what happens with the tests if we remove that part from the patch.
Comment #62
fagoInterestingly, I had this fail while working on #1869250: Various EntityNG and TypedData API improvements also - and I think it contains a fix for it - this one:
Let's see whether it's enough to fix this problem or it requires the new BC also (I can't remember the details any more) -> added this hunk to the patch.
Comment #64
fagoconflicts with recently committed ng-changes of #1826688: REST module: PATCH/update, re-rolled patch.
Comment #66
das-peter CreditAttribution: das-peter commented"Translation successfully deleted." that's exactly the issue I was struggling with.
getTranslationLanguages()
keeps returning the langcode that should be removed because non-translatable properties have localized values assigned and the translation handler isn't capable of removing them.Comment #67
fago"non-translatable properties have localized values"
Yep, the question is why. As talked via IRC it could stem from the BC mode. Is the problematic langcode the entity language only? If so, we know that it would be fixed with the new BC mode.
Comment #68
das-peter CreditAttribution: das-peter commentedI debugged again and I was like being struck by lightening. The issue was so obvious I'll feel stupid the next several days...
DatabaseStorageControllerNG::attachPropertyData()
attached the properties, based on the data table schema, to the record object. But the schema holds not only the translatable properties but also some shared ones (e.g. id, uuid).To fix this I load the field definition via
DatabaseStorageControllerNG::getFieldDefinitions()
and check if a property from the schema is translatable berfore set it on the record.Another approach could be to iterate over the diff between the main and data table schema diff, but that feels again like rely on an assumption.
My local tests are still running but I'm optimistic and thus give the testbot a shot.
Comment #69
BerdirYes, this makes sense :)
Let's get this in and then re-roll the other issue, which isn't quite ready yet and currently failing tests.
Comment #70
fagoGood catch das-peter, makes sense. Yep, let's move on with this!
Comment #71
YesCT CreditAttribution: YesCT commented#68: entityNG-data-table-1833334-68.patch queued for re-testing.
Comment #73
BerdirHad a look at the test failure. What the test is trying to do is unset a dynamic field, then serialize it (where it doesn't show up at all then), then PUTs it to the service which saves the entity and because it does not exist at all, it's not updated.
What's interesting is that calling debug($entity->getPropertyValues()); in EntityResource::put() seems to internally initialize the field, with an empty array as value, then the tests pass.
I'm not sure how to properly fix this, should we ensure that all properties are initialized before calling field_attach_*(), does the serializer need to export empty but defined property as an empty array/NULL, are the tests even valid?
I also tried to only set the value to NULL like this
$update_entity->field_test_text->value = NULL;
instead of the currentunset($update_entity->field_test_text);
but even that results in the field completely missing in the serialized output, so I'm wondering if the serialization is incorrect?Comment #74
fagook, that's the same problem I ran into with #1869250: Various EntityNG and TypedData API improvements - where I fixed it. The problem arises thanks to http://drupal.org/node/1868274#comment-6863934.
As already fixed it #1869250: Various EntityNG and TypedData API improvements, may I propose we move on with it first and get it in asap?
Comment #75
Gábor Hojtsy#1869250: Various EntityNG and TypedData API improvements is committed now, so ... back here? :)
Comment #76
plach[wrong patch]
Comment #77
plach#68: entityNG-data-table-1833334-68.patch queued for re-testing.
Comment #79
BerdirWorking on a re-roll, hope nobody else already is :)
Comment #80
BerdirUh, that was a fun one.
This might not be perfect, especially the parts in addPropertyData(), but it was the only thing that I could get past the tests (For the record, it is awesome that we have that test coverage now, helps immensly)
@fago: "The function signature is wrong. It does not get entities passed, but storage records which get mapped to entities later on.". That was/is wrong, that function converts $records to entities but keeps calling them $records. I introduced a new $entities array to make that more obvious and changed $records in that function back to $entities.
Actually, right now, I think we can change that and add the property data to $values, which should make it considerably faster than it will be with my ugly attempt. But that will have to wait until tomorrow unless someone else wants to work on it. So we should probably merge those two functions, create $values and then instantiate the entity classes.
Another fun snippet that I fixed:
:)
Comment #81
BerdirAlso, one unfortunate things about the way the test classes are structured now that Simpletest always reports the line where assert() is called in the test method, so you have to search for the assertion message to find the actual line. Not sure if we should use a different method now or how that detection exactly works, will have a look later.
Comment #83
Berdir#80: entityNG-data-table-1833334-80.patch queued for re-testing.
Comment #85
das-peter CreditAttribution: das-peter commentedLooks like
Drupal\entity_test\EntityTestTranslationController
was moved toDrupal\translation_entity\EntityTranslationControllerNG
.Unfortunately fixing this leads to 3 other fails in
Entity Test Translation UI
, this needs some more investigation.Comment #86
fagoYep, the NG-adaptions have been moved into a generally re-usable NG translation controller. Not sure why this causes test-fails with that patch?
Comment #87
BerdirYes, this fails.
The thing that fails is switching the source language, then the translatable field is empty. Before I look into it, maybe @fago has an idea what could be the reason?
Comment #88
das-peter CreditAttribution: das-peter commentedI just debugged
translation_entity_prepare_translation
a little bit and figured out that addingit
with the sourceen
works as expected, but ifit
is used as source forfr
the values ofit
seem to be broken.it
suddenly has more than one value in the translatable field; delta 0 contains NULL but delta 1 contains the original value...Comment #89
das-peter CreditAttribution: das-peter commentedAt the beginning of
DatabaseStorageControllerNG::attachLoad()
the affected entity holds NULL in delta 0 - I guess this has to do with the prototyping(?) - after callingfield_attach_load($this->entityType, $bc_entities);
the entity has delta 1 with the correct value attached. Could it be that NULL values should be stripped / replaced when creating bc entities?Comment #91
plachI think I identified the problem here, not sure how to fix it though.
When creating an entity, field widgets have
LANGUAGE_NONE
language. After submitting the form the languages for translatable fields are switched for the submitted values to match the form language (seeEntityTranslationController::submitEntityLanguage()
). This happens before building the new entity. Afterwards submitted field values are extracted infield_attach_submit()
. In this case each submitted translatable field value holds two languages, for instance:In
field_attach_submit()
all the available languages are processed. After debugging for a while I discovered that, due to the Entity Field API internals, when setting a value for a language all languages get the same value (I guess the culprit is a wrong reference). What is happening is that the value corresponding to the submitted form language is processed as first at line 198 of field.attach.inc and both languages (LANGUAGE_NONE
and'en'
) are getting the submitted value, afterwards the emptyLANGUAGE_NONE
value is processed and both languages get an empty array. As a result the submitted value is not stored:Comment #92
fagohm, so it wants to change LANGUAGE_NONE only while 'en' gets changed as well. Which one is the entity language?
Entity Field API uses LANGUAGE_DEFAULT (what equals LANGUAGE_NONE for now) to key values in default language, so the BC decorater creates a reference to that values. If then it adds LANGUAGE_NONE values at the same time I guess it accidentally writes to the reference. We could fix that by separating LANGUAGE_NONE and LANGUAGE_DEFAULT what (I tried) does not simply work but should be doable with the new BC-mode.
However, I'm wondering why a field has values in LANGUAGE_NONE + a custom language code? Does that make sense? LANGUAGE_NONE would imply to me there cannot be anything else as it is not translatable?
hm, that sounds strange. It implies that field_attach_load() adds values instead of replacing values - is that really the case? If not I guess the value gets already stored like that. So probably just #91 is the bug here, as prototyped values should be array(0 => array('value' => NULL));
Comment #93
plachIn this example
LANGUAGE_NONE
is the initial entity language, while'en'
is the submitted entity language.It is not meant to have both
LANGUAGE_NONE
and'en'
stored, but when the entity language is changed, as in this case, an empty translation corresponding to the previous entity language is set for all translatable fields. This way the submitted values get the new entity language while the old values are deleted. When we complete the transition to the Entity Field API we can simply remove this stuff as field values corresponding to the original language will always beLANGUAGE_DEFAULT
, while the others will always have the language corresponding to the translation they belong to. But for now we need to preserve the current behavior I guess. Or we could implementEntityFormControllerNG::submitEntityLanguage()
and override this logic.Anyway, I've no idea why tests fail only with this patch applied.
Comment #94
fagoI see. Thus, it needs to copy over default values (in pre-NG times) - but is that a reason for having both values in the submitted form values?
Yeah, or we can just override it in the NG controller to skip the copying-part as it's not needed any more - that would be reasonable.
Comment #95
plachLet's see whether this fixes the failures.
Comment #97
das-peter CreditAttribution: das-peter commentedDB content looks like this:
entity_test_mul
entity_test_mul_property_data
field_data_field_test_et_ui_test
While the properties seem to work fine the field contents really look odd :|
So we really have to look how field contents are saved.
However, this doesn't explain why
it
has more than just one delta after callingfield_attach_load($this->entityType, $bc_entities);
.Comment #98
das-peter CreditAttribution: das-peter commentedI debuggeg further and came across this:
DatabaseStorageControllerNG::invokeHook()
passes$entity->getBCEntity()
tofield_attach_insert()
.field_attach_insert()
triggers_field_invoke()
which iterates over the languages available for a field (detected byfield_available_languages()
). One interesting point there, is this line$entity->{$field_name}[$langcode] = $items;
, it sets the value back to the entity again - don't ask me why.However, after
_field_invoke()
the entity looks like this:I don't think that's correct :|
Part of this issue is, that
EntityBCDecorator::__get()
"creates" values with the langcodeund
for accessing the "default language". I think we could fix that by separating LANGUAGE_NONE and LANGUAGE_DEFAULT as fago already mentioned.Comment #99
plachYes, I found similar results while debugging. I managed to obtain at least correctly stored values with the attached hack (the first hunk is unrelated) but we need to incorporate it into the BC decorator somehow. As of now I'm trying to find out what's causing the mismatched deltas you are reporting above. I think this is caused by calling
EntityNG::getTranslation()
inDatabaseStorageControllerNG::attachPropertyData()
before loading configurable field values. Digging in further.Comment #100
plachOk, this one should pass tests. I think this solution is pretty hackish, I hope @fago can suggest something better. I am afraid we'll need to stick with 'und' field values being stored next to language-aware values for now, becuase removing them messes the BC decorator up.
I must say I'm a bit concerned about the complexity the Entity Field API currently introduces: a bug that I would properly fix in less than a hour took me one afternoon just to provide a hackish fix. I hope I just need some more experience and gain some confidence with the Typed data stuff...
Comment #102
plach#100: entityNG-data-table-1833334-100.patch queued for re-testing.
Comment #103
das-peter CreditAttribution: das-peter commented@plach Awesome work. Thanks a lot! And I agree that both of the changes look a bit scary.
On the
// @todo Hack around Entity Field BCDecorator.
change I was asking myself ifund
is a valid language code for a translatable field anyway, or iffield_available_languages()
should return only "real" languages in such a case.And for the other change I don't see another way atm. I think it's correct to use
$entities[$id]->getTranslation($langcode);
inDatabaseStorageControllerNG::attachPropertyData()
to set the properties.And even if I don't know it, I'm sure there's a reason why
EntityNG::getTranslatedField()
initializes not yet available fields with NULL as value.I'd like to RTBC this, but I think it's better fago takes a final look first ;)
Comment #104
plachNot sure it was clear from #100 but #99 is not included in the patch, because when loading field values the BC decorator expects to find
'und'
values, but other pieces of code expect to find values for the original language. So we need to live with both until we remove the BC layer.Comment #105
das-peter CreditAttribution: das-peter commentedOuch, I missed that part. So this basically means we end up having an
und
entry in the db-table. I don't think that was the idea. The question is if this is just caused by this patch or if this is already the case with the committed parts of the Entity Field BCDecorator.If it's caused by this patch I don't think we can RTBC it, otherwise I don't see a reason not to RTBC but create a follow-up issue to fix this.
Comment #106
plachAFAICS this is the current BC decorator behavior but I might be wrong, let's wait for @fago to have the last word on this.
Comment #107
fagoSo the problem is that field_available_languages() finds 'und' as valid language?
You may not delete 'und' values as those are expected by EntityNG as LANGUAGE_DEFAULT values. The proper fix would be changing the LANGUAGE_DEFAULT constant.
Is it really necessary to check the data schema here? Couldn't we just check $values - everything in the table must be in $values not?
This has performance implications as objects will be destroyed and have to be re-created. We should add a $entities[$row->entity_id]->{$field_name}[$row->langcode] = array() to field_sql_storage_field_storage_load() instead.
Comment #108
plachNo, while trying to make the tests pass again, both @das-peter and I realized that currently when saving a multilingual field both an
'und'
value and a duplicate one with language corresponding to the entity language are stored. This is not correct as only one should be there, but we can live with this until the Entity Field API conversion is complete. Once we get there we will have only the'und'
values (or whatever the language code forLANGUAGE_DEFAULT
will be).The problem that is making tests fail is that, since we need to get the various entity translations while attaching the data table values, all the entity fields are initialized, including configurable ones, which have no value because
field_attach_load()
has not be called yet. When this finally happens, field values are appended to the empty one created byEntityNG::getTranslation()
, so you get an extra empty item as first one, which messes all deltas up. This happens only with translations, because it's triggered by the prototyping logic ingetTranslatedField()
.Working on the suggested fixes.
Comment #109
plachNow, this looks good! Thanks @fago :)
Time to get this back to RTBC!
Comment #110
plach/me cannot write :(
Fixed typo.
Comment #112
plach#110: entityNG-data-table-1833334-110.patch queued for re-testing.
Comment #113
fagoindeed, that's better :) So let's move on with this!
Comment #114
YesCT CreditAttribution: YesCT commented#110: entityNG-data-table-1833334-110.patch queued for re-testing.
Comment #115
webchickThis looks pretty crazy. Tentatively assigning to catch.
Comment #116
webchickEr. And crazy not in a "drooling on yourself" way, but more in an "over my head" way. :)
Comment #117
catchThis looks reversed to me. If no properties are translatable, why query the data table at all?
This needs a transaction.
When I saw the title of this patch I was somewhat hoping it was going to kill hook_schema() for entities and instead put the responsibility for that in the Entity API instead. Follow-up?
When can this happen?
Comment #118
plachSure, we totally need this: #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.
Comment #119
plachAlso:
Even untranslatable properties are stored in the data table, similarly to fields. See Added multilingual support to the standard entity schema.
Comment #120
plachThe attached patch renames
DatabaseStorageControllerNG::storePropertyData()
toDatabaseStorageControllerNG::savePropertyData()
for consistency with the other method names and removes:which looks like unnecessary code baby-sitting. If this was needed to avoid a test failure we should fix the test instead.
@catch:
DatabaseStorageControllerNG::savePropertyData()
is called from within the transaction initiated byDatabaseStorageControllerNG::save()
hence it should be safe as it is. NeitherDatabaseStorageControllerNG::saveRevision()
opens one for the same reason.I think this answers all @catch's concerns, so this should be RTBC again if tests pass.
Comment #121
BerdirHm, I think the empty check is not baby-sitting but defensive programming ;)
Consider this:
What could happen here is that, for whatever reason, the passed in ids can not be loaded and an empty array is sent to delete(). Now, this results in a PDO Exception (id IN ()) and IMHO, it shouldn't be that easy to trigger PDO Exceptions when calling API functions.
load() checks this explicitly as well. It is possible to pass both an empty array to it (which does not return any results) or FALSE (which returns all results).
But I do not care enough to not mark it RTBC, I'll let @catch decide.
Comment #122
plachAnd the funny thing is yesterday I even checked
load()
to see what we were doing there. Here is one restoring the "defensive" lines. @catch can commit #120 or this one, as he prefers.Comment #124
BerdirTable not found random failure.
Comment #125
Berdir#122: entityNG-data-table-1833334-122.patch queued for re-testing.
Comment #126
BerdirComment #127
catchHmm. I'd normally say that calling code should check the IDs are there before doing something with them. But in the case of delete you just want to delete stuff, so if it's not there and there's nothing to delete there's not really any harm done (i.e. if another process got to it already or something).
Committed/pushed to 8.x, will need a change notice.
Comment #128
jibranAdded tag
Comment #129
BerdirThere are no public changes, so I just described what entity types need to define to be able to rely on this: http://drupal.org/node/1888646
Comment #130
fagoLooks good.
It has randomly mixed up the usage of the word "properties" and "fields" though, so I've updated it to use "fields" everywhere.
Comment #131
plachI added a link to the parent change notice. The rest looks good to me. Although we may want to switch to a "multilingual" terminology instead of talking about "translation" in the clean-up phase, since this is more correct.
Comment #132
Gábor HojtsyThanks all, this is a great step for translatable properties :) <3
Comment #134
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.