Start with refactoring entity schemas with the test entity so we can experiment with changes in EntityFieldQuery. As per discussion in #1498634: [meta] Define revision/translation SQL schema for core entities (no patch), we are making all properties of an entity essentially a multi-column field in terms of storage. We are not making changes to their exposure to the rest of the system.
The test_entity schema currently looks like the following: PK(id), name, uid, langcode
The proposed new schema is:
- entity_test: PK(id), langcode
- entity_test_property_data: PK(id, langcode), (source_langcode), name, uid
Comment | File | Size | Author |
---|---|---|---|
#42 | drupal-et_ml_schema-1658712-42.patch | 23.08 KB | plach |
#42 | drupal-et_ml_schema-1658712-42.interdiff.do_not_test.patch | 3.32 KB | plach |
#40 | drupal-et_ml_schema-1658712-40.patch | 22.48 KB | plach |
#39 | drupal-et_ml_schema-1658712-39.patch | 22.41 KB | plach |
#36 | drupal-et_ml_schema-1658712-36.interdiff.do_not_test.patch | 3.72 KB | plach |
Comments
Comment #1
Gábor HojtsySchema changed. This should hopefully break tests in many ways.
Comment #3
Gábor HojtsySounds like the next step is to extend/override the default DB controller to store/update the base properties in the property data table and only create/delete the base entity table value on the initial creation / deletion. This is not really modifiable in the base databasecontroller as-is since the other entity controllers extend that.
Comment #4
plachWorking on this.
Comment #5
Gábor HojtsyComment #6
plachI have a semi-working (actually semi-breaking everything ;) patch. I'll try to complete it tonight.
Comment #7
plachOk, let's see how many things break NOW.
Comment #8
plachComment #10
plachFixed tests
Comment #11
sunAdditional function calls to ::entityInfo() & Co. for every ::get() (and also ::set()) is concerning.
We need to think about ways to make this faster.
I wouldn't expect the properties on an entity to be dynamic in the first place, so I don't quite get why all of this doesn't happen in __construct() or so?
If $properties is an object, then it at least needs to be documented as such.
Furthermore, the plural name suggests that it is an array.
I'm seriously NOT a fan of the names in Symfony, but if we'd want to align ourselves with that, then the most closest name would be $propertyBag.
We badly need to get rid of these ambiguous method names. It's not clear at all whether they're getting or setting something, or perhaps even doing something completely different. So let's not continue this bad pattern for this example refactoring, please :)
Given what this method is doing, it could be getLangcodeValue() or resolveLangcodeValue(). The latter might be more clear.
Comment #12
plachA general note for reviewers: keep in mind this is only experimental code which is likely to look quite different when we come to real entity implementations. In many places it would make far more sense to alter the base classes instead of hacking things in the extending ones. However this is needed for now to keep things working.
The attached patch implements @sun's suggestions and fixes query builders conditions as a bonus :)
@sun:
Actually I'm not concerned about this, since when fields and properties are merged we won't need this check in the first place. For now I even doubled it to be able to reuse the parent implementation for fields, but this is going to be cleaned-up when (if) we start to propagate this change to real entities.
Totally agreed on the following notes about naming so the attached patch switches to arrays, which is also consistent with the constructor argument and allows to simplify code a bit. Just as a sidenote I totally don't like to have this publicly visible global accessors for properties and I wish we could have namespace-level visibility as in Java. However this is not a big deal as these accessors are going away together with properties as we know them now.
I picked
resolvePropertyLangcode()
as it felt more self-documenting to me. This is going away too, in favor ofEntity::getFieldLangcode()
, which we might move to the resolve terminology, meanwhile.Comment #13
Gábor HojtsyLooked at the patch and it seems to be generally good. For the querying part in the DB controller though, the data table purposely includes all non-multilingual properties (such as the source langcode) of the table, so you don't need to join the property table, you can query from the property table directly. The idea being that only creation and deletion would touch the base entity table, no?
Comment #14
plachYou're rght :)
Comment #15
Gábor HojtsyLooks generally good to me however, this looks suspicious:
Where are the $ids taken into account?
Comment #16
plachI knew I was drunk but I didn't think I was SO drunk :)
The attached patch fixes #15 and also language conditions which were lost in #14.
I guess we need better test coverage here...
Comment #17
Gábor HojtsyYeah, looks like some tests are in order for querying test entities.
Comment #18
plachAside from tests, do we need anything else here? I guess experimenting with multilingual EFQs + related tests deserves a dedicated issue...
Comment #19
Gábor Hojtsy@plach: right, that should be the next issue after we got this in :)
Comment #20
plachOK, tests + new issue coming ASAP.
Comment #21
plachOk, here are tests, the rest of the code is untouched. I didn't spend much time around query conditions since they should go away in favor of EFQ.
Comment #22
plachHere is the EFQ issue: #1691952: Make EntityFieldQuery work with multilingual properties.
Comment #23
Gábor HojtsyLooks good to me now, has test coverage even for some querying and all issues with the query were fixed.
Comment #24
fagoGood work! I've reviewed the patch:
Why is that being removed? Maybe it should be moved, but I don't see that case covered in the translation test case.
Good work on the tests, but we don't t() test assertion messages any more. Thus, all that t() calls in assertion messages should be removed.
As I've mentioned #1184272: Remove deprecated $conditions support from entity controller comment #11, I think querying for language specific values should work a bit differently. As said there, there is a distinction between querying for properties in a certain language and querying for an entity that has a certain default language. That said, querying with array('langcode' => 'value') should query for entities with default language 'value'.
Anyway, I think we should start working on #1184272: Remove deprecated $conditions support from entity controller and introduce the language enabled helper function.
source_langcode is not really a property, as it's not defined as such, but from reading the code it looks like as it would be set as well? E.g. if I use getProperties() it should not appear, as it isn't a property.
I can't follow why that's happening and only calling the parent in one case. If it's necessary somehow, it needs a comment.
I'd expect the langcode to be an optional parameter, as it is for the single set() or the getProperties() method. I don't see why we magically treat $properties['langcode'] different it, $properties['langcode'] suggests it's just a regular property value. Given that, $properties['langcode'] should write to $entity->langcode.
Do we really need to validate the langcode before moving on? That means quite some additional function calls everywhere. On the contrary I don't think something bad would happen if a not (more) existing language code is used, except that no value for it will be found? Moreover I'd even expect that to happen, as else it looks like my invalid language code works, thus is valid.
I'm not sure why that should work? Shouldn't it use DISTINCT then and just select the id? The data is loaded in another step anyway plus that's the way we want to proceed (separate query from loading). I guess that's something that should be covered in the tests too.
Comment #25
plachI wasn't sure how to address all the issues raised in #24 so I changed the approach quite a bit. The attached patch:
source_langcode
to a booleandefault_langcode
, so that we don't need to introduce a way to specify the source language. The reasoning behind this is that since all of this is going away in favor of fields, it's better to simplify the current code and keep a clear distinction between properties and entity metadata. We will introduce translation metadata in a dedicate issue concerning entity translation.It's only a helper method that will be replaced by
EntityInterface::getFieldLangcode()
in real entity implementations. However I optimized it a bit.I think the attached patch covers more or less everything in #24. As I was saying in #12, let's not waste our time in trying to polish too much this code, as it is likely to be completely thrown away as soon as fields and properties are merged.
Comment #27
plachFixed the last direct property access in tests.
Comment #28
Gábor HojtsyStill looks good to me. I think there are two competing priorities here. First, this is a relatively simple entity and people might look at this for example, so we need it to be coded top-notch; however, it is "just a test entity" and is also our entry point for working on EFQ support for multilingual properties, so we need it done sooner than later.
@fago: any more feedback? :)
Comment #29
fagoExactly. Imho, in particular the test-cases are important, as those define how that all should work.
Could we add a test that makes sure the values can be retrieved without specifying a $langcode as well ?
docu doesn't match the method parameters.
Imho, it's the job of the caller to pass a sanitized array. The passed array is documented to contain properties.
Also, why should "langcode" not be a real property? I'd say it is, it's just not translatable.
I'd say that should be always the default behaviour, unless we specify a custom language to use for querying. For that, we've no API yet though ;-)
see above - that hasn't been addressed?
Yeah, still I was questioning whether we need to implement that language magic of "if given langcode X is invalid, go with language neutral" at all. As written in #24:
Thus, I think that getters/setters should
We've no way to specify whether a property is translatable yet, so for now maybe just add a helper method which hardcodes that 'langcode' is not translatable and mark that this needs to be replaced by property metadata.
@$this->langcodes: Good idea. Maybe we could just initialize the properties array for all existing langcodes. Then on set(), if there is no properties array for that langcode, throw the exception.
Comment #30
Gábor HojtsyYes, however, I would argue for not trying to make it right the first time unless it takes little time. There are way too many things that will spin out of this, especially the EFQ issue, but a lot of other things that need to happen, so if we don't get this 100% right the first time, I would not be ashamed :)
Comment #31
plachThe attached patch implements almost everything in #29.
@fago:
Guys, don't get me wrong: I don't mean we should commit crap, but trying to polish to death multilingual property handling, which is going to be removed, is a waste of time IMHO. Tests are ok because they define how things will work no matter whether we are dealing with fields or properties.
Well, the idea is that only the main language is a property the other ones are metadata. However I've done as suggested.
The attached patch exploits the
default_langcode
column to specify whether conditions should apply to original values, translated values or both. The default behavior is the first one as suggested above.I restored those lines already in the previous patch. I didn't move them in the translation tests because without them the test they belong to looks pretty meaningless.
Actually the main goal for
resolvePropertyLanguage()
was providing a default when no language was specified. The fallback was unintended and wrong so I removed the method altogether.Here is where I think we are going too far for the scope of this patch. We already have an established behavior for multilingual fields which resembles pretty closely the one you are describing above, except for the fact that untranslatable fields are always
LANGUAGE_NOT_SPECIFIED
, which is one thing I'd like to change as I was writing in the content language battleplan. I don't think it makes any sense to try and replicate this behavior here for properties, above all because the goal of the patch is testing multilingual support for them and defaulting to untranslatable would make everything pretty weird and probably harder to implement. Here I would just assume that properties are natively multilingual and by default get the same language of the entity (see the change tosetLangcode()
, which was needed also to fix a failing test).Initially I thought it would make more sense to throw the exception in both cases, but then I realized that if a language is uninstalled I might want to load (and therefore instantiate) an entity having that langauge and read its values, maybe for changing their language or performing some kind of recovery. For this reason the attached patch throws an exception only when setting a value for an invalid language.
I started implmenting this suggestion but afterwards determining the existing translations required a slightly more complex logic so I left this out. However now all languages, even locked ones, are allowed, otherwise
LANGUAGE_NOT_SPECIFIED
would suddenly become an illegal value.Comment #32
BerdirLooked through the code, I like the approach, wondering about the performance implications, but we'll see how that goes. I guess this makes having entity caching in core even more important :)
Below is some detailed feedback, mostly nitpicks, feel free to ignore the things you consider irrelevant at this time, I just wrote done anything that I noticed. A few things should be fixed IMHO, so setting to needs work. I also haven't read everything in here, so feel also free to ignore everything that has already been discussed.
There is also $this->fail() that you can use for this.
I'm not sure if there is a policy for this, but I always add a use Exception; instead of using the \.
False probably equals NULL (this does return NULL, right?) but it might be useful to make the comparison more explicit and assert against NULL instead of FALSE.
Assertion message is wrong here. (neutral => specific).
$langcode here is whatever the last value in the $this->langcodes array is, as it as been set by the foreach loop. Relying on that is a bit confusing, maybe set it explicitly?
This and the stuff below tests the deprecated $conditions argument. Wouldn't it be better to extend (if not done yet) and test EFQ instead of this? Can totally be a follow-up, but it's a bit strange that we work on improving $conditions instead of EFQ.
I know this is copied from the base Entity class but it just makes no sense. You can't have an optional first and a required second argument. I think this even throws an E_STRICT?
Do we want to fix it here and open a separate issue to fix the base class?
Wondering if this could be a static property so that we only need to set it the first time such an entity is instantiated?
Can't we use setProperties() here? If we move the id/langcode special case handling to that method then we don't need to repeat it in attachLoad()
We have different approaches for exception messages with placeholders and I don't really like this one :) Shouldn't hold this up though, although you might want to atleast add the missing ' at the end ;).
See above, if we move the unset into setProperties() then we don't need to duplicate that logic. Also, this isn't consistent with the code in the constructor, as it doesn't exclude the langcode?
I know this is just a first iteration but we're currently not checking anywhere what the allowed properties are. If there is something unexpected in there, it will blow up here with an sql exception. Improve that in a follow-up?
Comment #33
plach@Berdir:
Thanks for the review, the attached patch implements most of your suggestions.
Indeed :)
If I understood #1184272: Remove deprecated $conditions support from entity controller right, the current plans are to keep conditions and implement them through EFQ under the hood. However the work here is in preparation to make EFQs able to deal with the (hopefully) ephemeral multilingual properties.
This was my initial approach. @fago explicitly asked to change to the current form somewhere above.
I have absolutely no preference about this, I think I modeled that line after a patch of @sun. Now I'm wondering where core development is heading if not even being sun-compliant™ is safe anymore ;)
As I was writing above I'd really like to avoid spending too much time on those unholy property accessors: they are evil (because we have no package-level visibility modifiers) and should go away as soon as we have property-field unification. If for any reason they are staying (and I really hope they won't) we will totally need some form of validation based on property definitions.
Comment #34
plachComment #35
fagoGood work, the patch looks already pretty good.
yep, that does not belong into setProperties(). setProperties() should receive an array of property values, it's not its job to predict where the trash is in those arrays and to clean-up.
However, I'd agree that we should leverage setProperties() in __construct().
@default_langcode in $conditions:
I like having default_langcode to specify the language handling, but I don't like having it in $conditions. Again, $conditions is an array of property values to query for, but $default_langcode is something different so it does not belong in there.
We should have a separate parameter for that, which we don't have yet. Maybe just add an @todo to the tests and the controller that this should move to its separate parameter once the API is refactored.
ok, then let's do so. Maybe we could just add a note to the class that all properties are implemented multilingual for now as we've no way to marking a property as translatable yet.
Comment #36
plachAnd here it is :)
Comment #37
plach#36: drupal-et_ml_schema-1658712-36.patch queued for re-testing.
Comment #39
plachRerolled
Comment #40
plach#39 missed a hunk
Comment #42
plachReworked #36 after the UUID patch has landed. Basically I had to restore the initial approach taken in the storage controller and join on the data table if conditions are specified. Otherwise we would be forced to replicate the entire base table schema in the data table, which is not what we agreed upon in #1498634: [meta] Define revision/translation SQL schema for core entities (no patch), I believe. We are not making the
node.type
column part of the property data table, I guess.I thought about this some more and I don't think there is something wrong with this solution, since it does more or less the same of what an EFQ would do here. Since the plans are to reimplement conditions through an EFQ, the saved join would be reintroduced as soon as we start working on this.
@Gabor:
Aside from this last change @fago's and @berdir's concerns were addressed, thus if you are ok with it I guess this should be more ore leass ready to go.
Comment #43
plachOT:
We should really have an
EntityInterface::uuid()
method instead of accessing it through the property accessors. Those should work only with regular properties/fields.Comment #44
plachFYI, I created a new sandbox to work on the Entity Translation stuff: http://drupal.org/sandbox/plach/1719670.
Comment #45
Gábor HojtsyAll right, let's go with this one then. Thanks for sticking to it :)
Comment #46
fagoThis looks good to me as well.
ad #43: yep opened #1720784: Add method for getting an entity's UUID
Comment #47
fagoComment #48
Dries CreditAttribution: Dries commentedLooks good to me. Committed to 8.x to help D8MI make important progress. Thanks.
Comment #49
plachThanks! Do we need a change notice here?
Comment #50
BerdirI think so.
If appropriate, I'd recommend to create a generic change record for multilingual properties or so that contains an overview on the related changes, all affected entities/properties and so on. Any issue that is relevant can then be linked and the change record can be updated continously. That worked quite well for http://drupal.org/node/1400186.
Comment #51
sunComment #52
Gábor HojtsyI agree that documenting the change itself for the test entity is not that valuable, however, a general change notice that we can keep updated and referenced from related changes would look good.
Comment #53
plachOk, I'll post one tonight. If anyone wants to take care of this before, feel free.
Comment #54
plachWorking on the change notice.
Comment #55
plachHere it is: http://drupal.org/node/1722906.
Comment #56
BerdirLooks great to me. Now we need to move that $condition code to the EntityFieldQuery implementation, because I'm working on removing $conditions in #1184272: Remove deprecated $conditions support from entity controller.
Comment #57
BerdirComment #58
plachRelated issue: #1691952: Make EntityFieldQuery work with multilingual properties.
Comment #59
Gábor HojtsyMoving off the sprint. Thanks all for your fantastic work!