Entity forms will let users edit different language variants of the same piece of data. Actually only one language variant will be editable at any time so we need a reliable way to know which is the form "active" language. Simply relying on the current content language does not always work as it sometimes makes pretty unconfortable the editing workflow, since the user may want to keep her current language and edit different language variants. Hence we need to be able to explicitly set the active form language through a parameter.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue tags: +language-config

This also concerns multilingual configuration.

Related issue: #1498880: Theme language switcher for seven theme.

andypost’s picture

there's a mention about forms in #1448330-25: [META] Discuss internationalization of configuration
Symfony has a great formAPI

Also I think the langcode properties/fields affects mostly on render layer of core

plach’s picture

Issue tags: +Entity forms
Gábor Hojtsy’s picture

@plach: can you provide an updated version of your thinking on this. I got @vasi1186 to work on #1280996: New language_select element type for form API for language selection generalization. Would be good to find common grounds sooner than later.

peximo’s picture

Status: Active » Needs review
FileSize
5.74 KB

Here is a first attempt. The patch introduces a new entity form language type and uses it as a default value for the form language if no one is explictly provided to entity_get_form().

Gábor Hojtsy’s picture

Issue tags: +sprint

Status: Needs review » Needs work

The last submitted patch, et-form_language-1498874-5.patch, failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
7.09 KB

Fixed negotiation callback for new language type and some tests.

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity/entity.module
@@ -12,6 +12,11 @@ use Drupal\entity\EntityStorageException;
+const ENTITY_LANGUAGE_TYPE_FORM = 'entity_language_form';

As discussed with plach we think it makes more sense to have general language type instead of this one. E.g. language_content_edit or something like that, and then use this as default. Let's do so then.

attiks’s picture

Status: Needs work » Needs review
FileSize
7.1 KB
1.75 KB

renamed

plach’s picture

Status: Needs review » Needs work

@fago and I agreed to move the form language type to the language module. Actually @peximo has already a patch for this but apparently he did not post it...

Gábor Hojtsy’s picture

@plach: that sounds good, makes sense.

plach’s picture

However Jose is not convinced that this approach can work for multiple realms, so I think we need to find an alternative not involving a new langauge type. Probably just relying on the content language as a default and relying on Request parameters otherwise.

peximo’s picture

Assigned: Unassigned » peximo

I'll work as per #13 on an alternative version without a new language type.

peximo’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

Removed the new language_form language type and set language_content as form default language.

fago’s picture

Status: Needs review » Needs work

Going without a separate language for now is fine to move on I think. As discussed let's keep the default in $form_state['langcode'] initially so the form controller can deal with it properly.

plach’s picture

Assigned: peximo » Unassigned
Status: Needs work » Needs review
FileSize
3.09 KB
2.17 KB

Addressed #15 since @peximo is working on other stuff, right now.

fago’s picture

Status: Needs review » Needs work

Oh, that looks very clean now :)

+++ b/core/modules/entity/lib/Drupal/entity/EntityFormController.php
@@ -208,11 +208,23 @@ class EntityFormController implements EntityFormControllerInterface {
+      // If the site is not multilingual or no translation for the given form
+      // language is available, fall back to the entity language.
+      $langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
+      $fallback = language_multilingual() ? language_fallback_get_candidates() : array();
+      while (!empty($langcode) && !isset($translations[$langcode])) {

Comment and code doesn't do the same? It uses content language and applies language fallbacks. We should do a final fallback to default entity language though if none of the fallback-languages is in the entity, or?

peximo’s picture

Status: Needs work » Needs review
FileSize
3.25 KB

Comments fixed as per fago's suggestion. Actually we already fall back to the entity language.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Talked with @fago and he's ok with the latest patch.

peximo’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.26 KB

getFormLangcode() should return a langcode not a language object. Fixed.

Status: Needs review » Needs work

The last submitted patch, et-form_language-1498874-21.patch, failed testing.

plach’s picture

We need tests for this, moreover the fix won't work if Language module is disabled. We need to fix the Entity::language() method to always return a valid language object.

Gábor Hojtsy’s picture

@peximo: are you continuing work on this issue? It is a very important part of making Drupal 8 multilingual! Thanks for working on it so far!

peximo’s picture

Assigned: Unassigned » peximo

@gabor: I'll work on this issue tomorrow morning.

peximo’s picture

Status: Needs work » Needs review
FileSize
10.45 KB

Fixed Entity::language() method and added some tests.
@gabor: plach thinks is better to do the test with the entity_test form controller (see #1757232: Improve test coverage for the entity form controller). So we might need to merge the two issues.

Status: Needs review » Needs work

The last submitted patch, et-form_language-1498874-26.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
11.46 KB
3.21 KB

Sorry, we were not taking entity language changes into consideration. Rerolled the patch with that fixed plus a couple of minor adjustments.

plach’s picture

Title: Provide language awareness to forms (introduce the form language concept) » Provide language awareness to entity forms (introduce the form language concept)
Issue tags: -language-config

Retitling and untagging since we are not trying to handle form language in a generic fashion as per the discussion with @Jose.

plach’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, et-form_language-1498874-28.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
11.69 KB
784 bytes

Actually there may be cases where the entity langcode is not set, hence notices are thrown.

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

The last submitted patch, et-form_language-1498874-31.patch, failed testing.

plach’s picture

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

No failures on my box.

#31: et-form_language-1498874-31.patch queued for re-testing.

plach’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

I'm not sure I fully understand why this is necessary (issue summary is a bit sparse), but all of you seem to agree that it is a good idea. I hope you guys asked each other some criticizing questions in the discussions ;)

The code in EntityFormController::getFormLangcode() could be improved for monolingual sites and clarified in terms of code logic, but that is pretty minor and can be done in a later follow-up.

Gábor Hojtsy’s picture

Issue tags: +Avoid commit conflicts

This deals with the rapidly changing entity landscape and is at a definite risk of commit conflicts.

Gábor Hojtsy’s picture

#34: et-form_language-1498874-34.patch queued for re-testing.

catch’s picture

This looks great. just one question:

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -22,7 +22,7 @@ class EntityTranslationTest extends WebTestBase {
-  public static $modules = array('entity_test', 'locale');
+  public static $modules = array('entity_test', 'locale', 'node');

Why not adding multilingual forms to the entity_test module?

Gábor Hojtsy’s picture

@catch: the entity_test module is a pure API test module, it does not have page callbacks or forms at all. I think it would be important at this point to get this moving instead of pushing it back to implement a UI for testing in entity_test module while we already have the necessary stuff to test elsewhere, such as in node module.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK that's fine, but in that case let's add a new test class for the forms, so we don't need to install node module for the other test methods.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.14 KB

Moved it to a separate file with only the setup routines required for this one.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Should be back to RTBC then.

catch’s picture

Status: Reviewed & tested by the community » Needs work

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityTranslationTest.phpundefined
@@ -22,7 +22,7 @@ class EntityTranslationTest extends WebTestBase {

@@ -22,7 +22,7 @@ class EntityTranslationTest extends WebTestBase {
    *
    * @var array
    */
-  public static $modules = array('entity_test', 'locale');
+  public static $modules = array('entity_test', 'locale', 'node');

This change shouldn't be needed then?

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.22 KB

Indeed, we should not have any business touching that file anymore.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

I don't think this really needs change notice so moving straight to fixed, but please re-open/write one if you disagree.

plach’s picture

I think we don't need a change notice here: this ended-up being a far less APIsh change than I originally thought.

Gábor Hojtsy’s picture

Issue tags: -sprint

This should be done for our sprint then :) Thanks all!

Status: Fixed » Closed (fixed)

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

chx’s picture

Issue tags: -Avoid commit conflicts
chx’s picture

Issue summary: View changes

Updated issue summary.