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

CommentFileSizeAuthor
#121 interdiff.117-121.txt1.08 KBpenyaskito
#121 1998648-hide-admin-language-121.patch9.24 KBpenyaskito
#118 1998648-hide-admin-language-117.patch9.23 KBYesCT
#118 interdiff-117-118.txt580 bytesYesCT
#117 1998648-hide-admin-language-116.patch9.28 KBYesCT
#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
#114 1998648-hide-admin-language-114.patch9.67 KBgrisendo
#111 1998648-hide-admin-language-109-111.interdiff.txt823 bytesgrisendo
#111 1998648-hide-admin-language-111.only-test.must-fail.patch5.43 KBgrisendo
#111 1998648-hide-admin-language-111.patch9.49 KBgrisendo
#109 1998648-hide-admin-language-103-109.interdiff.txt678 bytesgrisendo
#109 1998648-hide-admin-language-109.only-test.must-fail.patch5.43 KBgrisendo
#109 1998648-hide-admin-language-109.patch9.48 KBgrisendo
#103 1998648-hide-admin-language-101-103.interdiff.txt926 bytesgrisendo
#103 1998648-hide-admin-language-103.only-test.must-fail.patch5.43 KBgrisendo
#103 1998648-hide-admin-language-103.patch9.48 KBgrisendo
#101 1998648-hide-admin-language-98-101.interdiff.txt2.37 KBgrisendo
#101 1998648-hide-admin-language-101.only-test.must-fail.patch5.43 KBgrisendo
#101 1998648-hide-admin-language-101.patch9.49 KBgrisendo
#99 1998648-hide-admin-language-95-98.interdiff.txt5.34 KBgrisendo
#99 1998648-hide-admin-language-98.only-test.must-fail.patch5.47 KBgrisendo
#99 1998648-hide-admin-language-98.patch9.52 KBgrisendo
#88 1998648-hide-admin-language-85-88.interdiff.txt3.46 KBgrisendo
#88 1998648-hide-admin-language-88.only-test.must-fail.patch4.29 KBgrisendo
#88 1998648-hide-admin-language-88.patch8.35 KBgrisendo
#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
#95 1998648-hide-admin-language-95.patch8.31 KBgrisendo
#92 1998648-hide-admin-language-90-92.interdiff.txt2.56 KBgrisendo
#92 1998648-hide-admin-language-92.only-test.must-fail.patch4.18 KBgrisendo
#92 1998648-hide-admin-language-92.patch8.24 KBgrisendo
#90 1998648-hide-admin-language-88-89.interdiff.txt472 bytesgrisendo
#90 1998648-hide-admin-language-89.only-test.must-fail.patch4.29 KBgrisendo
#90 1998648-hide-admin-language-89.patch8.35 KBgrisendo
#85 1998648-hide-admin-language-79-85.interdiff.txt4.23 KBgrisendo
#85 1998648-hide-admin-language-85.only-test.must-fail.patch4.23 KBgrisendo
#85 1998648-hide-admin-language-85.patch8.28 KBgrisendo
#79 1998648-hide-admin-language-79.patch4.06 KBgrisendo
#79 interdiff.77b.79.txt2.92 KBgrisendo
#77 1998648-hide-admin-language-77.patch4.26 KBgrisendo
#77 1998648-hide-admin-language-77b.patch4.22 KBgrisendo
#77 interdiff.77.77b.txt565 bytesgrisendo
#75 1998648-hide-admin-language-75.patch1.65 KBgrisendo
#73 1998648-hide-admin-language-72.patch2.56 KBgrisendo
#71 1998648-hide-admin-language-66.patch3.96 KBgrisendo
#67 1998648-hide-admin-language-66.patch3.96 KBjair
#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
#53 1998648-hide-admin-language.interdiff-43-53.txt2.35 KBpenyaskito
#53 1998648-hide-admin-language-53.only-test.must-fail.patch4.23 KBpenyaskito
#53 1998648-hide-admin-language-53.patch8.57 KBpenyaskito
#43 interdiff.39-43.txt3.02 KBpenyaskito
#43 drupal8.hide_admin_lang_if_not_user.only-test-must-fail.1998648-43.patch4.23 KBpenyaskito
#43 drupal8.hide_admin_lang_if_not_user.1998648-43.patch8.81 KBpenyaskito
#39 interdiff.di-ftw.yesct-ftw.txt2.42 KBpenyaskito
#39 drupal8.hide_admin_lang_if_not_user.only-test.1998648-39.patch4.23 KBpenyaskito
#39 drupal8.hide_admin_lang_if_not_user.1998648-39.patch7.88 KBpenyaskito
#36 interdiff.txt3.17 KBpenyaskito
#36 drupal8.hide_admin_lang_if_not_user.only-test.1998648-36.patch4.23 KBpenyaskito
#36 drupal8.hide_admin_lang_if_not_user.1998648-36.patch6.38 KBpenyaskito
#33 interdiff.txt7.6 KBpenyaskito
#33 drupal8.hide_admin_lang_if_not_user.only-test.1998648-33.patch4.37 KBpenyaskito
#33 drupal8.hide_admin_lang_if_not_user.1998648-33.patch6.53 KBpenyaskito
#23 drupal8.hide_admin_lang_if_not_user.1998648-23-FAIL.patch3.97 KBKristen Pol
#23 drupal8.hide_admin_lang_if_not_user.1998648-23-PASS.patch6.07 KBKristen Pol
#15 drupal.1998648.hide_admin_lang_if_not_used.14.patch2.1 KBYesCT
#15 interdiff-13-14.txt1.13 KBYesCT
#14 drupal-1945226-Add-language-selector-on-menus-50.patch3.51 KBYesCT
#14 interdiff-49-50.txt827 bytesYesCT
#13 drupal.1998648.hide_admin_lang_if_not_used.13.patch1.8 KBKristen Pol
#7 drupal.1998648.hide_admin_lang_if_not_used.7.patch1.82 KBKristen Pol
#2 drupal.1998648.hide_admin_lang_if_not_used.2.patch1.31 KBKristen Pol
d8mi-admin-pages-language-detection.png46.79 KBKristen Pol
d8mi-admin-pages-language.png31.58 KBKristen Pol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Kristen Pol’s picture

Kristen Pol’s picture

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.

Kristen Pol’s picture

Issue tags: +language-base
Kristen Pol’s picture

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.

Gábor Hojtsy’s picture

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...

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

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)

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Nice, tested and works well.

alexpott’s picture

+++ 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),
        );
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Gábor Hojtsy’s picture

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

Kristen Pol’s picture

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

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

Here's an updated patch.

YesCT’s picture

[ignore this comment. uploaded wrong patch]

YesCT’s picture

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.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

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

YesCT’s picture

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

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I think we should write some tests for this.

Kristen Pol’s picture

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

YesCT’s picture

Some references:
https://drupal.org/contributor-tasks/write-tests

See http://drupal.org/simpletest for a start on using SimpleTest. and is also xjm's midwest presentation: http://midwest-developer-summit.com/sessions/automated-testing-drupal

Kristen Pol’s picture

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

Kristen Pol’s picture

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

Kristen Pol’s picture

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.

Gábor Hojtsy’s picture

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.

Kristen Pol’s picture

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!

Gábor Hojtsy’s picture

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

Kristen Pol’s picture

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

Kristen Pol’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +sprint
Gábor Hojtsy’s picture

Issue tags: -Novice

Definitely not novice anymore.

penyaskito’s picture

Assigned: Unassigned » penyaskito

Assigning to me.

penyaskito’s picture

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.

penyaskito’s picture

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

Gábor Hojtsy’s picture

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

penyaskito’s picture

+  /**
+   * 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

penyaskito’s picture

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

penyaskito’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Kristen Pol’s picture

Thanks penyaskito! And thanks Gábor for reviewing!

alexpott’s picture

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...

penyaskito’s picture

@alexpott was right, thanks for review :D

Gábor Hojtsy’s picture

The interdiff looks all logical to me on review.

Kristen Pol’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

YesCT’s picture

Issue tags: +RTBC July 1

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

YesCT’s picture

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

penyaskito’s picture

penyaskito’s picture

Gábor Hojtsy’s picture

alexpott’s picture

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),
    );
penyaskito’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks to be resolving @alexpott's concerns properly.

penyaskito’s picture

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

penyaskito’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

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
YesCT’s picture

Issue tags: +Needs reroll
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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.

Gábor Hojtsy’s picture

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?

penyaskito’s picture

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

penyaskito’s picture

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.

YesCT’s picture

Assigned: penyaskito » Unassigned

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

jair’s picture

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

jair’s picture

Issue summary: View changes

began a related issues section.

jair’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
3.96 KB

Here are the patch files

Status: Needs review » Needs work

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

jair’s picture

Issue tags: -Needs reroll

This has been re-rolled.

grisendo’s picture

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.

grisendo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.96 KB

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.

grisendo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.56 KB

Oops, wrong file. Re-testing.

Status: Needs review » Needs work

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

grisendo’s picture

Status: Needs work » Needs review
FileSize
1.65 KB

Cleaned up to fit latest D8 APIs

YesCT’s picture

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.

grisendo’s picture

Status: Needs work » Needs review
FileSize
565 bytes
4.22 KB
4.26 KB

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.

grisendo’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
4.06 KB

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

Gábor Hojtsy’s picture

Issue tags: +Needs tests

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

YesCT’s picture

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?

YesCT’s picture

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

grisendo’s picture

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.

Xano’s picture

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.

grisendo’s picture

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.

penyaskito’s picture

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

grisendo’s picture

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()

penyaskito’s picture

Status: Needs work » Needs review

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

grisendo’s picture

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

YesCT’s picture

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.

grisendo’s picture

Rerolled, patched and fixed @YesCT suggestions.

penyaskito’s picture

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 :/

YesCT’s picture

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)

grisendo’s picture

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

grisendo’s picture

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

YesCT’s picture

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.

YesCT’s picture

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

grisendo’s picture

YesCT’s picture

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?

grisendo’s picture

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.",
grisendo’s picture

Status: Needs review » Needs work

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

grisendo’s picture

grisendo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

YesCT’s picture

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.

YesCT’s picture

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.

grisendo’s picture

+++ 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.

grisendo’s picture

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.

grisendo’s picture

Status: Needs work » Needs review
YesCT’s picture

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

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

grisendo’s picture

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.

YesCT’s picture

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.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

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.

YesCT’s picture

Assigned: grisendo » YesCT
Status: Needs work » Needs review
FileSize
1.48 KB
9.28 KB

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.

YesCT’s picture

Issue summary: View changes

added link to more up to date screenshots.

YesCT’s picture

Assigned: YesCT » Unassigned
FileSize
580 bytes
9.23 KB

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.

Schnitzel’s picture

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!

alexpott’s picture

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

Patch no longer applies.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
1.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)

penyaskito’s picture

Issue tags: -Needs reroll

Forgot to remove the Needs reroll tag.

Gábor Hojtsy’s picture

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

Thanks for the reroll!

tim.plunkett’s picture

Issue tags: -Needs reroll

Removing tags

Gábor Hojtsy’s picture

Issue tags: -D8MI, -sprint, -language-base, -RTBC July 1
Xano’s picture

Status: Reviewed & tested by the community » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-base, +RTBC July 1
Gábor Hojtsy’s picture

Seems to be failing with unrelated fails :/

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c49e5c8 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

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.

Anonymous’s picture

Issue summary: View changes

updated the proposed resolution.

qmjeelani’s picture

The "Administration pages language" is using in Drupal 8. The setting can be used in Multilingual site, It is used for keep the Admin Side of the drupal should be in Language selected in this drop down.