Problem/Motivation

Currently we allow to configure content language settings only for translatable entity types, but this is an unnecessary limitation, since language assignment settings apply also to language-aware but non-translatable entity types. Follow-up of #2230637-88: Create a Language field widget and the related formatter.

Proposed resolution

Display all language-aware entity types on the content language settings page. Remove workarounds from aggregator.

Remaining tasks

Review. Commit.

User interface changes

When language module (and for this screenshot aggregator) is enabled:

When content translation is also enabled:

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because non-translatable types cannot be configured for language defaults on the common configuration screen. Even for their own one-off configuration screens need workaround to work see http://cgit.drupalcode.org/drupal/tree/core/modules/aggregator/src/FeedF....
Issue priority Major because this affects all non-translatable language-aware entity types in core and contrib.
Prioritized changes Prioritized because this reduces fragility by allowing to remove one-off workarounds and improves user experience by removing a confusing and unnecessary differentiation.
Disruption No disruption foreseen.
CommentFileSizeAuthor
#43 2397729-language-aware-content-settings.interdiff.38-43.txt787 bytespenyaskito
#43 2397729-language-aware-content-settings-43.patch5.23 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,105 pass(es). View
#41 Content language | s2b43a3e92a5d91d.s3.simplytest.me 2015-02-06 09-03-20.png132.05 KBGábor Hojtsy
#41 Content language | s2b43a3e92a5d91d.s3.simplytest.me 2015-02-06 09-01-32.png122.16 KBGábor Hojtsy
#38 2397729-language-aware-content-settings.interdiff.36-37.txt844 bytespenyaskito
#38 2397729-language-aware-content-settings-37.patch5.23 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,115 pass(es). View
#36 2397729-language-aware-content-settings.interdiff.32-36.txt1.01 KBpenyaskito
#36 2397729-language-aware-content-settings-36.patch4.4 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
#32 2397729-language-aware-content-settings-32.patch3.39 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,908 pass(es), 9 fail(s), and 2 exception(s). View
#32 interdiff.txt3.3 KBGábor Hojtsy
#25 Content language Site-Install.png113.43 KBpenyaskito
#24 2397729-language-aware-content-settings.interdiff.18-24.txt1.05 KBpenyaskito
#24 2397729-language-aware-content-settings-24.patch3.31 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,912 pass(es). View
#21 Content language | s3ee58b0b66f998b.s3.simplytest.me 2015-01-19 15-30-22.png151.05 KBGábor Hojtsy
#20 drupal8-d8mi-2397729-entity-langcode-agg.png66.61 KBKristen Pol
#20 drupal8-d8mi-2397729-entity-langcode-settings.png101.69 KBKristen Pol
#18 2397729-language-aware-content-settings.interdiff.3-18.txt968 bytespenyaskito
#18 2397729-language-aware-content-settings-18.patch3.14 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,087 pass(es). View
#3 language-aware_content_settings-2397729-3.patch3.26 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,303 pass(es). View
#2 language-aware_content_settings-2397729-2.patch0 bytesplach
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

plach’s picture

Issue summary: View changes
Status: Active » Postponed

Better waiting for #2143729: Entity definitions miss a language entity key to be committed, as it will provide a way to code a cleaner solution.

plach’s picture

plach’s picture

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

Now for reals :)

plach’s picture

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -68,7 +69,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      if (!$entity_type instanceof ContentEntityTypeInterface || !isset($this->entityManager->getBaseFieldDefinitions($entity_type_id)['langcode'])) {

We will need to check that the langcode entity key is defined. Adding an API function/method to do that would be nice.

plach’s picture

Category: Bug report » Task
Issue summary: View changes

Fixed beta-phase evaluation.

plach’s picture

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

I'd much rather display N/A in place of the checkbox instead of messing with where the columns are. We worked so hard on lining up the columns so you can take a quick look and see what is going on. Not including the translatable column for some while including for others seems to be a problem to me.

plach’s picture

Well, that was a very raw draft: we could also add some CSS styling to align columns again, actually this is what I'd do if I didn't read #7, but no strong preference really.

Gábor Hojtsy’s picture

One thing I would clearly avoid is using disabled checkboxes, that would be a disaster. Disabled checkboxes are in most cases impossible to tell apart from enabled checkboxes on many systems.

plach’s picture

Agreed :)

alexpott’s picture

Status: Postponed » Needs work

I agree with the beta evaluation - reducing fragility and making things less hacky for contrib should be permitted if the disruption is minimal.

plach’s picture

@alexpott:

Thanks for the feedback! I'd leave this postponed on #2143729: Entity definitions miss a language entity key, so we can rely on the langcode entity key to determine whether an entity type is language-aware.

plach’s picture

Status: Needs work » Postponed

Forgot to change status...

penyaskito’s picture

Status: Postponed » Active
plach’s picture

Status: Active » Needs review

We have a patch, so setting NR for the bot, but it's NW actually.

plach’s picture

Status: Needs review » Needs work

Green, nice :)

Gábor Hojtsy’s picture

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -68,7 +69,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      if (!$entity_type->isTranslatable()) {
+      if (!$entity_type instanceof ContentEntityTypeInterface || !isset($this->entityManager->getBaseFieldDefinitions($entity_type_id)['langcode'])) {
         continue;

So looks like this will need to be updated for the langcode key. @plach / @penyaskito? :)

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
3.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,087 pass(es). View
968 bytes

Like this?

Kristen Pol’s picture

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -62,7 +63,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      if (!$entity_type->isTranslatable()) {
+      $langcodeKey = $entity_type->getKey('langcode');
+      if (!$entity_type instanceof ContentEntityTypeInterface || empty($langcodeKey)) {
         continue;
       }

Should there be parantheses? e.g.

+      if (!($entity_type instanceof ContentEntityTypeInterface) || empty($langcodeKey)) {
Kristen Pol’s picture

I do see the Aggregator and File entities in the list.

I don't know how to test the File (I didn't see an edit form for File) but I see language selector on the Aggregator Feed:

Gábor Hojtsy’s picture

Status: Needs review » Needs work
FileSize
151.05 KB

As discussed before this does away with all our work to line up things... See #7. I think we should display 'N/A' for translatable when it is not applicable or something similar. Code-wise it looks good.

plach’s picture

Or using a multiple colspan.

Gábor Hojtsy’s picture

@penyaskito: can you bring this forward?

penyaskito’s picture

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

Added N/A.

penyaskito’s picture

Gábor Hojtsy’s picture

Yay, looks great, thanks @penyaskito.

We use the same N/A in the language summary table for English if English is not possible to translate. So this is very consistent. Needs tests to ensure that eg. File appears on admin/config/regional/content-language both before content translation module and after and then it does not have a checkbox.

plach’s picture

Yep, looks great, @penyaskito++

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Needs works for tests only then :)

plach’s picture

We should alse revert the changes introduced in the aggregator feed (or item?) form in #2230637: Create a Language field widget and the related formatter.

Gábor Hojtsy’s picture

I digged that up to make it easy. Looks like the whole method can go away now.

diff --git a/core/modules/aggregator/src/FeedForm.php b/core/modules/aggregator/src/FeedForm.php
index ea6714e..d84c11e 100644
--- a/core/modules/aggregator/src/FeedForm.php
+++ b/core/modules/aggregator/src/FeedForm.php
@@ -7,13 +7,9 @@
 
 namespace Drupal\aggregator;
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\Entity\ContentEntityForm;
-use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Form\FormStateInterface;
-use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Url;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
  * Form controller for the aggregator feed edit forms.
@@ -24,21 +20,13 @@ class FeedForm extends ContentEntityForm {
    * {@inheritdoc}
    */
   public function form(array $form, FormStateInterface $form_state) {
-    $feed = $this->entity;
-
-    // @todo: convert to a language selection widget defined in the base field.
-    //   Blocked on https://drupal.org/node/2226493 which adds a generic
-    //   language widget.
-    // Language module may expose or hide this element, see language_form_alter().
-    $form['langcode'] = array(
-      '#title' => $this->t('Language'),
-      '#type' => 'language_select',
-      '#default_value' => $feed->language()->getId(),
-      '#languages' => LanguageInterface::STATE_ALL,
-      '#weight' => -4,
-    );
+    $form = parent::form($form, $form_state);
+    // @todo Allow non translatable entity types having language support to be
+    // configured in the content language setting.
 
-    return parent::form($form, $form_state, $feed);
+    // Ensure the language widget is displayed.
+    $form['langcode']['#access'] = TRUE;
+    return $form;
   }
 
   /**
andypost’s picture

  1. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -87,11 +87,16 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +    $form['settings'][$entity_type_id]['#translatable'] = $entity_type_translatable;
    

    is this used?

  2. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -139,13 +144,12 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +  $element = $variables['element'];
       if (!\Drupal::currentUser()->hasPermission('administer content translation')) {
         return;
    ...
    -  $element = $variables['element'];
       $build = &$variables['build'];
    

    moved for no reason

  3. +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -62,7 +63,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -      if (!$entity_type->isTranslatable()) {
    +      $langcodeKey = $entity_type->getKey('langcode');
    +      if (!$entity_type instanceof ContentEntityTypeInterface || empty($langcodeKey)) {
    

    use $entity->haskey()

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
3.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,908 pass(es), 9 fail(s), and 2 exception(s). View

Resolving #29/30 and #31. Still needs tests, feel free to work on that. ContentTranslationSettingsTest can be extended to test that the file entity appears for example. and has no checkbox for translation.

Status: Needs review » Needs work

The last submitted patch, 32: 2397729-language-aware-content-settings-32.patch, failed testing.

Gábor Hojtsy’s picture

So FeedLanguageTest fails on no langcode field where expected...

Gábor Hojtsy’s picture

@penyaskito: wanna keep this going? :)

penyaskito’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
1.01 KB

Back to green.

Status: Needs review » Needs work

The last submitted patch, 36: 2397729-language-aware-content-settings-36.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,115 pass(es). View
844 bytes

This should be enough, easier than I thought.

Status: Needs review » Needs work

The last submitted patch, 38: 2397729-language-aware-content-settings-37.patch, failed testing.

Gábor Hojtsy’s picture

Title: Allow to configure content language settings for all language-aware entity types » Content language settings cannot be configured for non-translatable types on the one page configuration form
Category: Task » Bug report
Issue summary: View changes
FileSize
122.16 KB
132.05 KB

Retirled as a bug. Adding updated screenshots to summary, and updated summary in general.

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Looks good, except this tiny thing:

+++ b/core/modules/aggregator/src/Tests/FeedLanguageTest.php
@@ -49,6 +49,15 @@ protected function setUp() {
+    // Enable translation for feeds.
+    $edit['entity_types[aggregator_feed]'] = TRUE;
+    $edit['settings[aggregator_feed][aggregator_feed][settings][language][language_alterable]'] = TRUE;

This is precisely NOT translation but language selection, right? :)

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,105 pass(es). View
787 bytes

Right, the comment was wrong.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice patch, nice test, nice screenshots. Committed a85e59d and pushed to 8.0.x. Thanks!

Thanks for including the beta evaluation in the issue summary.

  • alexpott committed a85e59d on 8.0.x
    Issue #2397729 by penyaskito, Gábor Hojtsy, plach, Kristen Pol: Content...
plach’s picture

Woot

Gábor Hojtsy’s picture

Issue tags: -sprint

Amazing, thanks all for your great work.

Status: Fixed » Closed (fixed)

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