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).
If this setting is disabled, it is confusing to have the "Administration pages language" setting:
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
- Install Drupal 8
- Add more than one language
- Go to: admin/config/regional/language/detection
- Notice that the default selection for the account administration pages setting is unchecked
- Go to your profile edit page... e.g. user/[uid]/edit... which is user/1/edit for super user
- Notice that the "Administration pages language" option is shown
- Repeat for when the account administration pages setting is checked (still shows which is good in this case)
Related issues
Comment | File | Size | Author |
---|---|---|---|
#121 | interdiff.117-121.txt | 1.08 KB | penyaskito |
#121 | 1998648-hide-admin-language-121.patch | 9.24 KB | penyaskito |
#118 | 1998648-hide-admin-language-117.patch | 9.23 KB | YesCT |
#118 | interdiff-117-118.txt | 580 bytes | YesCT |
#117 | 1998648-hide-admin-language-116.patch | 9.28 KB | YesCT |
Comments
Comment #1
Kristen PolFix tags. Assign to me for now.
Comment #2
Kristen PolHere'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.
Comment #3
Kristen PolComment #4
Kristen PolComment #6
Gábor HojtsySeems 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...
Comment #7
Kristen PolHere'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)
Comment #8
attiks CreditAttribution: attiks commentedNice, tested and works well.
Comment #9
alexpottPlease change this to...
Comment #10
alexpottComment #11
Gábor HojtsyAlso LANGUAGE_CONFIGURABLE changed too. #1620010: Move LANGUAGE constants to the Language class.
Comment #12
Kristen PolThanks for the feedback. I will try to get to these this week.
Comment #13
Kristen PolHere's an updated patch.
Comment #14
YesCT CreditAttribution: YesCT commented[ignore this comment. uploaded wrong patch]
Comment #15
YesCT CreditAttribution: YesCT commentedright patch this time.
module exists is depreciated.
(#2007684: Update deprecated doc for module_exists() and call method inside)
Also, added a comment.
Comment #16
Kristen PolTested and looks great and checked the code and it looks good, too. Thanks Cathy!
Comment #17
YesCT CreditAttribution: YesCT commentedI agree rtbc, concerns from #9 and #11 have been addressed.
Comment #18
Dries CreditAttribution: Dries commentedI think we should write some tests for this.
Comment #19
Kristen Pol@YesCT - let me know if you can either write the tests or give me a quick tutorial on how to write tests :)
Comment #20
YesCT CreditAttribution: YesCT commentedSome 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
Comment #21
Kristen PolThanks @YesCT! I'll see if I can read those tomorrow.
Comment #22
Kristen PolI skimmed through them and it doesn't seem too scary ;) I'll see if I can take a stab at it tonight.
Comment #23
Kristen PolI've written some tests and have them in their own patch (named *FAIL) and included them with the fix in another patch (named *PASS).
Comment #25
Gábor HojtsyFirst 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.
Comment #26
Kristen PolAh, 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!
Comment #27
Gábor HojtsyI think have a non admin user and an admin user. No need to change permissions. Also not needed to have three users.
Comment #28
Kristen PolI'm still planning on updating the tests... been sick and threw my back out yesterday :/
Comment #29
Kristen PolI'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.
Comment #30
Gábor HojtsyComment #31
Gábor HojtsyDefinitely not novice anymore.
Comment #32
penyaskitoAssigning to me.
Comment #33
penyaskitoFor 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.
Comment #34
penyaskitoFor the record: first patch should be red, second one green if it works as expected.
Comment #35
Gábor HojtsyI think we do \Drupal directly actually, not use Drupal(?!)
This should currently be Contains and \Drupal
Can we fit this into 80 chars?
Should fit on one line if at all possible :)
meethod => method
Comment #36
penyaskitoMy best is:
Other issues have been fixed.
Comment #37
Gábor Hojtsy@YesCT echoes that using Drupal direct is not good, the module handler needs to be injected.
Comment #38
penyaskito@YesCT asked to add DI to the form, so needs work.
Comment #39
penyaskitow00t! I know a little more about d8 than yesterday, thanks @YesCT :)
Added dependency injection to the form.
Comment #40
Gábor HojtsyLooks good functionally. Great changes, especially the DI improvement.
Comment #41
Kristen PolThanks penyaskito! And thanks Gábor for reviewing!
Comment #42
alexpottWe 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...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...Comment #43
penyaskito@alexpott was right, thanks for review :D
Comment #44
Gábor HojtsyThe interdiff looks all logical to me on review.
Comment #45
Kristen PolLooks good to me too... can we put back to RTBC or need to do something more testing?
Comment #46
Gábor HojtsyNow that the tests proved it works too, not just looking good, yay!
Comment #47
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #48
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #49
penyaskito#43: drupal8.hide_admin_lang_if_not_user.only-test-must-fail.1998648-43.patch queued for re-testing.
Comment #50
penyaskito#43: drupal8.hide_admin_lang_if_not_user.1998648-43.patch queued for re-testing.
Comment #51
Gábor Hojtsy#43: drupal8.hide_admin_lang_if_not_user.1998648-43.patch queued for re-testing.
Comment #52
alexpottIt'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...
Comment #53
penyaskitoUpdated 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.
Comment #54
Gábor HojtsyLooks to be resolving @alexpott's concerns properly.
Comment #55
penyaskitoClean-up from #53 can be handled in #2058319: AccountForm clean-up
Comment #55.0
penyaskitoUpdated issue summary.
Comment #56
alexpottNeeds a reroll...
Comment #57
YesCT CreditAttribution: YesCT commentedComment #58
Gábor HojtsyThis 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.
Comment #60
Gábor Hojtsy#58: 1998648-hide-admin-language-58.patch queued for re-testing.
Comment #62
Gábor HojtsyThe fails seem to evolve around the user not being an object *in the tests* that this patch does not touch:
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?
Comment #63
penyaskitoI will take a look today or tomorrow, hopefully before the D8MI meeting.
Comment #64
penyaskitoHaven'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.
Comment #65
YesCT CreditAttribution: YesCT commentedI'm pretty sure @penyaskito said it is ok for someone else to have a try at this.
Comment #66
jair CreditAttribution: jair commentedI 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:
Comment #66.0
jair CreditAttribution: jair commentedbegan a related issues section.
Comment #67
jair CreditAttribution: jair commentedHere are the patch files
Comment #69
jair CreditAttribution: jair commentedThis has been re-rolled.
Comment #70
grisendo CreditAttribution: grisendo commentedI was looking at this, the patch doesn't apply anymore and I will try to reroll it.
Comment #71
grisendo CreditAttribution: grisendo commentedRe-rolled, let's see...
Comment #73
grisendo CreditAttribution: grisendo commentedOops, wrong file. Re-testing.
Comment #75
grisendo CreditAttribution: grisendo commentedCleaned up to fit latest D8 APIs
Comment #76
YesCT CreditAttribution: YesCT commentedwait, 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.
Comment #77
grisendo CreditAttribution: grisendo commentedRe-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.
Comment #79
grisendo CreditAttribution: grisendo commentedOk, errors fixed and Dependency Injection applied.
Some whitespaces removed as well.
Sending to test.
Comment #80
Gábor HojtsyThis definitely needs tests. Did we have them before above?
Comment #81
YesCT CreditAttribution: YesCT commentedHm. Might have been nice to just do the changes, and then a separate interdiff for the whitespace fix. What was actually changed?
[..took out the whitespace changes from the diff..]
So, you took out the implements the Interface? Is that because the NG has it?
Comment #82
YesCT CreditAttribution: YesCT commented@penyaskito 's had tests in #53 but they were not in #58.
Comment #83
grisendo CreditAttribution: grisendo commentedEntityFormControllerNG 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.
Comment #84
XanoEntityManager::getController() checks for EntityControllerInterface (which has createInstance()), but EntityManager::getFormController() bypasses this and checks for ContainerInjectionInterface (which has create()) instead.
Comment #85
grisendo CreditAttribution: grisendo commentedAdded test from @penyaskito #53.
I am getting an error testing in my local machine, maybe changes will be needed.
Comment #87
penyaskitoPlease, rename drupalPost() calls to drupalPostForm() in the test because of CR: drupalPost() and drupalPostAJAX() have been renamed. Thanks for working on this, Fran!
Comment #88
grisendo CreditAttribution: grisendo commentedOk, 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()
Comment #89
penyaskito#88 looks nice from my point of view. Let's see what testbot says.
Comment #90
grisendo CreditAttribution: grisendo commentedI notice somewhere a whitespace was added. I upload new patches.
Comment #91
YesCT CreditAttribution: YesCT commentedis theirself a word?
maybe it is a new word. :)
standards say should be public. (there might be an issue about projected, but lets stick with the standard)
https://drupal.org/node/325974
one line summaries.
The summary should be one line of up to 80 characters ending in ".".
https://drupal.org/coding-standards/docs#file
wow! long method name. :)
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.
Comment #92
grisendo CreditAttribution: grisendo commentedRerolled, patched and fixed @YesCT suggestions.
Comment #93
penyaskitoTest 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 :/
Comment #94
YesCT CreditAttribution: YesCT commentedyeah, 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)
Comment #95
grisendo CreditAttribution: grisendo commentedPatches attached for reverting tests' method names.
Screenshots come next.
Comment #96
grisendo CreditAttribution: grisendo commentedManual 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):
With the patch applied, only one language enabled, and admin language negotiation disabled, admin language combo is disabled in user/%user/edit:
With the patch applied, more than one language enabled, and admin language negotiation disabled, admin language combo is disabled in user/%user/edit:
With the patch applied, only one language enabled, and admin language negotiation enabled, admin language combo is disabled in user/%user/edit:
With the patch applied, more than one language enabled, and admin language negotiation enabled, admin language combo is enabled in user/%user/edit:
Comment #97
YesCT CreditAttribution: YesCT commentedI 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.
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
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. ;)
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.
add a comment to say we are adding a custom language, because if there is only one language the setting wont show anyway.
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.
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.
Comment #98
YesCT CreditAttribution: YesCT commentedbtw, @grisendo
those screenshots and manual testing are super awesome and complete, clear and easy to understand.
Comment #99
grisendo CreditAttribution: grisendo commentedComment #100
YesCT CreditAttribution: YesCT commentedI think the second one here needs another message?
*is* available
check is longer than 80?
Comment #101
grisendo CreditAttribution: grisendo commentedFixed, 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"
Comment #102
grisendo CreditAttribution: grisendo commentedForget it, I made a non-sense phrase as well :( working on it.
Comment #103
grisendo CreditAttribution: grisendo commentedOk, comment fixed.
Comment #104
grisendo CreditAttribution: grisendo commentedComment #106
YesCT CreditAttribution: YesCT commented7,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.
Comment #107
YesCT CreditAttribution: YesCT commented#103: 1998648-hide-admin-language-103.patch queued for re-testing.
Comment #109
grisendo CreditAttribution: grisendo commentedI forgot to change this variable to $this->regularUser... ¬¬
Comment #111
grisendo CreditAttribution: grisendo commentedJust 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.
Comment #112
grisendo CreditAttribution: grisendo commentedComment #113
YesCT CreditAttribution: YesCT commentedtests are better and the recent interdiffs look good. rtbc if green.
Comment #114
grisendo CreditAttribution: grisendo commentedAfter 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.
Comment #115
YesCT CreditAttribution: YesCT commentedthis 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.
Comment #115.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #116
alexpottEntityFormController already has the moduleHandler injected through setter injection. Just use $this->moduleHandler ... it'll be there already.
Comment #117
YesCT CreditAttribution: YesCT commentedI 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
Where is setModuleHandler called?
And does the __constructor in AccountFormController:
override that? Do we need to call parent? but there is no parent constructor
but in EntityManager in getFormController()
is
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
phpstorm says use Drupal\Core\Extension\ModuleHandlerInterface; is not used.
I'm going to ask someone at the sprint.
Comment #117.0
YesCT CreditAttribution: YesCT commentedadded link to more up to date screenshots.
Comment #118
YesCT CreditAttribution: YesCT commentedthis 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.
Comment #119
Schnitzel CreditAttribution: Schnitzel commentedchecked the interdiff (removing the additional loading of $this->moduleHandler) the rest was already RTBC.
looks good!
Comment #120
alexpottPatch no longer applies.
Comment #121
penyaskitoRerolled, interdiff is between patches (diff -u 1998648-hide-admin-language-117.patch 1998648-hide-admin-language-121.patch > interdiff.117-121.txt)
Comment #122
penyaskitoForgot to remove the Needs reroll tag.
Comment #123
Gábor HojtsyThanks for the reroll!
Comment #124
tim.plunkettRemoving tags
Comment #125
Gábor Hojtsy#121: 1998648-hide-admin-language-121.patch queued for re-testing.
Comment #126
Xano#121: 1998648-hide-admin-language-121.patch queued for re-testing.
Comment #128
Gábor Hojtsy#121: 1998648-hide-admin-language-121.patch queued for re-testing.
Comment #129
Gábor HojtsySeems to be failing with unrelated fails :/
Comment #130
Gábor HojtsyComment #131
alexpottCommitted c49e5c8 and pushed to 8.x. Thanks!
Comment #132
Gábor HojtsyYay, so relieving to see this in core now. Thanks everyone for keeping this alive! :)
Comment #133.0
(not verified) CreditAttribution: commentedupdated the proposed resolution.
Comment #134
qmjeelani CreditAttribution: qmjeelani commentedThe "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.