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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.33 KB

Schema changed. This should hopefully break tests in many ways.

Status: Needs review » Needs work

The last submitted patch, entity-test-multilingual-1.patch, failed testing.

Gábor Hojtsy’s picture

Sounds 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.

plach’s picture

Assigned: Unassigned » plach

Working on this.

Gábor Hojtsy’s picture

Priority: Normal » Major
plach’s picture

I have a semi-working (actually semi-breaking everything ;) patch. I'll try to complete it tonight.

plach’s picture

Priority: Major » Normal
Status: Needs work » Needs review
FileSize
9.34 KB

Ok, let's see how many things break NOW.

plach’s picture

Priority: Normal » Major

Status: Needs review » Needs work

The last submitted patch, drupal-et_ml_schema-1658712-5.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
12.25 KB

Fixed tests

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,111 @@
+  public function get($property_name, $langcode = NULL) {
+    $entity_info = $this->entityInfo();

Additional 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?

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,111 @@
+  public function setProperties($properties) {
+    $langcode = $this->propertyLangcode($properties->langcode);

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,111 @@
+  protected function propertyLangcode($langcode = NULL) {

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.

plach’s picture

Status: Needs work » Needs review
FileSize
12.94 KB

A 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:

Additional 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?

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.

If $properties is an object, then it at least needs to be documented as such. [...]

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.

We badly need to get rid of these ambiguous method names. [...]

I picked resolvePropertyLangcode() as it felt more self-documenting to me. This is going away too, in favor of Entity::getFieldLangcode(), which we might move to the resolve terminology, meanwhile.

Gábor Hojtsy’s picture

Looked 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?

plach’s picture

You're rght :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks generally good to me however, this looks suspicious:

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -0,0 +1,87 @@
+  protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
+    $query = db_select('entity_test_property_data', 'data');
+
+    $query->addTag($this->entityType . '_load_multiple');
+
+    // Select the actual data.
+    $query->fields('data');
+
+    // Set conditions.
+    foreach ($conditions as $field => $value) {
+      $query->condition('data.' . $field, $value);
+    }
+
+    return $query;

Where are the $ids taken into account?

plach’s picture

I 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...

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Yeah, looks like some tests are in order for querying test entities.

plach’s picture

Aside from tests, do we need anything else here? I guess experimenting with multilingual EFQs + related tests deserves a dedicated issue...

Gábor Hojtsy’s picture

@plach: right, that should be the next issue after we got this in :)

plach’s picture

OK, tests + new issue coming ASAP.

plach’s picture

Ok, 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.

plach’s picture

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now, has test coverage even for some querying and all issues with the query were fixed.

fago’s picture

Status: Reviewed & tested by the community » Needs work

Good work! I've reviewed the patch:

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityApiTest.php
@@ -78,15 +78,10 @@ class EntityApiTest extends WebTestBase {
-
-    // Make sure setting/getting translations boils down to setting/getting the
-    // regular value as the entity and property are not translatable.
-    $entity->set('uid', NULL, 'en');
-    $this->assertNull($entity->uid, 'Language neutral property has been set.');

Why is that being removed? Maybe it should be moved, but I don't see that case covered in the translation test case.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.php
@@ -116,4 +118,81 @@ class EntityTranslationTest extends WebTestBase {
+    $this->assertEqual($entity->language()->langcode, $langcode, t('Entity created as language neutral.'));

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.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.php
@@ -116,4 +118,81 @@ class EntityTranslationTest extends WebTestBase {
+    $this->assertEqual(count($entities), 0, t('No entity loaded by name translation without specifying a language.'));
+    $entities = entity_test_load_multiple(array(), array('langcode' => $langcode, 'name' => $properties[$langcode]['name']));
+    $this->assertEqual(count($entities), 1, t('One entity correctly loaded by name translation.'));

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.

+++ b/core/modules/entity/tests/modules/entity_test/entity_test.install
@@ -43,6 +43,39 @@ function entity_test_schema() {
+      'source_langcode' => array(

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,116 @@
+    if (empty($values['source_langcode'])) {
+      $this->langcode = $langcode;
+      parent::__construct($values, $entity_type);
+    }
+    else {
+      $this->entityType = $entity_type;
+    }

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,116 @@
+   *   A keyed array of properties to be set with their 'langcode' as one of the
+   *   keys. If no language is provided the entity langauge is used.
+   */
+  public function setProperties(array $properties) {

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,116 @@
+  protected function resolvePropertyLangcode($langcode = NULL) {

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
@@ -0,0 +1,97 @@
+    // Make sure we return one record for each entity even if no language has
+    // been specified.
+    if (empty($conditions['langcode'])) {
+      $query->condition('data.source_langcode', '');

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.

plach’s picture

I wasn't sure how to address all the issues raised in #24 so I changed the approach quite a bit. The attached patch:

  • Distinguishes more rigidly between properties and metadata (entity id and language information).
  • Changes the source_langcode to a boolean default_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.
  • Forces language conditions to act only on the default language values as per @fago's reccommendation.
  • Introduce a setter for the entity default langcode to be able to use the distinct clause in the query builder method and select only the entity id.

Do we really need to validate the langcode before moving on? [...]

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.

Status: Needs review » Needs work

The last submitted patch, drupal-et_ml_schema-1658712-25.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
18.52 KB
898 bytes

Fixed the last direct property access in tests.

Gábor Hojtsy’s picture

Still 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? :)

fago’s picture

Status: Needs review » Needs work

Still 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.

Exactly. Imho, in particular the test-cases are important, as those define how that all should work.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.php
@@ -116,4 +118,79 @@ class EntityTranslationTest extends WebTestBase {
+    $this->assertEqual($entity->language()->langcode, $langcode, 'Entity created as language neutral.');
+    $this->assertEqual($name, $entity->get('name', $langcode), 'The entity name has been correctly stored as a language-aware property.');
+    $this->assertEqual($uid, $entity->get('uid', $langcode), 'The entity author has been correctly stored as a language-aware property.');
+    $this->assertFalse($entity->get('name', LANGUAGE_NOT_SPECIFIED), 'The entity name is not available as a language neutral property.');
+    $this->assertFalse($entity->get('uid', LANGUAGE_NOT_SPECIFIED), 'The entity author is not available as a language neutral property.');

Could we add a test that makes sure the values can be retrieved without specifying a $langcode as well ?

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,129 @@
+   * @param array $properties
+   *   A keyed array of properties to be set with their 'langcode' as one of the
+   *   keys. If no language is provided the entity langauge is used.
+   */
+  public function setProperties(array $properties, $langcode = NULL) {

docu doesn't match the method parameters.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.php
@@ -0,0 +1,129 @@
+    // Make sure only real properties are stored.
+    unset($properties['id'], $properties['langcode'], $properties['default_langcode']);

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
@@ -0,0 +1,103 @@
+        if ($field == 'langcode') {
+          // Ensure that conditions involving the 'langcode' property concern
+          // only values in the original entity language.
+          $query->condition('data.default_langcode', 1);
+        }

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 ;-)

Why is that being removed? Maybe it should be moved, but I don't see that case covered in the translation test case.

see above - that hasn't been addressed?

It's only a helper method that will be replaced by EntityInterface::getFieldLangcode() in real entity implementations. However I optimized it a bit.

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:

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.

Thus, I think that getters/setters should

  • fallback to the language-neutral if a property is not marked as translatable.
  • go with the langcode given else. For get(), if it's invalid or if there is no value for it, simply return NULL - there is no translation for that langcode. Calling set() with an invalid language code should probably raise an exception. Or maybe the exception would even be a good idea for get() as well?

    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.

Gábor Hojtsy’s picture

Still 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.

Exactly. Imho, in particular the test-cases are important, as those define how that all should work.

Yes, 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 :)

plach’s picture

Status: Needs work » Needs review
FileSize
21.81 KB
13.86 KB

The attached patch implements almost everything in #29.

@fago:

Exactly. Imho, in particular the test-cases are important, as those define how that all should work.

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.

Also, why should "langcode" not be a real property? I'd say it is, it's just not translatable.

Well, the idea is that only the main language is a property the other ones are metadata. However I've done as suggested.

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 ;-)

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.

see above - that hasn't been addressed?

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.

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.

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.

Thus, I think that getters/setters should fallback to the language-neutral if a property is not marked as translatable.
[...]
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.

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 to setLangcode(), which was needed also to fix a failing test).

For get(), if it's invalid or if there is no value for it, simply return NULL - there is no translation for that langcode. Calling set() with an invalid language code should probably raise an exception. Or maybe the exception would even be a good idea for get() as well?

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.

@$this->langcodes: Good idea. Maybe we could just initialize the properties array for all existing langcodes.

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.

Berdir’s picture

Status: Needs review » Needs work

Looked 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.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+      // This line is not expected to be executed unless something goes wrong.
+      $this->assertTrue(FALSE, $message);

There is also $this->fail() that you can use for this.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+    catch (\Exception $e) {
+      $this->assertTrue($e instanceof \InvalidArgumentException, $message);

I'm not sure if there is a policy for this, but I always add a use Exception; instead of using the \.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+    $this->assertFalse($entity->get('name', $langcode), 'The entity name is not available as a language-aware property.');
+    $this->assertFalse($entity->get('uid', $langcode), 'The entity author is not available as a language-aware property.');

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.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+    $this->assertEqual($entity->language()->langcode, $langcode, 'Entity created as language neutral.');

Assertion message is wrong here. (neutral => specific).

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+    entity_create('entity_test', array('uid' => $properties[$langcode]['uid']))->save();

$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?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -115,5 +117,105 @@ class EntityTranslationTest extends WebTestBase {
+    $entities = entity_test_load_multiple(array(), array('name' => $name), TRUE);

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.

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
@@ -0,0 +1,134 @@
+  public function __construct(array $values = array(), $entity_type) {

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?

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
@@ -0,0 +1,134 @@
+    // The allowed languages are simply all the ones available in the system.
+    $this->langcodes = drupal_map_assoc(array_keys(language_list(LANGUAGE_ALL)));

Wondering if this could be a static property so that we only need to set it the first time such an entity is instantiated?

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
@@ -0,0 +1,134 @@
+    // Set initial values ensuring that only real properties are stored.
+    foreach ($values as $property_name => $value) {
+      if ($property_name != 'id' && $property_name != 'langcode' && $property_name != 'default_langcode') {
+        $this->properties[$this->langcode][$property_name] = $value;

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()

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTest.phpundefined
@@ -0,0 +1,134 @@
+      throw new \InvalidArgumentException("Detected an invalid language '$langcode' while setting '$property_name' to '$value.");

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 ;).

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -0,0 +1,113 @@
+      // Make sure only real properties are stored.
+      unset($values['id'], $values['default_langcode']);
+      $entity->setProperties($values, $langcode);

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?

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -0,0 +1,113 @@
+      $properties = $entity->getProperties($langcode);
+
+      $values = array(
+        'id' => $entity->id(),
+        'langcode' => $langcode,
+        'default_langcode' => intval($default_langcode == $langcode),
+      ) + $properties;

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?

plach’s picture

@Berdir:

Thanks for the review, the attached patch implements most of your suggestions.

Looked 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 :)

Indeed :)

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.

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.

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()

This was my initial approach. @fago explicitly asked to change to the current form somewhere above.

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 ;).

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 ;)

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?

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.

plach’s picture

Status: Needs work » Needs review
fago’s picture

Good work, the patch looks already pretty good.

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()

This was my initial approach. @fago explicitly asked to change to the current form somewhere above.

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.

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 to setLangcode(), which was needed also to fix a failing test).

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.

plach’s picture

plach’s picture

Status: Needs review » Needs work
Issue tags: +Entity system, +D8MI, +sprint, +language-content

The last submitted patch, drupal-et_ml_schema-1658712-36.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
22.41 KB

Rerolled

plach’s picture

#39 missed a hunk

Status: Needs review » Needs work

The last submitted patch, drupal-et_ml_schema-1658712-40.patch, failed testing.

plach’s picture

Reworked #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.

plach’s picture

OT:

We should really have an EntityInterface::uuid() method instead of accessing it through the property accessors. Those should work only with regular properties/fields.

plach’s picture

FYI, I created a new sandbox to work on the Entity Translation stuff: http://drupal.org/sandbox/plach/1719670.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All right, let's go with this one then. Thanks for sticking to it :)

fago’s picture

Status: Reviewed & tested by the community » Needs work

This looks good to me as well.

ad #43: yep opened #1720784: Add method for getting an entity's UUID

fago’s picture

Dries’s picture

Status: Needs work » Fixed

Looks good to me. Committed to 8.x to help D8MI make important progress. Thanks.

plach’s picture

Thanks! Do we need a change notice here?

Berdir’s picture

Title: Refactor test_entity schema to be multilingual » Change notification for: Refactor test_entity schema to be multilingual
Priority: Major » Critical
Status: Fixed » Active

I 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.

sun’s picture

Issue tags: +API change
Gábor Hojtsy’s picture

I 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.

plach’s picture

Assigned: plach » Unassigned

Ok, I'll post one tonight. If anyone wants to take care of this before, feel free.

plach’s picture

Assigned: Unassigned » plach

Working on the change notice.

plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review
Berdir’s picture

Priority: Critical » Major
Status: Needs review » Fixed

Looks 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.

Berdir’s picture

Title: Change notification for: Refactor test_entity schema to be multilingual » Refactor test_entity schema to be multilingual
plach’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Moving off the sprint. Thanks all for your fantastic work!

Automatically closed -- issue fixed for 2 weeks with no activity.