Problem/Motivation

This is a routing related bug similar to #1831846: Help block is broken with language path prefixes which did not surface yet since we do not have tests covering this scenario.

Steps to reproduce:

- Enable language and interface translation module
- Add one more language
- Translate a string so you have some evidence the language changes or not
- Enable the admin user language detection in language detection and selection
- Set the #1 user's admin language preference to that new language
- Move it up to the first place (so it is actually useful :)
- Now visit pages in the new language with a prefix
=> WSOD

The reason is the code attempts to load a router from the current path when deciding whether it is on an admin page, and if this negotiation method is before the path negotiation, the path will not be updated to remove the langcode yet, so the route cannot be loaded and throws an exception. This exception is never caught. The admin language feature so would only work AFTER the path prefix method, but then it is not useful, since the path prefix would already set the language (unless it was not in a prefix).

Proposed resolution

1. Run inbound path processing before looking up the route.
2. If that did not even help, catch the exception and fall back on assuming it is not an admin path.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

penyaskito’s picture

Issue summary: View changes

I can reproduce this. The bug is now even worse, as the problem is not that the admin chosen language is not used, but that our site is completely broken outside of admin pages

Symfony\Component\Routing\Exception\ResourceNotFoundException: None of the routers in the chain matched this request GET / HTTP/1.1 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Accept-Language: en-us,en;q=0.5 Host: localhost User-Agent: Symfony/2.X in Symfony\Cmf\Component\Routing\ChainRouter->doMatch() (line 202 of core/vendor/symfony-cmf/routing/Symfony/Cmf/Component/Routing/ChainRouter.php).
dawehner’s picture

Priority: Normal » Critical

Isn't that a critical in that case?

penyaskito’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Title: Admin user language preference does not work ahead of path prefixes » Admin user language preference WSOD if ahead of path prefixes
Assigned: Unassigned » Gábor Hojtsy
Issue tags: +sprint

Yeah I could still reproduce @penyaskito's error. The site WSODs on non-admin pages if it is configured ahead of the path language. OTOH the admin language does work almost always if after the path language negotiation, which is interesting. Although still not perfect as the admin toolbar is not displayed in the chosen language on non-admin pages, only on admin pages. Will open or look for an issue on that.

Looking into the WSOD now.

YesCT’s picture

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

Status: Active » Needs review
FileSize
2.73 KB

All right, figured this out. So when the admin path negotiation is before the URL path negotiation, there may be a prefix in the URL but it is not yet removed from the request path. So we are dealing with a full original path. That will not find a matching route and throws that ResourceNotFoundException that we never catch. Unfortunately there is a chicken-egg AKA circular dependency problem here. We want to load a route for the path but the language negotiation cleans up the URL so it can be loaded in the first place. But we are in the middle of language negotiation. Fantastic, right?

So we cannot really do many clever things here AFAIS. What we can do is to try a few variants of the URL. We can remove the first part if the full did not match (the prefix may only be 1 component). If that did not work either, we can use the front page, so we have something to work against. Although this may lead to false positives in the lookup, the only reason we use the routes is to tell if they are admin or not so if we happen to pick a bad route (eg. someone visits /boo/this/page/is/not/there), not much will go wrong. It will figure out this path does not have a route and then the shorter one neither, and then use the front page for this negotiation and move on. In any case, we need to catch the exception and handle it. This way it works fine for me.

Now needs tests.

Gábor Hojtsy’s picture

BTW this is not a very general solution, theoretically any negotiation method may add prefixes and suffixes to the URL and if there is a suffix for example, this will totally fail and always end up with the front page. At least it will not WSOD. To have a very generic solution where any negotiator may have a say in modifying the URL before the admin negotiator looks it up we would need some API changes or at least additions. The code as proposed will make the admin preferred language negotiator still not compatible with another negotiator that may set a URL suffix (as I've seen some developers do).

Gábor Hojtsy’s picture

So I figured out the inbound path processor can be used in itself to remove any prefix/suffix. No need to hack. No circular dependency then. Also @penyaskito points out looking up / and then figuring out its not admin is pointless, we can just return FALSE on the exception. Now with the path processing on, we can avoid one more step, so much cleaner fix.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Will not continue for a bit at least. Maybe tomorrow if nobody beats me to it.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

Working on tests.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
Issue summary: View changes
Issue tags: -Needs tests
FileSize
3.16 KB
6.88 KB

There you go, first the interdiff and test only patch. Apparently there was no test for when the admin language negotiation actually applied. Such a test would expose the fails. I added tests for the method not applying if there is no user setting yet (which is why it makes sense to move the admin language method first and foremost, since it will only apply on admin pages and only for those who have the setting), then only applying for pages where there was no language in the URL (if URL took precedence) or regardless of URL (if admin took precedence). This locally passes with the fix and fails hard without. Reviews please.

Updated issue summary.

The last submitted patch, 11: interdiff-and-test-only.patch, failed testing.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

The test is more concise that what I expected from the steps to reproduce, congrats!

Looks good to go IMHO.

alexpott’s picture

+++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
@@ -105,10 +118,20 @@ public function getLangcode(Request $request = NULL) {
+          // Process the path as an inbound path. This will remove any language
+          // prefixes and other path components that inbound processing would
+          // clear out, so we can attempt to load the route clearly.
+          $path = $this->pathProcessorManager->processInbound(urldecode(trim($request->getPathInfo(), '/')), $request);
+          $attributes = $this->router->match('/' . $path);

I'm concerned that this means we'll be doing the same processInbound logic twice on every request. Once the language module is enabled.

Gábor Hojtsy’s picture

@alexpott: But we need to. If the admin negotiation method is before the URL method (or some other method that appends language to the URL that I've seen at a client earlier this year), the path will be in its original form. How else can you tell if its an admin path? The only way is to figure it out from the route, so you need the route. Since you are there before the other methods modified the path to be able to load the route based on them, you need to process the path to be able to load the route.

catch’s picture

Could we look at a follow-up to remove (or document the lack of) support for appending language to the URL, or alternatively some way of enforcing that particular negotiation methods run before others? Not sure how viable that is, but feels like the responsibility for doing tricky stuff with URLs should be on the modules doing that, not the other negotiation methods.

Gábor Hojtsy’s picture

@catch: the negotiation methods need to be able to modify the URL, and we don't really need to know what they do, so long as we can resolve it when needed. We do need to run the URL processing also to resolve path aliases, unless we can load routes based on path aliases (I doubt). So even if there is no URL modification but just a path alias, we could still not load the route for it. We need to run the path processors AFAIS, no way around it here.

Gábor Hojtsy’s picture

Assigned: Unassigned » catch

@Dries wanted to take a look but I said there is already @alexpott and @catch here, and they have reservations, so better assign to one of them to make this clearer.

Gábor Hojtsy’s picture

Assigned: catch » Unassigned

Talked to @alexpott again, and he has no firm reservations anymore, also he may look at this later unless @catch or @Dries beats him to it.

  • catch committed 16caf92 on 8.0.x
    Issue #1833010 by Gábor Hojtsy: Fixed Admin user language preference...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Gone ahead and committed/pushed to 8.x, don't see a way around this indeed.

Committed/push to 8.0.x, thanks!

alexpott’s picture

Was going to knock this back for a slight improvement... perhaps this can be a followup issue.

I think we can improve the amount of times we do the processing of inbound requests by looking at how getAdminPath is used...

    // User preference (only for authenticated users).
    if ($this->languageManager && $this->currentUser->isAuthenticated() && $this->isAdminPath($request)) {
      $preferred_admin_langcode = $this->currentUser->getPreferredAdminLangcode();
      $default_langcode = $this->languageManager->getDefaultLanguage()->id;
      $languages = $this->languageManager->getLanguages();
      if (!empty($preferred_admin_langcode) && $preferred_admin_langcode != $default_langcode && isset($languages[$preferred_admin_langcode])) {
        $langcode = $preferred_admin_langcode;
      }
    }

We could change this to:

    // User preference (only for authenticated users).
    if ($this->languageManager && $this->currentUser->isAuthenticated()) {
      $preferred_admin_langcode = $this->currentUser->getPreferredAdminLangcode();
      if ($preferred_admin_langcode != $this->languageManager->getDefaultLanguage()->getId()) {
        $languages = $this->languageManager->getLanguages();
        if (isset($languages[$preferred_admin_langcode]) && $this->isAdminPath($request)) {
          $langcode = $preferred_admin_langcode;
      }
    }

And therefore only do the check when absolutely necessary. $this->currentUser->getPreferredAdminLangcode() can never be empty since it returns the default language if the value is null.

Gábor Hojtsy’s picture

Issue tags: -sprint

@alexpott: there are definitely things to improve there. First I think its problematic that there is no way to set NO admin preferred language. If you are to save your user profile sometime once the admin language option is there, you will get something saved for your user. The check on whether it is the same as the default language is bogus. There is no base to assume that means you wanted no admin language, in fact it may be you wanted to force pages to that language. If there is a NONE option to out out, then we can check for that instead of returning the default language for the getPreferredAdminLangcode() (which was in itself silly IMHO). Then only users who have something set would get this, and we would only run it for that case.

Opened #2313157: Optimize admin language detection and make it optional for this.

Gábor Hojtsy’s picture

@alexpott's suggestion now implemented at #2313157: Optimize admin language detection and make it optional.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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