Problem/Motivation

The content admin page does not have an icon if you install Drupal in a language other than English because the classes on elements are deducted from the name of the item originally on the menu (in English). Every other item is saved in English and runtime t()-ed, even the people page which is also a view. But this page for some reason ends up translated in the menu table on save to boot. That is a problem for a multilingual site since they cannot translate this to other languages from English (eg. not use the community provided translations) and also it is plain wrong since all the items are English. The icon not displaying is just the symptom:

ContentAdminTranslated.png

Proposed resolution

Store this menu item in English as well.

Remaining tasks

Figure out why is this happening.
Fix it. Write tests as needed.
Review.
Commit.

User interface changes

The icon would show up properly in translations :)

API changes

Maybe.

Files: 
CommentFileSizeAuthor
#25 menu_context-2115025_21.patch4.7 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,021 pass(es).
[ View ]
#21 interdiff.txt1.67 KBpfrenssen
#21 menu_context-2115025_21-test_only.patch3.7 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 59,052 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#21 menu_context-2115025_21.patch4.71 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 59,121 pass(es).
[ View ]
#12 interdiff.txt768 bytesGábor Hojtsy
#10 menu_context-2115025.patch3.58 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,065 pass(es).
[ View ]
#8 menu_context-2115025.patch3.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,048 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#6 menu-build-context-free.patch1 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,035 pass(es).
[ View ]
#2 GoodTitle.png42.34 KBGábor Hojtsy
#1 views_menu_title.patch711 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,471 pass(es).
[ View ]
ContentAdminTranslated.png131.7 KBGábor Hojtsy

Comments

Gábor Hojtsy’s picture

Status:Active» Needs review
StatusFileSize
new711 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,471 pass(es).
[ View ]

Simple patch to add menu items from views labels loaded in the original (English) language.

Gábor Hojtsy’s picture

StatusFileSize
new42.34 KB

This does fix the problem:

GoodTitle.png

BTW @dawehner expressed some concerns that then domain/group specific overrides will not apply either. Not sure what is the use case of overriding the title of a view that appear in a menu per domain or group. But an alternative for that would be to instantiate a nonexistent language (and go into a language context with that). We can improve the language override lookup to ignore looking for overrides eg. when the language code is 'und', so that can be used to skip just language overrides.

These are the kind of tricks that may get much simpler with #2098119: Replace config context system with baked-in locale support and single-event based overrides which separates language overrides from other overrides in two very different systems (language baked in, other overrides flat, no contexts). I think there is value in fixing this bug before that too, since its pretty bad looking on translated Drupal 8 sites out of the box. Since that patch would remove contexts, it would need to decide what to do in this case in the new setup of things.

andypost’s picture

Suppose fix should be different - context should be set in code that executes hook_menu_alter()

otoh views_get_applicable_views() uses EntityQuery to fetch views executable so probably this operation should be context free or set own context

+++ b/core/modules/views/views.module
@@ -347,6 +347,9 @@ function views_menu() {
function views_menu_alter(&$callbacks) {
...
+  // Ensure to load the free context, so you don't use translated menu titles
+  // and other stuff.
+  config_context_enter('config.context.free');
   $views = views_get_applicable_views('uses_hook_menu');

suppose better to set context in code that calls hook_menu_alter()

dawehner’s picture

otoh views_get_applicable_views() uses EntityQuery to fetch views executable so probably this operation should be context free or set own context

I think this is a bad choice as views_get_applicable_views() might be called in places at which the localized content is important: For example a list of views to select from. You want the human name of a
view to be potentially translated.

andypost’s picture

@dawehner so the only one approach left here - set proper context for hook_menu_alter() not the single implementation. Seems the hook called from 2 places in menu.inc

Gábor Hojtsy’s picture

StatusFileSize
new1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,035 pass(es).
[ View ]

@andypost: That's an option as well. Here you go.

Gábor Hojtsy’s picture

Issue summary:View changes
Issue tags:+Configuration context
dawehner’s picture

StatusFileSize
new3.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,048 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I tried to write a test for that, but sadly I could not make it to pass.

Status:Needs review» Needs work

The last submitted patch, 8: menu_context-2115025.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new3.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,065 pass(es).
[ View ]
new1.27 KB

@dawehner: looks like the copy/instance of the free context in the menu rebuild and are not the same. The class is the same which is totally equivalent in terms of behavior (there is no additional data attached to a free context, so if the class name is the same, the context is the same in this case). I don't exactly know why the instances would be different, but they definitely are. If we just test for the class name, it is totally fine.

Gábor Hojtsy’s picture

BTW thanks @boobaa and @Sweetchuck for helping to look into this / find the bug.

Gábor Hojtsy’s picture

StatusFileSize
new768 bytes

Woops, the interdiff in #10 is unrelated. This one is the right one.

dawehner’s picture

Well yeah, they are not the same but the question is why?

Given that we use the class name of the free context also in the test case, this interdiff might not test what we really want to ensure.

Gábor Hojtsy’s picture

I think since the class name of the context being the free one ensures everything else consistently responds to it, I think we are testing that it is the free context. But we should theoretically have only one instance of it at least if we reference both of them via the container like we do.

pfrenssen’s picture

Assigned:Unassigned» pfrenssen
pfrenssen’s picture

Issue summary:View changes
pfrenssen’s picture

Isn't it possible that the config object has a reference to something that changes between the moment when menu_test_menu() is invoked, and the moment the object is hashed with spl_object_hash()? For example, some unrelated config setting that changes, or a reference to the container that gets hashed along with the config, ..

I don't know how you can check if two PHP variables point to the same referenced memory location. I did some googling and this seems to be only possible by using XDebug, and even then only indirectly by counting the number of references: xdebug_debug_zval(). We can't use that obviously.

pfrenssen’s picture

Assigned:pfrenssen» Unassigned
StatusFileSize
new4.63 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterRebuildTest.php.
[ View ]
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterRebuildTest.php.
[ View ]

Wrote an alternative test. This one creates a menu router item that takes the title from config.

The last submitted patch, 18: menu_context-2115025_18.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 18: menu_context-2115025_18-test_only.patch, failed testing.

pfrenssen’s picture

Status:Needs work» Needs review
StatusFileSize
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,121 pass(es).
[ View ]
new3.7 KB
FAILED: [[SimpleTest]]: [MySQL] 59,052 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.67 KB

What, this failed on directly accessing a function as an array, which is supported on PHP 5.4. According to the System requirements D8 should run on PHP 5.4?

Patch is rerolled against PHP 5.3, and has some documentation improvements.

The last submitted patch, 21: menu_context-2115025_21-test_only.patch, failed testing.

The last submitted patch, 21: menu_context-2115025_21-test_only.patch, failed testing.

Gábor Hojtsy’s picture

I like these tests because they prove exactly what they are supposed to :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterRebuildTest.php
@@ -0,0 +1,60 @@
+ * Definition of Drupal\system\Tests\Menu\MenuRouterRebuildTest.

Should be "Contains" instead of "Definition of" :)

Gábor Hojtsy’s picture

StatusFileSize
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,021 pass(es).
[ View ]

Manually rerolled to say "Contains \Drupal..."

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Perfect!

philipz’s picture

I've applied this patch and installed Drupal 8. The Content item is translated correctly and the icon is displayed too. Looks like the problem is fixed :)

Gábor Hojtsy’s picture

Issue tags:+Vienna2013
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Really nice work on this one!

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags:-sprint

Superb, thanks!

Status:Fixed» Closed (fixed)

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

YesCT’s picture