Problem/Motivation

Contact categories, custom block types, new block configuration, filter formats, user roles, etc. are saved with unknown language. However these config entities contain user facing textual pieces, so this becomes a problem when translating, given that we don't know the source language. The Drupal shipped configuration should be English, the user created configuration should use specific language codes too. Some of the shipped configuration was fixed in #1935000: Some configuration entities are created as in language unknown but there were some left around.

Proposed resolution

- Fix shipped configuration to use langcode: en as per #1935000: Some configuration entities are created as in language unknown.
- Fix creation of new configuration entities to use the site default language (or include a language selector when needed)

Remaining tasks

- Review
- Test

User interface changes

There will be a UI, if we need to change/create one or more of the entity forms.

API changes

No API changes.

Related issues

#1935000: Some configuration entities are created as in language unknown for previous fixes.
#1935022: Add a language selector on views fixed this for Views.
#1945226: Add language selector on menus fixes this for newly created menus (built in menus were fixed in the first patch).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Gábor Hojtsy’s picture

No, contact categories are NOT content entities, they cannot have bundles. #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable is not related.

Gábor Hojtsy’s picture

Title: Fix contact category entity language behaviour » New config entities often save as langcode: und incorrectly
Status: Active » Needs review
Issue tags: +sprint
FileSize
4.29 KB

Retitled for the general problem that affects multiple modules. Here is a patch covering the following:

- custom block type form
- block config form
- contact category form and the one shipped category
- filter format form
- user role form and the two shipped user roles (the standard profile shipped admin role is already ok)

Manual testing of these worked. Not sure what kind of automated testing we should put behind these.

Other possible config entities, that might be affected: breakpoints, breakpoint groups, editors (does not seem to actually have user facing textual content though), entity displays, image styles, picture mapping, shortcut sets. Note that vocabularies have this covered via the language selector. Views got a language selector recently in #1935022: Add a language selector on views and menus are proposed to have a language selector in #1945226: Add language selector on menus. These other config entities do not have language selectors (and might or might not need one). At the minimum we need them to save in a sensible language code, so they are translatable. Contrib could also put language selectors on them by altering the forms if it wants to.

I also figured that custom blocks for some reason want to add a language selector but blocks don't have them by default. This patch might break that. Hopefully we have some test coverage for that which will catch it :D

Let's get reviews and testbot running.

Gábor Hojtsy’s picture

Forgot to add proper langcode to shipped custom block type too.

Gábor Hojtsy’s picture

Updated issue summary for full scope.

Gábor Hojtsy’s picture

Issue summary: View changes

Update summary.

vijaycs85’s picture

vijaycs85’s picture

Looks good to me, sending it for re-testing... RTBC if green...

vijaycs85’s picture

Issue tags: +Needs tests

Just had a word with @Gábor Hojtsy on IRC. This needs tests. adding tag...

tim.plunkett’s picture

Issue tags: -Needs tests
FileSize
3.85 KB
3.4 KB

What about this?

tim.plunkett’s picture

Issue tags: +Needs tests

.

Status: Needs review » Needs work

The last submitted patch, config-entity-1947814-9.patch, failed testing.

Gábor Hojtsy’s picture

@tim: well, that breaks most entities :) They assume there is no pre-existing langcode value field that they would need to unset / override. I'm not sure its best to now go and modify the forms that extend from the base entity. At least I'd say this should be in the config entity form base class to begin with, not the generic entity form base class.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
3.63 KB

@tim.plunkett: seems like there is no form class for config entities in particular. We can either introduce one (inheriting from EntityFormController specifically), do the changes as needed individually, like we did above; or make the base form addition conditional on whether there is already an element for that. In the attached patch I chose this last option, so it keeps the added langcode element as-is if already provided by extended implementations. The parent form controller is invoked last after the elements are added so this should be a possible solution. What do you think?

vijaycs85’s picture

Updating test cases to check the config entities are getting saved in site default language instead of und.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Yay, test updates look great :)

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.phpundefined
@@ -84,6 +84,7 @@ function testTextFormatCrud() {
+    $default_langcode = !$format->isNew() ? $format->langcode : language_default()->langcode;
 
     // Verify filter_format_load().
     $filter_format = filter_format_load($format->format);
@@ -91,6 +92,8 @@ function verifyTextFormat($format) {

@@ -91,6 +92,8 @@ function verifyTextFormat($format) {
     $this->assertEqual($filter_format->name, $format->name, format_string('filter_format_load: Proper title for text format %format.', $t_args));
     $this->assertEqual($filter_format->cache, $format->cache, format_string('filter_format_load: Proper cache indicator for text format %format.', $t_args));
     $this->assertEqual($filter_format->weight, $format->weight, format_string('filter_format_load: Proper weight for text format %format.', $t_args));
+    // Check that the filter was created in site default language.
+    $this->assertEqual($format->langcode, $default_langcode, format_string('filter_format_load: Proper language code for text format %format.', $t_args));

This looks suspicious. The langcode should be the default langcode regardless of whether the format is new or old. Shipped format config files lack langcode: en in them (or have langcode: und specifically)?

vijaycs85’s picture

Status: Needs work » Needs review

#14: 1947814-config-entity-14.patch queued for re-testing.

vijaycs85’s picture

As discussed on IRC, the problem is langcode in Entity class.

--- a/core/lib/Drupal/Core/Entity/Entity.php
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -27,7 +27,7 @@ class Entity implements IteratorAggregate, EntityInterface {
    *
    * @var string
    */
-  public $langcode = LANGUAGE_NOT_SPECIFIED;
+  public $langcode;

and

@@ -68,6 +68,7 @@ class Entity implements IteratorAggregate, EntityInterface {
    */
   public function __construct(array $values, $entity_type) {
     $this->entityType = $entity_type;
+    $this->langcode = language_default()->langcode;

Fixes the issue. However not sure, if it can be changed.

Gábor Hojtsy’s picture

I think that would be an interesting cut through the base of part of the problem. Not sure how it affects existing entity types, but we will see.

Status: Needs review » Needs work

The last submitted patch, 1947814-config-entity-16.patch, failed testing.

vijaycs85’s picture

Updating patch that fixes 4 out of 7 fails in #17

Fixed:
BlockStorageUnitTest.php (line: 109)
ConfigEntityTest.php (line: 44, 58, 100)
SaveTest.php (line: 46)
UserPictureUpgradePathTest.php (line: 47)

Todo:
Basically failing when try to pass langcode by anyway.

1. TermLanguageTest.php (line: 66, 70, 78)
2. TranslationLinkTest.php (line: 58)
3. UserLanguageCreationTest.php (line: 75, 93)

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1947814-config-entity-20.patch, failed testing.

Gábor Hojtsy’s picture

I looked at the taxonomy test. It seems like for some reason the selected langcode does not override the original one added with this patch to init to the site default. I could not track down the reason, the taxonomy form uses the regular http://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/e... code that seems to be capable to override it... Any help in tracking this down would be great! Thanks!

vijaycs85’s picture

Component: contact.module » configuration entity system
Status: Needs work » Needs review
FileSize
1.28 KB
13.25 KB

Updating defualt langcode in ConfigEntityBase instead of Entity.

tim.plunkett’s picture

This should be in the create method of the storage controller.

vijaycs85’s picture

Thanks for that quick review @tim.plunkett. Does it work?

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -sprint, -language-config, -Configuration schema

The last submitted patch, 1947814-config-entity-26.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +sprint, +language-config, +Configuration schema

#26: 1947814-config-entity-26.patch queued for re-testing.

vijaycs85’s picture

For 2 fails in #24, just need fix on content entity test case asset changes made on #20.

Still not sure about 5 fails in #26. Sent it for re-testing...

Status: Needs review » Needs work

The last submitted patch, 1947814-config-entity-26.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
12.15 KB

Fixing all 4 test cases.

Status: Needs review » Needs work

The last submitted patch, 1947814-config-entity-31.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
621 bytes
12.75 KB

Updating test fail in ConfigImportUITest.php...

andypost’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -289,6 +289,9 @@ public function create(array $values) {
+    // Make sure entity created with site default language.
+    $entity->langcode = language_default()->langcode;

But how to pass special language when needed to init with 'und'?

vijaycs85’s picture

@andypost, as far is I discussed with @Gábor Hojtsy, we always define config entity with site default language.

tim.plunkett’s picture

Assigned: Unassigned » Gábor Hojtsy

I've not heard that, so I'm not going to RTBC. If @Gábor Hojtsy confirms, this is good to go from my perspective.

YesCT’s picture

I took a look, and the changes make sense while reading the code and it looks good standards wise.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -289,6 +289,9 @@ public function create(array $values) {
 
+    // Make sure entity created with site default language.
+    $entity->langcode = language_default()->langcode;
+

I agree we need to answer the question: "How is it possible after this patch to create entities in other languages". Looks like this will overwrite the langcode value provided in create($values) with the site default language? What about only doing this fallback is there was no langcode provided in $values? That would be possible before the new $class code in create() even, like so:

public function create(array $values) {
  $class = $this->entityInfo['class'];

  // Set default language to site default if not provided.
  $values += array('langcode' => language_default()->langcode);

  $entity = new $class($values, $this->entityType);

// ...

Instead of overwriting whatever got into the object in the first place. What do you think?

tim.plunkett’s picture

Yes, that's how we should do it if we care about config entities having langcodes. Glad I asked :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
12.87 KB

Updating changes mentioned in #38

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, looks great now. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e97a786 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, superb, thanks! I've added a change notice for this for reference in the future for ongoing conversions. http://drupal.org/node/1965388

YesCT’s picture

@tim.plunkett pointed out to me that in #1945226-20: Add language selector on menus

I had to move the call to parent::form to the end.

Because this patch checked first if langcode was added, and if not it added it.
Then in the menu form function, it tried to add it in a different place, but it was already added.

diff --git a/core/modules/menu/lib/Drupal/menu/MenuFormController.php b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
index e229451..3ab6c83 100644
--- a/core/modules/menu/lib/Drupal/menu/MenuFormController.php
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -19,7 +19,6 @@ class MenuFormController extends EntityFormController {
    * Overrides Drupal\Core\Entity\EntityFormController::form().
    */
   public function form(array $form, array &$form_state, EntityInterface $menu) {
-    $form = parent::form($form, $form_state, $menu);
     $system_menus = menu_list_system_menus();
     $form_state['menu'] = &$menu;
 
@@ -88,7 +87,7 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
       );
     }
 
-    return $form;
+    return parent::form($form, $form_state, $menu);
   }

I'm making a note here in case there are other places beside menus where we want to move the call to parent::form

Gábor Hojtsy’s picture

Yeah, we found at other places the parent form call was at the end, which is the expected way to write the form :) We also added automated testing to the above items, so they should be covered/verified. Menus were not touched in this issue specifically since #1945226: Add language selector on menus exists.

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

Anonymous’s picture

Issue summary: View changes

Fix ref in UI text section.