Problem/Motivation

There is an option to enable "administration pages language" detection (admin/config/regional/language/detection) (text label pending #1974044: "Account administration pages" detection method confusing).

d8mi-admin-pages-language-detection.png

If this setting is disabled, it is confusing to have the "Administration pages language" setting:

d8mi-admin-pages-language.png

on the user account page (user/[uid]/edit) because it wouldn't do anything.

Proposed resolution

If "administration pages language" detection is disabled or if there is only one language (no choice to make), hide the "Administration pages language" setting on the user account page.
Only show the setting if "administration pages language" detection is enabled and there is more than one language

Remaining tasks

none.

User interface changes

Changes UI on page: user/[uid]/edit

lots of screenshots in #96.

API changes

None

Steps to reproduce

  1. Install Drupal 8
  2. Add more than one language
  3. Go to: admin/config/regional/language/detection
  4. Notice that the default selection for the account administration pages setting is unchecked
  5. Go to your profile edit page... e.g. user/[uid]/edit... which is user/1/edit for super user
  6. Notice that the "Administration pages language" option is shown
  7. Repeat for when the account administration pages setting is checked (still shows which is good in this case)

Related issues

Files: 
CommentFileSizeAuthor
#121 interdiff.117-121.txt1.08 KBpenyaskito
#121 1998648-hide-admin-language-121.patch9.24 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,360 pass(es).
[ View ]
#118 1998648-hide-admin-language-117.patch9.23 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 59,164 pass(es).
[ View ]
#118 interdiff-117-118.txt580 bytesYesCT
#117 1998648-hide-admin-language-116.patch9.28 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]
#117 interdiff-114-116.txt1.48 KBYesCT
#114 1998648-hide-admin-language-111-114.interdiff.txt3.03 KBgrisendo
#114 1998648-hide-admin-language-114.only-test.must-fail.patch5.62 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,633 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#114 1998648-hide-admin-language-114.patch9.67 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,031 pass(es).
[ View ]
#111 1998648-hide-admin-language-109-111.interdiff.txt823 bytesgrisendo
#111 1998648-hide-admin-language-111.only-test.must-fail.patch5.43 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 59,031 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#111 1998648-hide-admin-language-111.patch9.49 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 58,908 pass(es).
[ View ]
#109 1998648-hide-admin-language-103-109.interdiff.txt678 bytesgrisendo
#109 1998648-hide-admin-language-109.only-test.must-fail.patch5.43 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,822 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#109 1998648-hide-admin-language-109.patch9.48 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,919 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#103 1998648-hide-admin-language-101-103.interdiff.txt926 bytesgrisendo
#103 1998648-hide-admin-language-103.only-test.must-fail.patch5.43 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,745 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#103 1998648-hide-admin-language-103.patch9.48 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,806 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#101 1998648-hide-admin-language-98-101.interdiff.txt2.37 KBgrisendo
#101 1998648-hide-admin-language-101.only-test.must-fail.patch5.43 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#101 1998648-hide-admin-language-101.patch9.49 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#99 1998648-hide-admin-language-95-98.interdiff.txt5.34 KBgrisendo
#99 1998648-hide-admin-language-98.only-test.must-fail.patch5.47 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,765 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#99 1998648-hide-admin-language-98.patch9.52 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#88 1998648-hide-admin-language-85-88.interdiff.txt3.46 KBgrisendo
#88 1998648-hide-admin-language-88.only-test.must-fail.patch4.29 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,472 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#88 1998648-hide-admin-language-88.patch8.35 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 58,768 pass(es).
[ View ]
#96 nopatch-2lang-unchecked.png113.86 KBgrisendo
#96 patch-1lang-unchecked.png114.8 KBgrisendo
#96 patch-2lang-unchecked.png109.29 KBgrisendo
#96 patch-1lang-checked.png114.53 KBgrisendo
#96 patch-2lang-checked.png109.64 KBgrisendo
#95 1998648-hide-admin-language-92-95.interdiff.txt1.62 KBgrisendo
#95 1998648-hide-admin-language-95.only-test.must-fail.patch4.25 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,787 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#95 1998648-hide-admin-language-95.patch8.31 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]
#92 1998648-hide-admin-language-90-92.interdiff.txt2.56 KBgrisendo
#92 1998648-hide-admin-language-92.only-test.must-fail.patch4.18 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,911 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#92 1998648-hide-admin-language-92.patch8.24 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 58,850 pass(es).
[ View ]
#90 1998648-hide-admin-language-88-89.interdiff.txt472 bytesgrisendo
#90 1998648-hide-admin-language-89.only-test.must-fail.patch4.29 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,568 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#90 1998648-hide-admin-language-89.patch8.35 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]
#85 1998648-hide-admin-language-79-85.interdiff.txt4.23 KBgrisendo
#85 1998648-hide-admin-language-85.only-test.must-fail.patch4.23 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,901 pass(es), 2 fail(s), and 26 exception(s).
[ View ]
#85 1998648-hide-admin-language-85.patch8.28 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,492 pass(es), 1 fail(s), and 14 exception(s).
[ View ]
#79 1998648-hide-admin-language-79.patch4.06 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]
#79 interdiff.77b.79.txt2.92 KBgrisendo
#77 1998648-hide-admin-language-77.patch4.26 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 57,698 pass(es), 302 fail(s), and 133 exception(s).
[ View ]
#77 1998648-hide-admin-language-77b.patch4.22 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 57,629 pass(es), 311 fail(s), and 532 exception(s).
[ View ]
#77 interdiff.77.77b.txt565 bytesgrisendo
#75 1998648-hide-admin-language-75.patch1.65 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 58,643 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#73 1998648-hide-admin-language-72.patch2.56 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 57,709 pass(es), 128 fail(s), and 11 exception(s).
[ View ]
#71 1998648-hide-admin-language-66.patch3.96 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998648-hide-admin-language-66_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#67 1998648-hide-admin-language-66.patch3.96 KBjair
FAILED: [[SimpleTest]]: [MySQL] 57,228 pass(es), 123 fail(s), and 11 exception(s).
[ View ]
#67 interdiff-58-66.txt2.49 KBjair
#66 Screen Shot 2013-08-16 at 2.35.16 PM.png64.55 KBjair
#58 1998648-hide-admin-language-58.patch4.24 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 56,914 pass(es), 294 fail(s), and 134 exception(s).
[ View ]
#53 1998648-hide-admin-language.interdiff-43-53.txt2.35 KBpenyaskito
#53 1998648-hide-admin-language-53.only-test.must-fail.patch4.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 57,256 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#53 1998648-hide-admin-language-53.patch8.57 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 57,546 pass(es).
[ View ]
#43 interdiff.39-43.txt3.02 KBpenyaskito
#43 drupal8.hide_admin_lang_if_not_user.only-test-must-fail.1998648-43.patch4.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 56,662 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#43 drupal8.hide_admin_lang_if_not_user.1998648-43.patch8.81 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 57,294 pass(es).
[ View ]
#39 interdiff.di-ftw.yesct-ftw.txt2.42 KBpenyaskito
#39 drupal8.hide_admin_lang_if_not_user.only-test.1998648-39.patch4.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#39 drupal8.hide_admin_lang_if_not_user.1998648-39.patch7.88 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,654 pass(es).
[ View ]
#36 interdiff.txt3.17 KBpenyaskito
#36 drupal8.hide_admin_lang_if_not_user.only-test.1998648-36.patch4.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 drupal8.hide_admin_lang_if_not_user.1998648-36.patch6.38 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,640 pass(es).
[ View ]
#33 interdiff.txt7.6 KBpenyaskito
#33 drupal8.hide_admin_lang_if_not_user.only-test.1998648-33.patch4.37 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,109 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#33 drupal8.hide_admin_lang_if_not_user.1998648-33.patch6.53 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,467 pass(es).
[ View ]
#23 drupal8.hide_admin_lang_if_not_user.1998648-23-FAIL.patch3.97 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] 55,450 pass(es), 1 fail(s), and 16 exception(s).
[ View ]
#23 drupal8.hide_admin_lang_if_not_user.1998648-23-PASS.patch6.07 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] 55,067 pass(es), 1 fail(s), and 16 exception(s).
[ View ]
#15 drupal.1998648.hide_admin_lang_if_not_used.14.patch2.1 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,650 pass(es).
[ View ]
#15 interdiff-13-14.txt1.13 KBYesCT
#14 drupal-1945226-Add-language-selector-on-menus-50.patch3.51 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,598 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 interdiff-49-50.txt827 bytesYesCT
#13 drupal.1998648.hide_admin_lang_if_not_used.13.patch1.8 KBKristen Pol
PASSED: [[SimpleTest]]: [MySQL] 57,549 pass(es).
[ View ]
#7 drupal.1998648.hide_admin_lang_if_not_used.7.patch1.82 KBKristen Pol
PASSED: [[SimpleTest]]: [MySQL] 55,876 pass(es).
[ View ]
#2 drupal.1998648.hide_admin_lang_if_not_used.2.patch1.31 KBKristen Pol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.1998648.hide_admin_lang_if_not_used.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
d8mi-admin-pages-language-detection.png46.79 KBKristen Pol
d8mi-admin-pages-language.png31.58 KBKristen Pol

Comments

StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.1998648.hide_admin_lang_if_not_used.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a simple patch. Note that this uses variable_get because the data currently lives in variable. Dependent on:

#1862202: Objectify the language system

#1827038: Remove stale references to language_content_type variable

to convert variable_get.

Issue tags:+language-base

Status:Active» Needs review
Issue tags:+Novice

Status:Needs review» Needs work

The last submitted patch, drupal.1998648.hide_admin_lang_if_not_used.2.patch, failed testing.

Seems like the patch did not apply. Can you reroll that, so it applies?

Also, 'language_negotiation_language_interface' is a combination key of the 'language_negotiation' prefix and '_language_interface' for the language negotiation type. The admin language option may be enabled in other types. Although this is an edge case. I think the option should be dependent on checking ALL types and if any of them have this setting on. You can get a list of all language types from http://api.drupal.org/api/drupal/core%21includes%21language.inc/function...

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,876 pass(es).
[ View ]

Here's a reworked patch that checks all language types... and hopefully passes tests! (note that I was using "git diff -b" and that was producing the testbot errors before)

Status:Needs review» Reviewed & tested by the community

Nice, tested and works well.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -197,13 +197,24 @@ public function form(array $form, array &$form_state) {
+      if ($show_admin_language) {
+        $form['language']['preferred_admin_langcode'] = array(
+          '#type' => 'language_select',
+          '#title' => t('Administration pages language'),
+          '#languages' => LANGUAGE_CONFIGURABLE,
+          '#default_value' => $user_preferred_admin_langcode,
+          '#access' => user_access('access administration pages', $account),
+        );
+      }

Please change this to...

        $form['language']['preferred_admin_langcode'] = array(
          '#type' => 'language_select',
          '#title' => t('Administration pages language'),
          '#languages' => LANGUAGE_CONFIGURABLE,
          '#default_value' => $user_preferred_admin_langcode,
          '#access' => $show_admin_language && user_access('access administration pages', $account),
        );

Status:Reviewed & tested by the community» Needs work

Also LANGUAGE_CONFIGURABLE changed too. #1620010: Move LANGUAGE constants to the Language class.

Thanks for the feedback. I will try to get to these this week.

Status:Needs work» Needs review
StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,549 pass(es).
[ View ]

Here's an updated patch.

StatusFileSize
new827 bytes
new3.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,598 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

[ignore this comment. uploaded wrong patch]

StatusFileSize
new1.13 KB
new2.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,650 pass(es).
[ View ]

right patch this time.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -199,13 +199,22 @@ public function form(array $form, array &$form_state) {
+    if (module_exists('language')) {

module exists is depreciated.
(#2007684: Update deprecated doc for module_exists() and call method inside)

Also, added a comment.

Status:Needs review» Reviewed & tested by the community

Tested and looks great and checked the code and it looks good, too. Thanks Cathy!

I agree rtbc, concerns from #9 and #11 have been addressed.

Status:Reviewed & tested by the community» Needs review

I think we should write some tests for this.

@YesCT - let me know if you can either write the tests or give me a quick tutorial on how to write tests :)

Thanks @YesCT! I'll see if I can read those tomorrow.

I skimmed through them and it doesn't seem too scary ;) I'll see if I can take a stab at it tonight.

StatusFileSize
new6.07 KB
FAILED: [[SimpleTest]]: [MySQL] 55,067 pass(es), 1 fail(s), and 16 exception(s).
[ View ]
new3.97 KB
FAILED: [[SimpleTest]]: [MySQL] 55,450 pass(es), 1 fail(s), and 16 exception(s).
[ View ]

I've written some tests and have them in their own patch (named *FAIL) and included them with the fix in another patch (named *PASS).

Status:Needs review» Needs work

The last submitted patch, drupal8.hide_admin_lang_if_not_user.1998648-23-PASS.patch, failed testing.

First off, congrats on your first test! However, I see two problems. Two strange things. First, all of the users created have 'access administration pages' permission, so none of them are non-admins? Second, you never actually enable the admin language negotiation method in the test. I expected the test to focus on first checking that whatever the permission level of a user, the admin language option is not present on user edit, then enable the method, then check if admin level users have it on their account settings, but regular users still don't.

Ah, yes! Forgot all about the "admin language negotiation method" which is sort of the point :P

The "non_admin_user" shouldn't have that permission... that was a mistake. But, in IRC, it sounded like it would be better to just use one user and have them change permissions for the different tests? Yes?

I'll try to get to this tomorrow night. Thanks!

I think have a non admin user and an admin user. No need to change permissions. Also not needed to have three users.

I'm still planning on updating the tests... been sick and threw my back out yesterday :/

Assigned:Kristen Pol» Unassigned

I'm unassigning for the moment in case someone wants to update the tests during the sprint. Otherwise, I will try to look at in the next couple days.

Issue tags:+sprint

Issue tags:-Novice

Definitely not novice anymore.

Assigned:Unassigned» penyaskito

Assigning to me.

Status:Needs work» Needs review
StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,467 pass(es).
[ View ]
new4.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,109 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new7.6 KB

For me doesn't make sense neither to show the admin language setting if there is only one language on your site, so I changed that too. Let me know if is the way to go.

For the record: first patch should be red, second one green if it works as expected.

Status:Needs review» Needs work

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -7,6 +7,7 @@
+use Drupal;
@@ -199,13 +200,26 @@ public function form(array $form, array &$form_state) {
+    if (Drupal::moduleHandler()->moduleExists('language')) {

I think we do \Drupal directly actually, not use Drupal(?!)

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.phpundefined
@@ -0,0 +1,114 @@
+/**
+ * @file
+ * Definition of Drupal\user\Tests\UserAdminLanguageTest.
+ */

This should currently be Contains and \Drupal

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.phpundefined
@@ -0,0 +1,114 @@
+/**
+ * Functional tests for a user's ability to change their admininstration pages language.
+ */

Can we fit this into 80 chars?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.phpundefined
@@ -0,0 +1,114 @@
+  /**
+   * Tests that the admin language configuration is not available if the site
+   * is not multilingual.
+   */
...
+  /**
+   * Tests that the admin language configuration is not available if the admin
+   * language negotiation is not enabled.
+   */
...
+  /**
+   * Tests that the admin language configuration is available for admins in
+   * multilingual sites, but not for users without the right permissions.
+   */

Should fit on one line if at all possible :)

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.phpundefined
@@ -0,0 +1,114 @@
+   * Helper meethod for setting the language user admin negotiation.
...
+   * Helper meethod for adding a custom language.

meethod => method

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new6.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,640 pass(es).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.17 KB

+  /**
+   * Tests that the admin language configuration is not available if the admin
+   * language negotiation is not enabled.
+   */

Should fit on one line if at all possible :)

My best is:

  /**
   * Tests that admin language is configurable only if admin language negotiation
   * is enabled.
   */

Other issues have been fixed.

Status:Needs review» Needs work

@YesCT echoes that using Drupal direct is not good, the module handler needs to be injected.

@YesCT asked to add DI to the form, so needs work.

Status:Needs work» Needs review
StatusFileSize
new7.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,654 pass(es).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,650 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.42 KB

w00t! I know a little more about d8 than yesterday, thanks @YesCT :)
Added dependency injection to the form.

Status:Needs review» Reviewed & tested by the community

Looks good functionally. Great changes, especially the DI improvement.

Thanks penyaskito! And thanks Gábor for reviewing!

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -2,21 +2,53 @@
+  public function __construct($operation, ModuleHandlerInterface $module_handler) {
+    parent::__construct($operation);
+
+    $this->moduleHandler = $module_handler;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info, $operation = NULL) {
+    return new static(
+      $operation,
+      $container->get('module_handler')
+    );
+  }

We can drop the operation argument here... it's not provided by the calls to createInstance... see \Drupal\Core\Entity\EntityControllerInterface and there is not need to call the parent constructor either...

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -199,13 +231,26 @@ public function form(array $form, array &$form_state) {
+      if (language_multilingual()) {

This function just does return variable_get('language_count', 1) > 1; so that when we move language to CMI we don't have an unnecessary procedural call here...

Status:Needs work» Needs review
StatusFileSize
new8.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,294 pass(es).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 56,662 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.02 KB

@alexpott was right, thanks for review :D

The interdiff looks all logical to me on review.

Looks good to me too... can we put back to RTBC or need to do something more testing?

Status:Needs review» Reviewed & tested by the community

Now that the tests proved it works too, not just looking good, yay!

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -199,13 +240,26 @@ public function form(array $form, array &$form_state) {
+    // Only show the account setting for Administration pages language to users
+    // if one of the detection and selection methods uses it.
+    if ($this->moduleHandler->moduleExists('language')) {
+      $show_admin_language = FALSE;
+      if ($this->languageManager->isMultilingual() ) {
+        foreach (language_types_info() as $type_key => $language_type) {
+          $negotiation_settings = variable_get("language_negotiation_{$type_key}", array());
+          if ($show_admin_language = isset($negotiation_settings[LANGUAGE_NEGOTIATION_USER_ADMIN])) {
+            break;
+          }
+        }
+      }
+      $form['language']['preferred_admin_langcode'] = array(
+        '#type' => 'language_select',
+        '#title' => t('Administration pages language'),
+        '#languages' => Language::STATE_CONFIGURABLE,
+        '#default_value' => $user_preferred_admin_langcode,
+        '#access' => $show_admin_language && user_access('access administration pages', $account),
+      );
+    }

It's best not have form elements that only appear under certain conditions... it makes form_alters much harder than they need to be... let's change the code to be...

<code>
+++ b/core/modules/user/lib/Drupal/user/AccountFormController.phpundefined
@@ -199,13 +240,26 @@ public function form(array $form, array &$form_state) {
    // Only show the account setting for Administration pages language to users
    // if one of the detection and selection methods uses it.
    $show_admin_language = FALSE;
    if ($this->moduleHandler->moduleExists('language')) {
      if ($this->languageManager->isMultilingual() ) {
        foreach (language_types_info() as $type_key => $language_type) {
          $negotiation_settings = variable_get("language_negotiation_{$type_key}", array());
          if ($show_admin_language = isset($negotiation_settings[LANGUAGE_NEGOTIATION_USER_ADMIN])) {
            break;
          }
        }
      }
    }
    $form['language']['preferred_admin_langcode'] = array(
      '#type' => 'language_select',
      '#title' => t('Administration pages language'),
      '#languages' => Language::STATE_CONFIGURABLE,
      '#default_value' => $user_preferred_admin_langcode,
      '#access' => $show_admin_language && user_access('access administration pages', $account),
    );

Status:Needs work» Needs review
StatusFileSize
new8.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,546 pass(es).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,256 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new2.35 KB

Updated per @alexpott suggestion. There are other form elements that only appear under certain conditions in the same AccountFormController, so maybe a clean-up of those would make sense, but is out of the scope of this issue.

Status:Needs review» Reviewed & tested by the community

Looks to be resolving @alexpott's concerns properly.

Clean-up from #53 can be handled in #2058319: AccountFormController form clean-up

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Needs work

Needs a reroll...

git ac https://drupal.org/files/1998648-hide-admin-language-53.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8775  100  8775    0     0   8727      0  0:00:01  0:00:01 --:--:-- 10714
error: patch failed: core/modules/user/lib/Drupal/user/AccountFormController.php:2
error: core/modules/user/lib/Drupal/user/AccountFormController.php: patch does not apply

Issue tags:+Needs reroll

Status:Needs work» Needs review
StatusFileSize
new4.24 KB
FAILED: [[SimpleTest]]: [MySQL] 56,914 pass(es), 294 fail(s), and 134 exception(s).
[ View ]

This would be really great to see land.... Here is a straight reroll. The only change before the reroll seems to be that AccountFormController became an EntityFormControllerNG instead of an EntityFormController. Let's see.

Status:Needs review» Needs work
Issue tags:-D8MI, -sprint, -language-base, -Needs reroll, -RTBC July 1

The last submitted patch, 1998648-hide-admin-language-58.patch, failed testing.

Status:Needs work» Needs review

#58: 1998648-hide-admin-language-58.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+D8MI, +sprint, +language-base, +Needs reroll, +RTBC July 1

The last submitted patch, 1998648-hide-admin-language-58.patch, failed testing.

The fails seem to evolve around the user not being an object *in the tests* that this patch does not touch:

Fatal error: Call to a member function isActive() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/Tests/UserCreateTest.php on line 118
FATAL Drupal\user\Tests\UserCreateTest: test runner returned a non-zero error code (255).
User entity tests 16 passes, 0 fails, and 0 exceptions
Role upgrade test 74 passes, 0 fails, and 0 exceptions
User administration 83 passes, 7 fails, and 3 exceptions
Fatal error: Call to a member function getPreferredLangcode() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/Tests/UserLanguageCreationTest.php on line 71
FATAL Drupal\user\Tests\UserLanguageCreationTest: test runner returned a non-zero error code (255).
User language settings 33 passes, 6 fails, and 2 exceptions
User entity callback tests 7 passes, 0 fails, and 0 exceptions
User edit 54 passes, 31 fails, and 8 exceptions
Fatal error: Call to a member function isActive() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php on line 49
FATAL Drupal\user\Tests\UserRegistrationTest: test runner returned a non-zero error code (255).
Reset password 65 passes, 4 fails, and 3 exceptions
Fatal error: Call to a member function getFileUri() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/Tests/UserPictureTest.php on line 64
FATAL Drupal\user\Tests\UserPictureTest: test runner returned a non-zero error code (255).

I'm not sure looking at this patch how can that be. The patch passes around an $account to user_acess() but the code is already doing that before the patch and the code changes don't see to involved user account objects apart of that. So not sure why is this happening. Anybody wanna take a look? @penyaskito?

I will take a look today or tomorrow, hopefully before the D8MI meeting.

Haven't dedicated a lot of time, but looks like the form has a non-entity field and this is breaking the user creation: "Field administer_users is unknown."
This form value is passed for avoiding to check access to the form in submit() functions. I have to try to clean it up, maybe we can move it there now.

Assigned:penyaskito» Unassigned

I'm pretty sure @penyaskito said it is ok for someone else to have a try at this.

StatusFileSize
new64.55 KB

I used the patch from #58. I got a white screen. I changed the parent class from EntityFormControllerNG to EntityFormController; this worked. I also removed a few unnecessary white spaces.

However, the tests in the patch at #53 are not included in the patch at #58.
I am going to run the tests from #62 locally and make a patch that includes them.

#64still needs to be addressed.

The original requirement is met; the "Administration Pages Language" is not displayed when "User Account (administration setting)" is unchecked:
Screen Shot 2013-08-16 at 2.35.16 PM.png

Issue summary:View changes

began a related issues section.

Status:Needs work» Needs review
StatusFileSize
new2.49 KB
new3.96 KB
FAILED: [[SimpleTest]]: [MySQL] 57,228 pass(es), 123 fail(s), and 11 exception(s).
[ View ]

Here are the patch files

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-66.patch, failed testing.

Issue tags:-Needs reroll

This has been re-rolled.

Assigned:Unassigned» grisendo
Issue tags:+Needs reroll

I was looking at this, the patch doesn't apply anymore and I will try to reroll it.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new3.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998648-hide-admin-language-66_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled, let's see...

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

The last submitted patch, 1998648-hide-admin-language-66.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] 57,709 pass(es), 128 fail(s), and 11 exception(s).
[ View ]

Oops, wrong file. Re-testing.

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-72.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,643 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Cleaned up to fit latest D8 APIs

Status:Needs review» Needs work

wait, we injected the modulehandler in #39 So I think we still want to do that.

Working with @grisendo at the sprint to figure this out.

Status:Needs work» Needs review
StatusFileSize
new565 bytes
new4.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,629 pass(es), 311 fail(s), and 532 exception(s).
[ View ]
new4.26 KB
FAILED: [[SimpleTest]]: [MySQL] 57,698 pass(es), 302 fail(s), and 133 exception(s).
[ View ]

Re-rolling from #58 @Gábor Hojtsy is in patch 77.
there was an error class not found. so we added "NG" to the use. see the interdiff.

now
There is an error when we load the form in the apache log. it was core/modules/user/lib/Drupal/user/AccountFormController.php on line 229

and we (5 of us) do not know how to fix it. seems the args not going to constructor.

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-77b.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
new4.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]

Ok, errors fixed and Dependency Injection applied.
Some whitespaces removed as well.
Sending to test.

Issue tags:+Needs tests

This definitely needs tests. Did we have them before above?

Hm. Might have been nice to just do the changes, and then a separate interdiff for the whitespace fix. What was actually changed?

index 357138c..15eec28 100644
--- a/core/modules/user/lib/Drupal/user/AccountFormController.php
+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -11,13 +11,12 @@
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Language\Language;
use Drupal\Core\Language\LanguageManager;
-use Drupal\Core\Entity\EntityControllerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
/**
  * Form controller for the user account forms.
  */
-abstract class AccountFormController extends EntityFormControllerNG implements EntityControllerInterface {
+abstract class AccountFormController extends EntityFormControllerNG {
    /**
     * The module handler to invoke hooks on.

[..took out the whitespace changes from the diff..]
     * {@inheritdoc}
     */
-   public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
+   public static function create(ContainerInterface $container) {
      return new static(
        $container->get('module_handler'),
        $container->get('language_manager')

So, you took out the implements the Interface? Is that because the NG has it?

@penyaskito 's had tests in #53 but they were not in #58.

Status:Needs review» Needs work

EntityFormControllerNG extends EntityFormController which implements EntityFormControllerInterface, not EntityControllerInterface.

EntityControllerInterface requires to implement "createInstance" method, which was never called.
EntityFormControllerInterface gets "create" method executed when creating that form, and the parameters are passed to the constructor there. Node entity forms do that way.

I am going to take @penyaskito's tests from #53 and apply here.

So, you took out the implements the Interface? Is that because the NG has it?

EntityManager::getController() checks for EntityControllerInterface (which has createInstance()), but EntityManager::getFormController() bypasses this and checks for ContainerInjectionInterface (which has create()) instead.

Status:Needs work» Needs review
StatusFileSize
new8.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,492 pass(es), 1 fail(s), and 14 exception(s).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,901 pass(es), 2 fail(s), and 26 exception(s).
[ View ]
new4.23 KB

Added test from @penyaskito #53.
I am getting an error testing in my local machine, maybe changes will be needed.

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-85.patch, failed testing.

Please, rename drupalPost() calls to drupalPostForm() in the test because of CR: drupalPost() and drupalPostAJAX() have been renamed. Thanks for working on this, Fran!

StatusFileSize
new8.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,768 pass(es).
[ View ]
new4.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,472 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.46 KB

Ok, thanks!
Also changed $this->admin_user->uid to $this->admin_user->id(), and $this->regular_user->uid to $this->regular_user->id()

[edit] This change was because of #2039199: Convert ->uid to ->id(), isAnonymous() and isAuthenticated()

Status:Needs work» Needs review

#88 looks nice from my point of view. Let's see what testbot says.

StatusFileSize
new8.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]
new4.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,568 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new472 bytes

I notice somewhere a whitespace was added. I upload new patches.

Status:Needs review» Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,114 @@
    + * Tests users' ability to change administration pages language for theirself.

    is theirself a word?
    maybe it is a new word. :)

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,114 @@
    +  protected function setUp() {

    standards say should be public. (there might be an issue about projected, but lets stick with the standard)

    https://drupal.org/node/325974

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,114 @@
    +   * Tests that admin language is configurable only if admin language negotiation
    +   * is enabled.

    one line summaries.

    The summary should be one line of up to 80 characters ending in ".".

    https://drupal.org/coding-standards/docs#file

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,114 @@
    +  function testUserAdminLanguageConfigurationNotAvailableWithoutAdminLanguageNegotiation() {

    wow! long method name. :)

  5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,114 @@
    +   * Helper meethod for adding a custom language.

    method.

    wait. this was mentioned before by @Gábor Hojtsy and fixed by @penyaskito.
    Maybe the tests were added from an earlier patch than the one in #53. Wait it was. but somewhere between 36 or so and something else @penyaskito did not use his own fixes. *wink*.

    I'll let you two sort this out.

Status:Needs work» Needs review
StatusFileSize
new8.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,850 pass(es).
[ View ]
new4.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,911 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new2.56 KB

Rerolled, patched and fixed @YesCT suggestions.

Test class docblock:

"+ * Tests users' ability to change their own administration _pages_ language."
Added _pages_

About the method names, I am strongly against abbreviations there. If @YesCT does not have better alternatives, I would leave them as they are before the patch.

PS: Sorry for the lack of dreditor :/

yeah, actually I agree.

I'm not sure if they need to be shorter. I was just noting they were long. :)

Abbreviations make it hard for people who do not know english well to know what we mean. Even those who do know english well can find it confusing.

Before and after screen shots of the most recent core and the most recent patch would be great as a final check here. (and embedding them in the issue summary would be great help)

StatusFileSize
new8.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,878 pass(es).
[ View ]
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,787 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.62 KB

Patches attached for reverting tests' method names.
Screenshots come next.

StatusFileSize
new109.64 KB
new114.53 KB
new109.29 KB
new114.8 KB
new113.86 KB

Manual tests with screenshots:

Without the patch applied, and more than one language enabled, when admin language negotiation is disabled (admin/config/regional/language/detection), both combos are displayed in user/%user/edit (which is wrong):
nopatch-2lang-unchecked.png

With the patch applied, only one language enabled, and admin language negotiation disabled, admin language combo is disabled in user/%user/edit:
patch-1lang-unchecked.png

With the patch applied, more than one language enabled, and admin language negotiation disabled, admin language combo is disabled in user/%user/edit:
patch-2lang-unchecked.png

With the patch applied, only one language enabled, and admin language negotiation enabled, admin language combo is disabled in user/%user/edit:
patch-1lang-checked.png

With the patch applied, more than one language enabled, and admin language negotiation enabled, admin language combo is enabled in user/%user/edit:
patch-2lang-checked.png

Status:Needs review» Needs work

I read all the lines in the tests.
I really wanted to rtbc this, but I think the tests were not testing what we thought they were, and language negotiation is hard to understand. So, we can help future people who might have to edit these tests by explaining them some more. ... so needs work.

  1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +class UserAdminLanguageTest extends WebTestBase {
    +  protected $admin_user;
    +  protected $regular_user;

    I missed that these were added in #88. They need doc blocks.

    https://drupal.org/node/1354#var

    and they need types, which might be documented in the OO standards for classes: https://drupal.org/node/608152

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +   * Tests that admin language negotiation is configurable only if enabled.
    +   */
    +  function testUserAdminLanguageConfigurationNotAvailableWithoutAdminLanguageNegotiation() {
    +    $this->drupalLogin($this->admin_user);
    +    $path = 'user/' . $this->admin_user->id() . '/edit';
    +    $this->drupalGet($path);
    +    // Ensure administration pages language settings widget is not available.
    +    $this->assertNoFieldById('edit-preferred-admin-langcode', '', 'Administration pages language selector not available.');

    This doc block comment does not match what the test is doing.

    it is not looking at the negotiation settings.

    it is ensuring the administration pages language setting is not available.

    to check it is available... (when a negotiation method is using/checked/enabled the detection method Account preferences for administration pages) ... well it is not doing that check.

    the code in this function looks the same as the one above, but without the line $this->setLanguageNegotiation();

    So ... maybe a copy and paste error from way earlier in this issue?

    I think we need to have two languages in this test. the test above shows when we only have one language we will not see the setting on the user edit anyway. so this is not testing having the detection menthod not checked.

    Note the setLanaugageNegotiation checks the detection method and since this test does not have that line, the method is not enabled.

    I think we want to
    1) keep the doc block the same and make the test actually do that by
    a. add language at beginning of this test.
    b. add a second part to this test that setLanguage Negotiation()
    c. does almost the same lines again but does assertFieldById (instead of assert*No*FieldById.

    then it probably makes sense to rename this method. ;)

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +   * Tests that the admin language is configurable only for admins.

    maybe add a paragraph to this doc block that says something like:

    If a user has the permission access administration pages, they should be able to see the setting to pick the language they want those pages in.

    If a user does not have the permission access administration pages, it would confuse them to have a setting for pages they cannot access. They should not be able to set a language for those pages they cannot see.

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +    $this->addCustomLanguage();

    add a comment to say we are adding a custom language, because if there is only one language the setting wont show anyway.

  5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +   * Helper method for setting the language user admin negotiation.

    Actually I think the 80 char summary is confusing.
    How about:
    Sets the User interface negotiation detection method.

    Add a paragraph to explain more. this would not fit in a one line 80 char summary.

    Enables the Account preference for administration pages detection method for the User interface language negotiation type.

  6. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -0,0 +1,113 @@
    +      'language_interface[enabled][language-url]' => TRUE,

    is this off by default? oh. we are resubmitting a form. maybe we are just keeping the default setting which is usual to have url true. I think this is ok.

btw, @grisendo
those screenshots and manual testing are super awesome and complete, clear and easy to understand.

Status:Needs work» Needs review
StatusFileSize
new9.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new5.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,765 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
new5.34 KB

Status:Needs review» Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -54,36 +66,56 @@ function testUserAdminLanguageConfigurationNotAvailableWithOnlyOneLanguage() {
         $this->assertNoFieldById('edit-preferred-admin-langcode', '', 'Administration pages language selector not available.');
    ...
    +    $this->assertFieldById('edit-preferred-admin-langcode', '', 'Administration pages language selector not available.');

    I think the second one here needs another message?

    *is* available

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
    @@ -54,36 +66,56 @@ function testUserAdminLanguageConfigurationNotAvailableWithOnlyOneLanguage() {
    +    // Adds a new language, because with only one language the setting won't show.

    check is longer than 80?

Status:Needs work» Needs review
StatusFileSize
new9.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new5.43 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new2.37 KB

Fixed, and revised/fixed all the comments to fit in 80 characters line.

Also, I found and fixed a typo in Test description, "administation" instead of "administration"

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
@@ -0,0 +1,146 @@
+      'description' => "Tests user's ability to change their administation pages language.",

Status:Needs review» Needs work

Forget it, I made a non-sense phrase as well :( working on it.

StatusFileSize
new9.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,806 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
new5.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,745 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
new926 bytes

Ok, comment fixed.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-103.patch, failed testing.

Status:Needs work» Needs review

7,505,938 Requested by test client #664. 1 hour 15 min ago
7,502,603 Requested by test client #1973. 5 hours 56 min ago
7,502,363 Test request received. 6 hours 18 min ago
cancelled the test.
retesting.

Status:Needs review» Needs work
Issue tags:+Needs tests, +D8MI, +sprint, +language-base, +RTBC July 1

The last submitted patch, 1998648-hide-admin-language-103.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,919 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,822 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new678 bytes

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminLanguageTest.php
@@ -0,0 +1,145 @@
+    $this->regular_user = $this->drupalCreateUser();

I forgot to change this variable to $this->regularUser... ¬¬

Status:Needs review» Needs work

The last submitted patch, 1998648-hide-admin-language-109.patch, failed testing.

StatusFileSize
new9.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,908 pass(es).
[ View ]
new5.43 KB
FAILED: [[SimpleTest]]: [MySQL] 59,031 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new823 bytes

Just read more carefully and saw that assertFieldById takes into account that the field exists AND has a specified value. So, set to English, which is the default one in tests.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

tests are better and the recent interdiffs look good. rtbc if green.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new9.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,031 pass(es).
[ View ]
new5.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,633 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.03 KB

After detecting what happened, I was thinking the test was not good enough... in fact, when we want to check that the combo is invisible, if the combo is visible (and with other language than english selected), tests will be pass also. Yeah, I know, default language is english, but... I don't know xD

I already implemented that change, and tested locally.

Status:Needs review» Reviewed & tested by the community

this looks really great. I tried it as admin, without language: hidden, with language and only english: hidden, with langauge and two languages: hidden, picking detection method: it shows.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -2,13 +2,16 @@
+ * Contains \Drupal\user\AccountFormController.
@@ -16,6 +19,43 @@
+   * The module handler to invoke hooks on.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
...
+  public function __construct(ModuleHandlerInterface $module_handler, LanguageManager $language_manager) {
+    $this->moduleHandler = $module_handler;
...
+      $container->get('module_handler'),

EntityFormController already has the moduleHandler injected through setter injection. Just use $this->moduleHandler ... it'll be there already.

Assigned:grisendo» YesCT
Status:Needs work» Needs review
StatusFileSize
new1.48 KB
new9.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,919 pass(es).
[ View ]

I did this... but
it confuses me.
abstract class AccountFormController extends EntityFormControllerNG {
there is no constructor in
EntityFormControllerNG
or
EntityFormController

but EntityFormController does have the property $moduleHandler;
and

  public function setModuleHandler(ModuleHandlerInterface $module_handler) {
    $this->moduleHandler = $module_handler;
    return $this;
  }

Where is setModuleHandler called?
And does the __constructor in AccountFormController:

  public function __construct(LanguageManager $language_manager) {
    $this->languageManager = $language_manager;
  }

override that? Do we need to call parent? but there is no parent constructor

but in EntityManager in getFormController()
is

      $controller
        ->setTranslationManager($this->translationManager)
        ->setModuleHandler($this->moduleHandler)
        ->setOperation($operation);
      $this->controllers['form'][$operation][$entity_type] = $controller;

only, I do not see how EntityManage works with AccountFormController.

How does this interact with AccountFormController, do the things in the constructor for AccountFormController happen *and* these from getFormController both happen?

uses... for AccountFormController

use Drupal\Core\Entity\EntityFormControllerNG;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Language\Language;
use Drupal\Core\Language\LanguageManager;
use Symfony\Component\DependencyInjection\ContainerInterface;

phpstorm says use Drupal\Core\Extension\ModuleHandlerInterface; is not used.

I'm going to ask someone at the sprint.

Issue summary:View changes

added link to more up to date screenshots.

Assigned:YesCT» Unassigned
StatusFileSize
new580 bytes
new9.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,164 pass(es).
[ View ]

this patch does not have the use.

====== it is not necessary to read the rest of this ==========
also, breaking news as I learn about code:

If I look at AccountFormController,
I can use the structure tab in phpstorm,
the bolded things are made in this class,
but the other things are also available.

clicking on the moduleHandler there, takes me to EntityFormController which has a moduleHandler property, but no constructor that sets it.
I think at this point, I am supposed to know to look for a method that has a name related to setting it.
It does have setModuleHandler().

The phpstorm, find usages of, does not give me anything.
But phpstorm find in path for ->setModuleHandler
in EntityManager, in getFormController()
$controller
->setTranslationManager($this->translationManager)
->setModuleHandler($this->moduleHandler)

before those line though, getFormController creates whatever kind of form
for example: a form to register a user.
...
If I wanted a user, and the register page, User.php
says * "register" = "Drupal\user\RegisterFormController"
so calling getFormController asking for a user object will say to use RegisterFormController. RegisterFormController extends AccountFormController.
...
so it would call the constructor for AccountFormController, and then set the module handler on the controller.

Status:Needs review» Reviewed & tested by the community

checked the interdiff (removing the additional loading of $this->moduleHandler) the rest was already RTBC.
looks good!

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

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new9.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,360 pass(es).
[ View ]
new1.08 KB

Rerolled, interdiff is between patches (diff -u 1998648-hide-admin-language-117.patch 1998648-hide-admin-language-121.patch > interdiff.117-121.txt)

Issue tags:-Needs reroll

Forgot to remove the Needs reroll tag.

Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs reroll

Thanks for the reroll!

Issue tags:-Needs reroll

Removing tags

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1998648-hide-admin-language-121.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D8MI, +sprint, +language-base, +RTBC July 1

Seems to be failing with unrelated fails :/

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed c49e5c8 and pushed to 8.x. Thanks!

Issue tags:-sprint

Yay, so relieving to see this in core now. Thanks everyone for keeping this alive! :)

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

Issue summary:View changes

updated the proposed resolution.