Problem/Motivation

The use of translated field descriptions in base field definitions and their inclusion in the entity database schema as column descriptions results in update.php changes if translations for those strings are available.

To reproduce, install in a non-en language, then import translations or manually translate something like "The node ID." to your language. Make sure to clear all caches after that.

Then visit update.php => oops.

It should run through, but that is no longer the case as soon as you have content, and it will also change in random ways depending on which language was used when the old values were generated. So if you then add another language, make that the default and visit newlanguage/update.php (it is a normal route now, so that actually works), it will be different *again*.

Additionally to that, descriptions are also used both as UI hints and DB schema descriptions, which is a dual role they cannot fulfill well. See #2350013: Descriptions for Promotion Options are too technical. We need to either make them database descriptions or UI descriptions but not both.

Proposed resolution

Don't include descriptions in the entity database schema. This was a nice to have, not a mandatory feature. Most base field descriptions are so basic, they provide no additional information beyond the column name.

Calling t() as part of defining fields can lead to performance and other problems and we should avoid that, but solving that problem requires more changes and has been moved to #2418521: Translating field definition descriptions problematic due to early t(), hard to test.

Remaining tasks

Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#136 interdiff.txt600 bytesGábor Hojtsy
#136 2363099-no-description-in-schema-136.patch14.55 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,886 pass(es). View
#129 2363099-no-description-in-schema-129-interdiff.txt852 bytesBerdir
#129 2363099-no-description-in-schema-129.patch13.96 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,882 pass(es). View
#126 interdiff.txt714 bytesGábor Hojtsy
#126 2363099-no-description-in-schema-126.patch13.96 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,589 pass(es). View
#113 interdiff.txt2.21 KBGábor Hojtsy
#113 2363099-no-description-in-schema-113.patch13.83 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,558 pass(es). View
#113 2363099-no-description-in-schema-113-test-only.patch3.17 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,948 pass(es), 3 fail(s), and 0 exception(s). View
#111 interdiff.txt712 bytesGábor Hojtsy
#111 2363099-no-description-in-schema-111.patch12.51 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,900 pass(es). View
#111 2363099-no-description-in-schema-111-test-only.patch1.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,936 pass(es), 1 fail(s), and 0 exception(s). View
#108 2363099-no-description-in-schema-108.patch12.51 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,934 pass(es), 1 fail(s), and 1 exception(s). View
#108 2363099-no-description-in-schema-108-test-only.patch1.85 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,925 pass(es), 1 fail(s), and 1 exception(s). View
#101 2363099-no-description-in-schema-101-interdiff.txt712 bytesBerdir
#101 2363099-no-description-in-schema-101.patch10.66 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,901 pass(es). View
#99 interdiff.txt8.66 KBGábor Hojtsy
#99 2363099-no-description-in-schema-99.patch10.33 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,889 pass(es), 1 fail(s), and 0 exception(s). View
#93 2363099-no-description-in-schema.patch2.02 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,882 pass(es), 5 fail(s), and 0 exception(s). View
#87 t-in-definition-labels-2363099-87.patch110.85 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,551 pass(es). View
#81 t-in-definition-labels-2363099-81.patch111.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,148 pass(es). View
#77 interdiff.txt1.61 KBGábor Hojtsy
#77 t-in-definition-labels-2363099-77.patch111.09 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch t-in-definition-labels-2363099-77.patch. Unable to apply patch. See the log in the details link for more information. View
#75 t-in-definition-labels-2363099-75.patch110.66 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,883 pass(es), 24 fail(s), and 13 exception(s). View
#75 interdiff.txt9.16 KBGábor Hojtsy
#74 t-in-definition-labels-2363099-74.patch106.82 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,940 pass(es). View
#70 t-in-definition-labels-2363099-70-interdiff.txt765 bytesBerdir
#70 t-in-definition-labels-2363099-70.patch108.67 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,362 pass(es). View
#68 t-in-definition-labels-2363099-68.patch108.67 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#59 interdiff.txt656 bytesGábor Hojtsy
#59 t-in-definition-labels-2363099-59.patch107.47 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,381 pass(es). View
#57 interdiff-46-to-57.txt3.17 KBGábor Hojtsy
#57 t-in-definition-labels-2363099-57.patch106.83 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch t-in-definition-labels-2363099-57.patch. Unable to apply patch. See the log in the details link for more information. View
#55 interdiff.txt4.35 KBGábor Hojtsy
#55 t-in-definition-labels-2363099-54.patch102.52 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#53 interdiff-wannabe.txt6.55 KBGábor Hojtsy
#46 Drupal database update d8.png85.05 KBBerdir
#46 t-in-definition-labels-2363099-45-interdiff.txt5.59 KBBerdir
#46 t-in-definition-labels-2363099-45.patch104.77 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,370 pass(es). View
#42 interdiff.txt1.11 KBswentel
#42 t-in-definition-labels-2363099-41.patch103.07 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,379 pass(es), 4 fail(s), and 0 exception(s). View
#38 interdiff.txt7.96 KBswentel
#38 t-in-definition-labels-2363099-37.patch103.29 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,384 pass(es), 4 fail(s), and 0 exception(s). View
#38 t-in-definition-labels-2363099-37-fail.patch102.54 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,379 pass(es), 1 fail(s), and 0 exception(s). View

Comments

Berdir’s picture

Issue tags: +Entity Field API
Berdir’s picture

Title: Translating field definition labels results results in different schema definitions, resulting in update.php changes » Translating field definition descriptions results results in different schema definitions, resulting in update.php changes

Descriptions, actually

Berdir’s picture

Assigned: Unassigned » plach
plach’s picture

Assigned: plach » Unassigned

I completely agree we should have untranslated labels/descriptions in our SQL schema. I also agree those early t() calls are not pretty, but I am wondering whether to fix this issue it would be enough to follow the approach used by dedicated field tables and just hard-code a few english strings.

Your analysis/proposed solution is valid but I am not sure removing early t() calls and fixing this issue need to be tied, although going the way you propose would fix both. OTOH fixing early t() calls is somehow "optional", and we might be able to find a solution that does not make string translation discovery harder, while fixing this is definitely required :)

Berdir’s picture

Status: Active » Needs review
FileSize
92.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/src/Entity/File.php. View

@plach: Thanks for the feedback. The problem isn't really " t() in early bootstrap" but more specifically "t() while building entity definitions". That can result in very nasty recursions, see the mess in #2345611: Load user entity in SessionHandler instead of using manual queries, which is following an existing pattern where we had to add a few such fixes already to fix http authentication.

This removes all t() calls that I could find for Data and FieldDefinition. We can still do that elsewhere if we want to do a different fix here to get rid of the critical problem.

Status: Needs review » Needs work

The last submitted patch, 5: t-in-definition-labels-2363099-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
92.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/image/src/Plugin/Field/FieldType/ImageItem.php. View
612 bytes

Fixed the ) in File.php

Status: Needs review » Needs work

The last submitted patch, 7: t-in-definition-labels-2363099-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
92.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,665 pass(es), 3 fail(s), and 30 exception(s). View
1014 bytes

Another one of those fixed.

Status: Needs review » Needs work

The last submitted patch, 9: t-in-definition-labels-2363099-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
92.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,469 pass(es), 0 fail(s), and 31 exception(s). View

Fixing those test fails.

Status: Needs review » Needs work

The last submitted patch, 11: t-in-definition-labels-2363099-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
93.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,473 pass(es). View
1.68 KB

Another try.

plach’s picture

I am wondering whether it would make sense to have an additional method on StringTranslationTrait, for instance StringTranslationTrait::nt($string), that would return the string as is but that would make it discoverable by potx/l10n server.

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -163,7 +163,8 @@ public function requiresFieldStorageSchemaChanges(FieldStorageDefinitionInterfac
    +      $return = $schema != $this->loadFieldSchemaData($original);
    +      return $return;
    

    Is this a debug leftover?

  2. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\DependencyInjection\DependencySerializationTrait;
    

    Is this used?

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -7,11 +7,21 @@
    +    __sleep as traitSleep;
    

    Would it make sense to use a less generic name? This would make easier to know which __sleep we are calling in case we added more traits implementing a __sleep method.

  4. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -96,7 +114,7 @@ public function setLabel($label) {
    +    return isset($this->definition['description']) ? $this->t($this->definition['description']) : NULL;
    

    Is there any reason for supporting arguments only on labels?

plach’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Also :)

Gábor Hojtsy’s picture

Discussed this again at the D8MI meeting. A noop function could work here because the definitons are cached, so the overhead of the noop would not be measurable in runtime. We'll need to take care of others not misusing the noop. nt() or markt() or tmark() or something along those line may work. Do we only need this for single strings or also for possibly singular/plural pairs?

Berdir’s picture

Status: Needs work » Needs review
FileSize
92.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch t-in-definition-labels-2363099-17.patch. Unable to apply patch. See the log in the details link for more information. View
1.31 KB

1. Yes, reverted.
2. Not anymore, removed.
3. Any specific suggestions? I copied this from the Entity base class.
4. No, but core only had a use case for labels.

Yes, needs tests. This isn't meant to be a complete patch, just wanted to get the ball rolling.

Yeah, I'm ok with adding a dummy wraper, but it would be even better if we could avoid it completely and somehow parse this case automatically, as it is very common. I'm for sure not looking forward to adding to re-adding a dummy method/function call again on all those cases ;)

plach’s picture

Mmh, d.o. ate my comment, so here is the short version:

Any specific suggestions? I copied this from the Entity base class.

Maybe dependencySerializationTraitSleep()?

Gábor Hojtsy’s picture

As I have said earlier, I am also fine with not introducing a no-op t and instead special case support setLabel() and setDescription() on base field definitions in potx. If we can keep the number of such special cases controlled, we are fine. Eg. Drupal 7 had similar special cases for hook_menu() titles and descriptions and Drupal 6 for hook_perm(). It is not an alien thing. It becomes very painful once there are too many special cases.

yched’s picture

special case support setLabel() and setDescription() on base field definitions in potx

Field definitions can live in different places / hooks. Wouldn't that be hard to detect without false positives ?

Gábor Hojtsy’s picture

@yched: no, we could special case setLabel() and setDescription() when inside a baseFieldDefinitions(), but once it spills out of there it becomes unwieldy. I don't think that would be practical. I am as said above fine with introducing a marker. @catch just bumped #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx).

Status: Needs review » Needs work

The last submitted patch, 17: t-in-definition-labels-2363099-17.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
92.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,056 pass(es). View

Just a re-roll for now.

swentel’s picture

FileSize
92.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,193 pass(es). View

Another reroll - what steps (besides tests) are we missing here ?

Gábor Hojtsy’s picture

Well as per #20 and #21 sounds like we may need a no-op t() so this works across other hooks as well for string detection.

Gábor Hojtsy’s picture

FileSize
97.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
93.66 KB

Introducing a no-op t() as nt() then. I also found an instance of setDescription() that used a replacement value, so we need the replacement argument logic the same way in setDescription like in setLabel. Finally found a couple calls to setLabel and setDescription that did not have t() yet. Added nt().

I briefly thought about nt() being able to take the t() arguments as well, but it NEEDS to return a string for it to be a no-op and retain the argument array at the same time. So it cannot be a drop-in replacement for t(), it cannot have the same API. Yeah...

plach’s picture

Status: Needs review » Needs work
Issue tags: +sprint, +Ghent DA sprint
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -67,17 +67,17 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
    +      ->setLabel($target_type_info->getLabel(), NULL)
    

    Do we need this?

  2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -7,11 +7,21 @@
    +    __sleep as traitSleep;
    

    I'd still prefer dependencySerializationTraitSleep if no one is against that.

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -83,12 +96,17 @@ public function getLabel() {
    +   *   on the first character of the key, the value is escaped and/or themed.
    
    @@ -104,12 +125,17 @@ public function getDescription() {
    +   *   on the first character of the key, the value is escaped and/or themed.
    

    'on' can go in the previous row.

And still needing tests :)

The last submitted patch, 27: t-in-definition-labels-2363099-27.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
99.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
5.89 KB
99.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Have a basic test working + addressing #28

Test probably needs cleanup and not sure about String::format in DataDefinition but we can talk that through tomorrow.

The last submitted patch, 30: t-in-definition-labels-2363099-29.patch, failed testing.

The last submitted patch, 30: t-in-definition-labels-2363099-29-fail.patch, failed testing.

plach’s picture

FileSize
99.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,171 pass(es), 1 fail(s), and 0 exception(s). View
100 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,169 pass(es). View
617 bytes

A small fix to a unit test

The last submitted patch, 33: t-in-definition-labels-2363099-33-fail.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -76,7 +76,7 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-      ->setLabel($target_type_info->getLabel(), NULL)
+      ->setLabel($target_type_info->getLabel())
       ->setDescription(nt('The referenced entity'))

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -86,7 +87,7 @@ public function setDataType($type) {
     if (isset($this->definition['label_arguments'])) {
-      return $this->t($this->definition['label'], $this->definition['label_arguments']);
+      return String::format($this->definition['label'], $this->definition['label_arguments']);
     }
     return isset($this->definition['label']) ? $this->t($this->definition['label']) : NULL;

Not sure I get the changes here.

The explicit NULL was to avoid translating the target entity type a second time (similar to how watchdog works), but not sure if it was ever implemented properly.

And why change to String::format()? My idea was to have a getUntranslatedLabel() or so method, that we can use.

Berdir’s picture

Status: Needs review » Needs work

I think the test should be in locale module and be renamed to TranslatedFieldStorageDefinitionTest or something like that.

And we should verify that getLabel() actually does return the translated label, because I think the change you made broke exactly that :)

Gábor Hojtsy’s picture

@berdir: The NULL handling in getLabel() and then I assume getDescription() too would need to be implemented. I think you only had that idea but did not get to it :)

swentel’s picture

Status: Needs work » Needs review
FileSize
102.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,379 pass(es), 1 fail(s), and 0 exception(s). View
103.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,384 pass(es), 4 fail(s), and 0 exception(s). View
7.96 KB

Went for getUntranslatedLabel() and getUntranslatedDescription() for now, got lost in the translation wrapper. Moved the test file as well.

Gábor Hojtsy’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -337,6 +337,13 @@ public function getLabel() {
    +    return $this->label();
    
    @@ -344,6 +351,13 @@ public function getDescription() {
    +    return $this->description;
    

    Do the field config label/description string not have arguments?

  2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -87,12 +87,22 @@ public function setDataType($type) {
    +    return isset($this->definition['label']) ? String::format($this->definition['label']) : NULL;
    
    @@ -116,12 +126,22 @@ public function setLabel($label, $arguments = array()) {
    +    return isset($this->definition['description']) ? String::format($this->definition['description']) : NULL;
    

    Is there any reason to format the single string?

swentel’s picture

re 1: no - unless I'm missing something
re 2: urg right, that's not needed indeed

Gábor Hojtsy’s picture

@swentel: so I was looking at why do you need these two new methods on FieldConfigBase, and none of the interfaces mandate it? So should it be on DataDefinitionInterface where getLabel and getDescription it? It may not be added on any interface yet if I'm not mistaken.

swentel’s picture

FileSize
103.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,379 pass(es), 4 fail(s), and 0 exception(s). View
1.11 KB

What was I thinking.

The last submitted patch, 38: t-in-definition-labels-2363099-37-fail.patch, failed testing.

The last submitted patch, 38: t-in-definition-labels-2363099-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 42: t-in-definition-labels-2363099-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
104.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,370 pass(es). View
5.59 KB
85.05 KB

Fixed the unit test, also made a few small changes to add support for NULL. We need to discuss if we want to allow/force TranslationWrapper instead.

I also tested this manually, if you install in german in HEAD, then you get the following list of updates. Applying the patch fixes it, no pending updates then.

Gábor Hojtsy’s picture

Re my comment in #41, they are already on DataDefinitionInterface, my mistake...

fago’s picture

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -83,12 +114,18 @@ public function getLabel() {
+   * @param array|null $arguments
+   *   An associative array of replacements to make after translation. Based on
+   *   the first character of the key, the value is escaped and/or themed.
+   *   See \Drupal\Component\Utility\String::format() for details. NULL means
+   *   that the label is already translated.

Imho that overloading is quite a WTF - NULL vs array() makes the string different!? Also, if passed that way getUntranslatedFoo() will lie to you.

Couldn't we support translation wrapper arguments instead, add un-wrapping support to them and unwrap them in getUntranslatedFoo()?

Berdir’s picture

Ok, discussed with @fago:

- Add the untranslated methods to FieldStorageDefinitionInterface too, as we do not extend from DataDefinitionInterface there
- Instead of the NULL stuff, allow to pass in a TranslationWrapper, check if we have one in getLabel()/getDescription(), if so, return it
- Instead of $this->t(), create a new TranslationWrapper() object so that we always return a TranslationWrapper
- Fix what breaks when we do that ;)

jibran’s picture

Title: Translating field definition descriptions results results in different schema definitions, resulting in update.php changes » Translating field definition descriptions results in different schema definitions, resulting in update.php changes
fago’s picture

Status: Needs review » Needs work

This needs work then.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
6.55 KB

So I worked on this but I don't think this is the right direction.... It sounds like way too much disruption for what is tried to being achieved here. We can change the DataDefinitionInterface to return a translation wrapper for getLabel() and getDescription(). BUT then to be consistent with field storage definitions, we need to change the getLabel() (delegated to label()) method there, which comes all the way from EntityInterface. I don't think that much change for this is worth it?

Here is a partial interdiff-wannbe at the point where I stopped...

Berdir’s picture

Yes, that doesn't work, FieldStorageConfig can never return a TranslationWrapper as it is a configuration entity and that is very different anyway (I do wonder what getUntranslatedLabel() should do there exactly..)

Let's wait with all the TranslationWrapper stuff for now, I don't think that's the right direction.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
102.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
4.35 KB

Just updating the data definition interface then with some fixes and the field storage interface with the two new methods.

Status: Needs review » Needs work

The last submitted patch, 55: t-in-definition-labels-2363099-54.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
106.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch t-in-definition-labels-2363099-57.patch. Unable to apply patch. See the log in the details link for more information. View
3.17 KB

Duh that was not rolled the right way. Not rolling again both the interdiff and the patch as compared to 47.

Status: Needs review » Needs work

The last submitted patch, 57: t-in-definition-labels-2363099-57.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
107.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,381 pass(es). View
656 bytes

Now FieldStorageConfig needs to implement it then. It is the third place we implement the same stuff now then.

Berdir’s picture

@yched and I were discussing that it might be better to have a more specific method on FieldStorageDefinitionInterface, we would suggest to name that getSchemaDescription()*. We're not sure if we need the generic methods there, the copy/duplicate case is pretty special and I'm not sure what exactly you need/want there.

Some feedback from @fago would be great for that :)

* We also agreed that we should actually return the label there for base fields, but that doesn't change the problem here.

Gábor Hojtsy’s picture

Assigned: Unassigned » fago

Assigned to @fago for feedback then.

fago’s picture

Assigned: fago » Unassigned
Status: Needs review » Needs work

getSchemaDescription()

Not sure about that - if it's the field storage definition which should control the schema, why not just return it from getSchema()? It's where it will go anyway.

* We also agreed that we should actually return the label there for base fields, but that doesn't change the problem here.

Makes sense to me.

BUT then to be consistent with field storage definitions, we need to change the getLabel() (delegated to label()) method there, which comes all the way from EntityInterface. I don't think that much change for this is worth it?

Right, that definitely goes too far then :/

So two issues then:
1.

+   * @param array|null $arguments
+   *   An associative array of replacements to make after translation. Based on
+   *   the first character of the key, the value is escaped and/or themed.
+   *   See \Drupal\Component\Utility\String::format() for details. NULL means
+   *   that the label is already translated.

This overloading is ugly/bad style, but moreover it's broken as getUntranslatedLabel() would not return an untranslated label any more. Subsequent code that would like to run t() on the unstranslated label would fail / try to translate translations. Not sure for which cases this is needed.

2.
How can we make the copy the definition object acutally work? E.g.
$copy->setLabel($orig->getUntranslatedLabel());
Now, $copy->getLabel() is not translated any more as replacements have been applied. So, should getUntranslatedLabel return the string + the argument instead? Or have two methods return each, or a "StringWrapper" object similar to TranslationWrapper, but which allows you to return each and formats the string in __toString()?

Gábor Hojtsy’s picture

Assigned: Unassigned » fago

@fago: Breaking up getLabel() to string and replacement may help in some cases, but we already have derived items where they would get the translated labels (eg. based on configuration information which naturally comes translated). So if we want to be so sure not to deal with translated info, we would need to load those untranslated as well. Translating them with t() later would not work with the configuration translation system (your live configuration is not your code, so is it not exposed to t()). So the whole premise of being sure you only get untranslated stuff is broken.

What we rely on labels and descriptions for and where do we need translated and untranslated things? (Assigning back to @fago but could very well be @Berdir too).

Berdir’s picture

Not sure about that - if it's the field storage definition which should control the schema, why not just return it from getSchema()? It's where it will go anyway.

The description is currently separate from the schema because the schema might have multiple columns, so there's not really a place to put it (top level maybe). But that doesn't solve the problem, then getSchema() in the plugins would need it.

Note that the schema description concept is currently only used for shared storage fields although we could theoretically use it as a table description for dedicated storage fields. But practically, this only really relevant for base fields atm.

Also keep in mind that field storage configs currently do not have a description and only a generated name that would not help much.

The last submitted patch, 57: t-in-definition-labels-2363099-57.patch, failed testing.

plach’s picture

Issue tags: +entity storage
Berdir’s picture

Status: Needs work » Needs review
FileSize
108.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Reroll for a start.

Status: Needs review » Needs work

The last submitted patch, 68: t-in-definition-labels-2363099-68.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
108.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,362 pass(es). View
765 bytes

Stupid typo.

Gábor Hojtsy’s picture

Honestly I am not sure if we have a direction here to go to? I would happily sit down and implement changes according to said direction on the other hand.

Regnoy’s picture

Apply patch(t-in-definition-labels-2363099-70.patch) and have this error.

Class 'Drupal\Core\Field\Plugin\Field\FieldType\TranslationWrapper' not found in C:\wamp\www\drupal8\core\lib\Drupal\Core\Field\Plugin\Field\FieldType\StringItemBase.php on line 35

fago’s picture

Assigned: fago » Unassigned

What we rely on labels and descriptions for and where do we need translated and untranslated things? (Assigning back to @fago but could very well be @Berdir too).

Usually, we want to the translated string. But in some cases we need the untranslated string, e.g. what I'm aware of is BaseFieldDefinition::createFromFieldStorageDefinition() which needs to copy over the untranslated string for later translation. This is what I referred to with #62.2).

Breaking up getLabel() to string and replacement may help in some cases, but we already have derived items where they would get the translated labels (eg. based on configuration information which naturally comes translated).

hm, right. As there is no way to fetch them untranslate + translate those strings later, I think the best we could do here is getting the translated string and do not try to translate it later on. This would just work and not break things. But as pointed out in 1), the problem is that the string returned from getUntranslatedLabel() still *needs translation* but will fail translation due to applied replacements.

Imho, the best solution would be to go with a returned object ala StringWrapper, which can give you the string template, the string replacements and the replaced string. Then, we could return that from getUntranslatedLabel() and use it when copying ala #62.2. Howsoever, if you think a "simpler" solution with adding more methods is more viable at that point, that's fine to me - but I do think we need to keep copy-ability ala #62.2 working.

Gábor Hojtsy’s picture

FileSize
106.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,940 pass(es). View

Rerolled first.

Gábor Hojtsy’s picture

FileSize
9.16 KB
110.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,883 pass(es), 24 fail(s), and 13 exception(s). View

All right, is this what you had in mind @fago? Added a StringWrapper that can wrap a string and arguments. Used that as return value of getUntranslated*. Updated docs. Added @todo where we are blatantly lying about it being untranslated. We should do something about that. Updated BaseFieldDefinition::createFromFieldStorageDefinition() to take the label/description string/arguments from the wrapper.

Status: Needs review » Needs work

The last submitted patch, 75: t-in-definition-labels-2363099-75.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
111.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch t-in-definition-labels-2363099-77.patch. Unable to apply patch. See the log in the details link for more information. View
1.61 KB

Of course we cannot use getUntralated*()s return value as an object it may be null. Such ugliness... Eh :/

Gábor Hojtsy’s picture

Assigned: Unassigned » fago

Back at @fago for review of #75/#77.

Status: Needs review » Needs work

The last submitted patch, 77: t-in-definition-labels-2363099-77.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
111.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,148 pass(es). View

Quick reroll. Patch applied with some offsets.

dawehner’s picture

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -75,20 +87,41 @@ public function setDataType($type) {
+    // If it was explicitly set to NULL, assume the label is already translated.

Can we maybe introduce some CONSTANT so its less magic involved?

Gábor Hojtsy’s picture

Another way to go about this is we use a TranslationWrapper in place of nt(). Add methods on TranslationWrapper to get the source data back out and therefore no need to introduce a StringWrapper either. Does that sound better? Anything against it?

Berdir’s picture

Having to manually create a TranslationWrapper object for all those cases would be annoying IMHO.

Gábor Hojtsy’s picture

BTW the issue summary on that one says:

We could also always use a translation wrapper and add a method to get the untranslated text out on that, but manually using the translation wrapper in those places is very annoying and people will forget to do that. See the issue referenced above.

I don't think we need to manually use the translation wrapper if we have it used in a getUntranslated* method family?

Gábor Hojtsy’s picture

@Berdir: well, it would be new TranslationWrapper('x') instead of nt('x') and you would use a common solution instead of yet one more way to do something.

Gábor Hojtsy’s picture

FileSize
110.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,551 pass(es). View

Patch did not apply due to LinkItem.php changes. Rerolled again.

Gábor Hojtsy’s picture

I updated http://hojtsy.hu/blog/2013-jul-24/drupal-8-multilingual-tidbits-10-conte... this week and it shows clearly that you already need to know to use t/trans/translate/@Translate/TranslationWrapper depending on context (PHP, twig, PHP, annotation, PHP) or none (static YAMLs) or schema (config YAMLs) to translate things. It would be great not to need to introduce nt() and StringWrapper to this already confusing mix IMHO if we can just as easily get away with TranslationWrapper, no? What are upsides for introducing yet more translation APIs? I think being unclear on these is a facet due to which this is not a very happy issue.

effulgentsia’s picture

If the only reason to not use ->setLabel(new TranslationWrapper(...)) is verbosity / exposing the wrapper concept in too many lines of code, what if we add a DataDefinition::t() method that wraps that: i.e., ->setLabel(DataDefinition::t(...))?

effulgentsia’s picture

Also, DataDefinition::t() doesn't have to return a TranslationWrapper. It could return a subclass (DataDefinitionTranslationWrapper?) which contains a getUntranslated() method, if we don't want to add such a method to TranslationWrapper. Which might be good, since the need to get an untranslated string is a bit special for data definitions, as it relates to our desire for the DDL to not be localized.

yched’s picture

Sorry for kicking in so late :-/, but couldn't we simply stop ading the description in the db schema ?
The "description" main use is currently to be displayed by the widgets as a FAPI #description in entity forms. The texts in there are displayed in the UI as hints for content authors, they are thus highly UI-oriented and contextual.

In most cases, that same UI-oriented string makes little sense in a db schema technical description.
#2350013: Descriptions for Promotion Options are too technical is a good example of "description" being wrong for describing a data model.

Plus :

- we only have a description for the field, while db columns are for the various properties. "Field type has one single column" degrades into "create one single column and name it directly like the field in base tables", but that's a very specific case. General case is : a field spans over several columns. So what's the point of repeating the field description for each column ?

- If I'm not mistaken (I'm not in front of my PhpStorm atm), FieldStorageConfig do not have a description, only FieldConfig (specific instances on specific bundles) have one - because the description is for widgets. So when creating the schema for a configurable FieldStorage, there's no description available (schema is for all bundles the field might appear) ?
BaseFieldDefinitions do have a description, since they're both a FSD and a FD. But if we can't put a description for configurable field table schemas, why should we for base tables ?

Gábor Hojtsy’s picture

@yched: there are other benefits to not t()ing the label/description right away, as explained to me by Berdir before, for example, in most cases you don't actually need the label/description, just the typing information, in which case you don't need to translate it (or retrieve / maintain a cache of that translation). I personally would be fine not adding the description in the db schema. If we agree on that, then we should look at if there are other things that make this a critical issue or its more like a nice to have.

Gábor Hojtsy’s picture

FileSize
2.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,882 pass(es), 5 fail(s), and 0 exception(s). View

So here is a minimal patch then that foregoes using the description which is the critical part of this issue, right? Let's discuss this one. No interdiff because this is a totally new one.

Status: Needs review » Needs work

The last submitted patch, 93: 2363099-no-description-in-schema.patch, failed testing.

Berdir’s picture

See points 1., 3. and 4. in the issue summary about other problems with calling t() there and why avoiding that would be good IMHO.

That said, if the easiest way to move forward is to just remove that, then that is OK with me.

I assume we did that to keep existing schemas before/after the schema generation as identical as possible, in many cases, we curently use the old schema descriptions as descriptions on the base field definitions.

Gábor Hojtsy’s picture

@Berdir: right, as I said the question is if 1, 3 and 4 from the issue summary make this critical or is 2 the part that makes it critical.

Berdir’s picture

That's hard to say. The fact that is currently impossible to run update.php for a localized site is certainly the most critical part about this issue (which is neither of those 4 points, they're all additions).

#2345611: Load user entity in SessionHandler instead of using manual queries is currently only a normal task, but it should probably be at least major, as we currently hardcode assumptions about the entity storage outside of the entity storage system. Not critical, I guess, so something that would unblock/greatly simplify that issue wouldn't be critical either.

4. is very hard to estimate for the performance impact, but likely also not critical.

Removing the description from the storage would have some nice implications, we currently have multiple UX issues open because we show those database descriptions in the UI to normal users, confusing the hell out of them. No longer using them for the storage would make it much easier to deal with those issues, we can just add what makes sense for the UI.

effulgentsia’s picture

If there's agreement here that it's a good idea to remove description from the DDL, does that completely remove the need for getUntranslatedLabel() and getUntranslatedDescription() methods? If so, I think it makes sense to scope this issue to just that, and then open a separate issue for using TranslationWrapper to resolve 1, 3, and 4. Don't know whether that separate issue should be critical or not.

Gábor Hojtsy’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
10.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,889 pass(es), 1 fail(s), and 0 exception(s). View
8.66 KB

Resolving most of the fails. Not entirely sure what is reason behind the sole remaining fail.

Status: Needs review » Needs work

The last submitted patch, 99: 2363099-no-description-in-schema-99.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,901 pass(es). View
712 bytes

One of the mocked storage definitions still returned a description that you removed from the expectation.

Gábor Hojtsy’s picture

Indeed. @yched: what do you think?

effulgentsia’s picture

Patch looks good to me.

To reproduce, install in a non-en language, then import translations or manually translate something like "The node ID." to your language. Make sure to clear all caches after that.

Then visit update.php => oops.

Do we want to add a test for that? However, is there a way to make such a test generic: rather than only translating description, translate every translatable string? Do we have the facility to do that in an automated way?

Gábor Hojtsy’s picture

We cannot discover every translatable string. Imagine the translatable strings exposed by views row plugins, displays, field formatters under certain conditions, etc. potx module comes closest to find all strings, but we don't want to depend on that in core (also it does not have a D8 version).

Berdir’s picture

Uhm, we used to have a test, @swentel worked on that, looks like that got lost in one of the rerolls?

That did not actually go to update.php I think but exxplicitly compared the schema? That is a bit bogus now as we don't add the description anymore, but having a test that goes to update.php in a multilingual environment with translations would certainly a be a good thing to have.

For translating everything, we could probably loop over the locale_source table and add a dummy translation for every string there, but that might be a lot, actually, so not sure? Maybe just add a translation for at least one label and description?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

locales_source only get populated as strings are found in foreign language requests, so it would not contain much in a test. Would likely not be useful to test this scenario. There was a LocaleTranslatedSchemaDefinitionTest.php indeed up until #70 and got lost in my reroll after that. Should be possible to merge with some modifications to the new expectations.

Berdir’s picture

Actually, I think locales_source would work better than you think.

All you need to do is visit de/update.php, and your locales_source table will contain more than enough strings, aka every string that was called during that process which likely means everything that could have a impact on update.php working/not working as expected.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,925 pass(es), 1 fail(s), and 1 exception(s). View
12.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,934 pass(es), 1 fail(s), and 1 exception(s). View

Let's go first with @swentel's test only patch. We can modify that to change all descriptions, but changing one should expose it just as well. It is true that if the field description changes, then this will not test what it intends anymore. We can do the wholesale locale string updates to avoid that.

The test only patch is from @swentel earlier. No other changes included.

Let's see how this fairs.

The last submitted patch, 108: 2363099-no-description-in-schema-108-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 108: 2363099-no-description-in-schema-108.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,936 pass(es), 1 fail(s), and 0 exception(s). View
12.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,900 pass(es). View
712 bytes

Config immutability landed since then.

The last submitted patch, 111: 2363099-no-description-in-schema-111-test-only.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
3.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,948 pass(es), 3 fail(s), and 0 exception(s). View
13.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,558 pass(es). View
2.21 KB

And one more test method that instead of setting up one specific translation (and ensuring that via the entity schema system), fills in translatable strings from update.php and then has fake translations for them and ensures no updates via the update.php again. As I said above, invoking the update.php like this will only discover strings that are used at that time. So this method will fail on the user entity object schema not on the node entity schema like the other method. I think keeping both is best.

Note that I am asserting positively for the dsm displayed and negatively for the missing update run link.

The last submitted patch, 113: 2363099-no-description-in-schema-113-test-only.patch, failed testing.

yched’s picture

Fine with the current patch.

Also, yeah agreed with the description of the problems caused by t() in field definitions, and that it would be really good to fix this with some nt() at some point.
We could also reconsider whether $field_definition->setDescription() is for technical data model documentation or for UI help text.
- arguably, the latter could live in the "form display options" section, and be overridable by form mode.

The only drawback I see with the latest appraoch (which I proposed, I know :-/) is that setDescription() is currently where a lot of "data model"-type strings are. If we stop putting those "descriptions" in the schema and state that they are for UI text now, we will start changing them to fix UX and lose the current "data-model" strings, which we'll have to fetch from git history if we re-add them in some different method / property later. Also, well, "setDescription()" is kind of nice for "data model", and that is what TypedData uses it for (e.g FieldItem::propertyDefinitions())

So maybe :
- Stop putting setDescription() strings in the schema for now until we resolve nt() and "which column should receive what - field or property description ?" - patch here does that.
- (probably in a separate issue ;-)) Put the "UI help" texts (we currently have very few of those AFAICT) in another place: new, separate BFD::setHelpText() method, or add them in the display options.
- Then we can start actually adjusting those for UX...

fago’s picture

The new approach works for me. I just fear that we never get to the nt() fix then, but well - it's not critical.

We could also reconsider whether $field_definition->setDescription() is for technical data model documentation or for UI help text.
- arguably, the latter could live in the "form display options" section, and be overridable by form mode.

I'd love to see that. Also for field labels, but that seems to be too large change right now. I.e., have a default UI text in the field definition and have it override-able per display mode. Right now, people set the appropriate form label as field label what's wrong imho; e.g. does not make sense when listing fields in views then.

Berdir’s picture

I was testing this on my project, and it does solve those errors, obviuosly, but update.php still didn't work for me, I continued with the remaining issues in #2336895: Allow entity type and field storage definition objects to be compared for definition equality, which from the issue description, is not fully related, but the @todo in the code was an exact match for what I was looking for. Anyway, could really use some feedback there as well.

Edit: Fixed, the link, thanks

Gábor Hojtsy’s picture

@berdir: wrong link?

Gábor Hojtsy’s picture

@fago, @yched, @Berdir: Any comments on improvements/changes needed in this patch? Maybe I misunderstood, but I found only orthogonal points and no specific improvements to be made here?

Berdir’s picture

I think the patch here is ready.

The only question is what kind of follow-ups, issue summary updates etc. we need.

As a starting point, I think what we should do is open a (major?) follow-up for the t() calls, copying most of the issue summary over to that. Then update the proposed solution and mention the other issue.

Gábor Hojtsy’s picture

Title: Translating field definition descriptions results in different schema definitions, resulting in update.php changes » Using translated field definition descriptions in entity schema results in different schema definitions, resulting in update.php changes
Issue summary: View changes
Berdir’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks, I would suggest to add a sentence like "".

Other than that, RTBC as far as I'm concerned. And we have an OK from @yched and @fago as well, so we should be good to go. I only fixed a single unit test in the new patch, so I think I'm allowed to set it to RTBC myself.

Berdir’s picture

Issue summary: View changes

Ah, wrote that we should add a message, then decided to just do it myself, then switched context and submitted without completing that.

Berdir’s picture

Issue summary: View changes
catch’s picture

Issue tags: +D8 upgrade path
+++ b/core/modules/locale/src/Tests/LocaleTranslatedSchemaDefinitionTest.php
@@ -0,0 +1,91 @@
+   */
+  protected function setUp() {
+    parent::setUp();
+    ConfigurableLanguage::createFromLangcode('fr')->save();
+    $this->config('system.site')->set('langcode', 'fr')->save();
+    drupal_flush_all_caches();
+  }
+

The drupal_flush_all_caches() could use a comment, looks a bit like desperation as it is.

Gábor Hojtsy’s picture

FileSize
13.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,589 pass(es). View
714 bytes

Added a short comment.

I am not sure why @swentel needed an all cache clear, so I looked at only clearing the entity manager cache. EntityManager::getBaseFieldDefinitions() uses a static cache in the class (which can be cleared with EntityManager::clearCachedFieldDefinitions()), but it also uses a cache backend if the local static cache did not retrieve the value. That is cleared on DefaultPluginManager::clearCachedDefinitions() (the parent). Even clearing those two caches does not fully resolve it though, because t() is also cached, and so on. I don't think its relevant for the test exactly what list of caches need to be cleared to reproduce the problem.

swentel’s picture

Yes, I got frustrated finding out which one needed to be cleared - it's a lot, so it would probably end up being a long list. This was the laziest and fastest solution :)

plach’s picture

+++ b/core/modules/locale/src/Tests/LocaleTranslatedSchemaDefinitionTest.php
@@ -31,6 +31,8 @@ protected function setUp() {
+    // Clear all caches so that the base field definition, its cache in
+    // the entity manager, the t() cache, etc. are all cleared.

80 chars :(

Berdir’s picture

FileSize
13.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,882 pass(es). View
852 bytes

That is quite the nitpick ;)

webchick’s picture

(not core maintainer webchick speaking, just regular ol' webchick)

As someone who spent the better part of her D6 life on #164983: Document database schema in hook_schema() [note, we subsequently figured out t()ing these made no sense since they were for DB admins only and they were already reading english table names], I would be a bit sad to see those descriptions go. They're documentation. You can argue how useful said documentation is, but you can also argue that about a lot of our documentation. ;) The point is, it's complete and consistent. Unless I'm missing a cluebat, this change would make entities, an increasingly larger and more important part of our data model, the "odd schema out" when it comes to schema-viewing tools like PHPMyAdmin and so on.

Is it possible by chance to keep the descriptions, but remove whatever code is doing anything with them other than the equivalent of COMMENT='The text value'; in whatever storage mechanism?

Obviously, not going to block a D8 upgrade path critical over it, but wanted to ask the question anyway.

Berdir’s picture

@webchick node schema tables should be considered invisible for the average developer, just like field tables. If you write a db query against them then @chx will open a bug report against your module :)

If you want to know what is in nodes then you should look at the field definitions.

This change will btw allow us to solve a few UX issues because those very technical descriptions bleed into the node form, through generic widgets

yched’s picture

@webchick : preserving those "data model explanations" strings somehow was what my proposal at the end of #115 was trying to achieve. The drawback is that this plan doesn't let us fix UI help strings until we introduce a separate property/method in FieldDefinition to hold DDL strings.

@Berdir has a point in #131 - having "data model explanations" is valuable, but maybe not really in the db schema, since you're not supposed to query the db directly (well, not if you're a contrib module - custom code can always do whatever it sees fit, I know some of my clients most likely won't stop writing direct queries for perf...).

Maybe for now we could just move them as code comments next to each $field['foo'] = new BFD() in [Entity]::baseFieldDefinitions() ? (doesn't have to be done here, could be done in a separate issue)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

If we want to have "data model explanations" then we can add them further down the line in an non API breaking way. What I would expect here is a CR explaining that the schema descriptions are being removed. And some update to documentation somewhere which details what the description field is used for - a very quick scan does not reveal anywhere obvious.

Gábor Hojtsy’s picture

Added change notice draft at https://www.drupal.org/node/2419867.

The issue at #2350013: Descriptions for Promotion Options are too technical shows very well how the descriptions are used (and why using them for DB schema and UI descriptions is bad). I'll look up where to update docs in the code for that.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
FileSize
14.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,886 pass(es). View
600 bytes

Looking at where is the description property defined for base field definitions, it is all the way back to DataDefinition.php:

  /**
   * Sets the human-readable description.
   *
   * @param string $description
   *   The description to set.
   *
   * @return static
   *   The object itself for chaining.
   */
  public function setDescription($description) {
    $this->definition['description'] = $description;
    return $this;

It even sets an array key, so not possible to document on this class or on extending classes. I don't think there is a consistent way we use any DataDefinition children's description. DataDefinitionInterface defines docs for getDescription(). On this low level I think we can add some very generic docs, but not sure how useful will this be. If it says anything new...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 26b99df and pushed to 8.0.x. Thanks!

  • alexpott committed 26b99df on 8.0.x
    Issue #2363099 by Gábor Hojtsy, Berdir, swentel, plach: Using translated...

Status: Fixed » Closed (fixed)

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

joachim’s picture

Am I right in thinking this issue is the reason that database tables such as {node} no longer have database comments for the columns? I think these are really useful for developers, and that therefore it's a DX regression to have lost these.

I appreciate that it wasn't working to have the current field description doing double duty in both the DB comments and the UI. But could we have a follow-up issue to restore the DB comments? How about something like this:

BaseFieldDefinition::setDeveloperComment()->("This is the DB comment")

which SQL storage would use when creating the table. Would that work?