Problem

From a standard profile English clean install

  1. Enable language module
  2. Go to /admin/config/regional/content-language
  3. Check custom language settings for "Content"
  4. Check "Show language selector on create and edit pages" for Article
  5. Save configuration
  6. Uncheck custom language settings for "Content" and don't uncheck "Show language selector on create and edit pages" inside article.
  7. Save configuration

Now you can see that the last uncheck is not saved and "Content" is still checked.

What I expect is that any language customization for "Content" is disabled.

As side note when content translation module is enabled and you have content types marked as translatable when you uncheck "Content" in the custom language settings the content stops being translatable and everything gets unchecked as expected.

Proposed resolution

Fix the form submit so the settings are saved regardless of the language selector settings.

Remaining tasks

 

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch YES Instructions YES
Update the issue summary noting if allowed during the beta YES Instructions YES - Should be okay
Add automated tests NO Instructions YES
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards NO Instructions NO

Issue fork drupal-2575535

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rodrigoaguilera created an issue. See original summary.

rodrigoaguilera’s picture

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-content

Bringing on sprint to investigate.

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: -sprint

Removing from D8MI sprint due to inactivity. Raising to major though.

prasad_gogate’s picture

Issue tags: +Triaged for D8 major current state, +drupalconasia2016

Issue is replicable
Verified on Drupal 8.0.2

Issue is, when checkbox for "content" from /admin/config/regional/content-language is de-selected and saved it does not reflected, this happens when you have sub-section "Show language selector on create and edit pages" is selected.

This happens since a child menu has a value you cannot remove the parent selection.

xjm’s picture

(Updating issue credit for the major triage.)

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu
Status: Active » Needs work

working on this.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
1.33 KB

checked with the existing values of entity and settings.

Status: Needs review » Needs work

The last submitted patch, 8: 2575535-8.patch, failed testing.

SwapS’s picture

Assigned: Unassigned » SwapS

This isn't solving the problem.

If you select any of the option in the content language setting and Submit the form without selecting any other content type specific configuration.
Drupal says 'Settings successfully updated.' but doesn't actually Save's the settings.
We probably should have a validate hook showing , clear message to the end user. (for better user experience )

Two of the other things :
1. Not sure how ,$existing_values = $form_state->getCompleteForm()['entity_types']['#default_value'] is helping.
Looking @ the codebase , it should have been : $form_state->getValues('entity_types') which return's an array . This array contains contains entity type's for which change is initiated.
Please correct me , if i am missing anything here.

2. For below piece of code

  public function submitForm(array &$form, FormStateInterface $form_state) {
    foreach ($form_state->getValue('settings') as $entity_type => $entity_settings) {
      foreach ($entity_settings as $bundle => $bundle_settings) {
        $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type, $bundle);
        $config->setDefaultLangcode($bundle_settings['settings']['language']['langcode'])
          ->setLanguageAlterable($bundle_settings['settings']['language']['language_alterable'])
          ->save();
      }
    }
    drupal_set_message($this->t('Settings successfully updated.'));
  }
 

Here , Inner foreach run's irrespective of whether

checkbox for that Entity type is checked/unchecked by end user

.

I would recommend having , a condition check before updating the config object.
Something like :

  public function submitForm(array &$form, FormStateInterface $form_state) {
    foreach ($form_state->getValue('settings') as $entity_type => $entity_settings) {
      foreach ($entity_settings as $bundle => $bundle_settings) {
        /* To get values of entity for which change is initiated */
        $entity_values = $form_state->getValues('entity_types');
        /* Below condition should help cater point 2. */
        if (in_array($entity_type, $entity_values)) { 
          $config = ContentLanguageSettings::loadByEntityTypeBundle($entity_type, $bundle);
          $config->setDefaultLangcode($bundle_settings['settings']['language']['langcode'])
            ->setLanguageAlterable($bundle_settings['settings']['language']['language_alterable'])
            ->save();
        }
      }
    }
    drupal_set_message($this->t('Settings successfully updated.'));
  }

Cheers,
SwapS

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Priority: Major » Normal

Discussed with @catch, @xjm, @cottser and @effulgentsia. We discussed this and could only see a UI bug with no bad side effects on the site. Yes the behaviour is unexpected but nothing is badly broken. Therefore downgrading to normal.

@prasad_gogate thanks for confirming the bug still exists.

@Gábor Hojtsy, as you promoted this to major in #6, perhaps we're missing something - if we are - let's it promote back to major.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bighappyface’s picture

bighappyface’s picture

bighappyface’s picture

Attached to this comment is a patch with a BrowserTestBase test demonstrating the bad behavior

bighappyface’s picture

Attached to this comment is a patch with the BrowserTestBasetest from #16 as well as a proposed fix for resetting the content language default settings, particularly the language_alterable value. As the description indicates the "Show language selector on create and edit pages" checkbox has to be unchecked for an entity type configuration to be saved. This is due to language_alterable being enabled automatically with JS:

http://cgit.drupalcode.org/drupal/tree/core/modules/content_translation/...

Assuming that the JS behavior is desired this patch will set the relative language_alterable setting to FALSE when the whole entity type is disabled.

The last submitted patch, 16: content_language-default-entity-settings-2575535-16.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bighappyface’s picture

Assigned: SwapS » Unassigned
Issue summary: View changes
Issue tags: -Needs tests, -Triaged for D8 major current state
alexpott’s picture

Issue summary: View changes
Status: Needs review » Needs work
+++ b/core/modules/content_translation/src/Tests/ContentTranslationDisableSettingTest.php
@@ -0,0 +1,59 @@
+    // Login as root user.
+    $this->drupalLogin($this->rootUser);
+    // Define assert session.
+    $assert = $this->assertSession();
+    // Navigate to content language settings page.
+    $this->drupalGet('admin/config/regional/content-language');

Comments in tests are good but this is probably a bit too much. The code almost says the same thing as the comment. Comments are important where it is not obvious. And yep this is subjective.

Also we don't really use the rootUser like this we tend to create users in tests that have the minimum permissions required. User 1 in Drupal land can sometimes have very different code paths to others.

bighappyface’s picture

@alexpott thank you for the feedback! I have updated the test per your guidance.

WidgetsBurritos’s picture

Status: Needs review » Reviewed & tested by the community

+1 LGTM

heykarthikwithu’s picture

+1 RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 4229a76 to 8.4.x and 4a31c0d to 8.3.x. Thanks!

I've backported this to 8.3.x since this change is patch release safe.

  • alexpott committed 4229a76 on 8.4.x
    Issue #2575535 by bighappyface, heykarthikwithu, rodrigoaguilera,...

  • alexpott committed 4a31c0d on 8.3.x
    Issue #2575535 by bighappyface, heykarthikwithu, rodrigoaguilera,...

Status: Fixed » Closed (fixed)

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

thibaudstreignart made their first commit to this issue’s fork.