Problem
PathSubscriber::onKernelRequestLanguageResolve()
cannot be converted to a path processor, because it relies on a side-effect of language initialization.- The language manager is tied to the request scope.
Goal
- Split language initialization and path resolution into two separate processes.
Details
PathSubscriber::onKernelRequestLanguageResolve()
currently shows this:
/** * Decode language information embedded in the request path. * * @todo Refactor this entire method to inline the relevant portions of * drupal_language_initialize(). See the inline comment for more details. * * @param Symfony\Component\HttpKernel\Event\GetResponseEvent $event * The Event to process. */ public function onKernelRequestLanguageResolve(GetResponseEvent $event) { // drupal_language_initialize() combines: // - Determination of language from $request information (e.g., path). // - Determination of language from other information (e.g., site default). // - Population of determined language into drupal_container(). // - Removal of language code from _current_path(). // @todo Decouple the above, but for now, invoke it and update the path // prior to front page and alias resolution. When above is decoupled, also // add 'langcode' (determined from $request only) to $request->attributes. drupal_language_initialize(); }
-
This only works, because, at the time the listener is called, the container is within request scope.
-
Thus, when
drupal_language_initialize()
callslanguage()
, it uses theLanguageManager
rather thanlanguage_default()
. -
The
LanguageManager
, which depends on the request, passes the request on to language negotiation functions. One of them is URL-based. That callback performs this:$current_path = $request->attributes->get('system_path'); if (!isset($current_path)) { $current_path = trim($request->getPathInfo(), '/'); } list($language, $path) = language_url_split_prefix($current_path, $languages); // Store the correct system path, i.e. minus the path prefix, in the // request. $request->attributes->set('system_path', $path);
-
There is no good reason for binding the language manager to the request. It was originally introduced with #1599108: Allow modules to register services and subscriber services (events).
Proposed solution
-
Make the LanguageManager independent of request scope. It is always responsible for language initialization, even if there is no request.
-
Add an event listener that sets the request on the LanguageManager, in order to perform request-aware language initialization when there is a request.
-
Convert all path listeners (the methods in the PathSubscriber class) to path processors, which are called in sequence by a single listener (#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence).
Notes
-
The language-aware path resolution still needs to happen in the onKernelRequestLanguageResolve() listener (i.e. the process whereby e.g. 'fr/my/path' gets converted to 'my/path', which in turn is set as the 'system_path' request attribute).
-
When moving language initialization into its own request listener, we need a (new) way to retrieve the 'system_path' request attribute.
-
The proper solution for this is to make Language module provide a "PathFutzer" class that we can call a method on.
That's done in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence. We need a temporary solution, which just stores the language-derived path from language negotiation in a static variable, and we then retrieve it from there in the path listener.
This is dirty, but only a temporary measure.
Comments
Comment #1
katbailey CreditAttribution: katbailey commentedThis patch includes work originally started in #1862202: Objectify the language system and then continued in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence - it represents the changes that are required by both of those issues.
Comment #3
sunI've taken the freedom to completely rewrite the issue summary, in order to understand the proposal. Please correct where I misunderstood it, thanks.
Comment #3.0
sunRewrite.
Comment #4
katbailey CreditAttribution: katbailey commentedI guess my original summary was a little rambling :-P Thanks for restructuring it - I just made a few corrections here and there.
Comment #5
plachJust to clarify: are you suggesting that if the
LanguageManager
has to initialize a language type without having a request object set, the request-dependent language detection methods (URL, Browser, Session) would just bail out and returnFALSE
, leaving the language negotiation process proceed happily?Comment #6
katbailey CreditAttribution: katbailey commented@plach with this patch the language negotiation methods will still only get called when there is a request, it's just that the logic about whether to call them gets moved out of the language() function in bootstrap.inc and into the LanguageManager itself. If there are language negotiation methods that are not dependent on the request then we can rework the logic of that in the other issue once this first step has been completed.
Comment #7
Crell CreditAttribution: Crell commentedI am no expert on the language system, but in my brief read through this seems overall sane, at least enough to unblock the two related issues.
Comment #8
Gábor HojtsyThere is probably only one negotiation method in core that has internal configuration and is independent of the request (the "Selected language" method, which returns a pre-selected language set up by the site admin). So the negotiation process as a whole cannot really be trusted to return the right value if there is no information about the request.
Comment #9
katbailey CreditAttribution: katbailey commentedMaking very slow progress on these test failures :-/ Still have no idea why the simpletest test is exploding - @sun, if you get a chance to look at that, it might be something really obvious to you.
It looks like we don't need to pass config to the language manager at all (I think the config reset() call was only needed in drupal_language_initialize() because of the hook_language_init invocation, and the only implementation of that was locale.module using it to add a config event subscriber... I converted that to a bundle), so that's good at least, unless testbot proves otherwise.
Comment #11
katbailey CreditAttribution: katbailey commentedOh yay - the simpletest test got fixed by one of the other fixes - progress not as slow as I had thought :-)
I should explain the fix I did for the locale config override test though as it looks like a total cheat (just remove the failing assertion - done! :-D)
The patch removes this hook_language_init implementation from locale.module (and in fact gets rid of hook_language_init entirely as this was the only implementation in core):
... and adds instead a LocaleBundle where the event subscriber gets registered. This means we don't need to explicitly call drupal_language_initialize() for the override to take effect. The assertion I removed was just checking the pre-overridden config setting, then calling drupal_language_initialize() and checking the overridden value. I think all this test needs to care about is that final assertion, so I removed the rest.
Does anyone see any problem with that?
Comment #12
katbailey CreditAttribution: katbailey commentedThis hopefully gets us green. The fix for the DateUpgradePath test probably stands in need in an explanation: basically, because the locale config override now actually works, i.e. without having to explicitly call drupal_language_initalize(), the 'en' config is used when the formats are looked up. So I'm changing the test to expect the 'en' formats, and there's no need to include 'en' later on so we just check the 'de' formats.
Comment #13
plachSounds great :)
Will have a look to this ASAP.
Comment #14
plachOverall this looks great to me. Probably we should ask @Jose Reyero to have a look to the config overrides stuff.
Can we have an
isMultilingual()
method instead? We usually have no interest in knowing how many languages are installed.Missing @params/@returns.
I'm not sure the two variables below are state: it's true that they can be derived, but it's also true that they aren't derived automatically. Hence they won't be updated by a config deployment.
Why
$default = TRUE
is not part of the array returned bygetLanguageDefault
? This is not consistent with whatlanguage_default()
returns. I guess we should just return aLanguage
object btw.What about lazy-initializing
$this->langcode
instead of repeating twice all the langcode resolution code below? Do we expect the value returned by the language manager to change?PHP docs need to be updated.
Shouldn't we call
LanguageManager::init()
here?Why is this needed?
Typo
Comment #15
katbailey CreditAttribution: katbailey commentedThanks for the review, @plach! I think I've dealt with everything you brought up, except for:
Yes, the language manager could get its $request property set on it after the first use of the alias manager.
Re the user roles stuff in UpgradePathTestBase - dammit, I was hoping nobody would ask about that one :-P Let's see if I can come up with a coherent explanation. The global user normally has the roles property set, and this matters for things like the LocaleLookup class, which uses the current user's role IDs as part of the cache key. Without this patch, the particular code path that leads to this roles-property-less user object being erroneously assumed to have a roles property by the LocaleLookup class simply never happens - and that's just an accident. Because we're slightly changing the code flow for language initialization, we expose this problem with the test, so I'm fixing it by adding the expected roles property.
Comment #17
plach#15: 1899994.disentangle-language-init.15.patch queued for re-testing.
Comment #19
plach#15: 1899994.disentangle-language-init.15.patch queued for re-testing.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commented#15: 1899994.disentangle-language-init.15.patch queued for re-testing.
Comment #22
plachRerolled to fix the minor thing below:
Comment not wrapping at column 80.
First of all I should say this looks ready to go to me. I was just wondering whether this check is actually needed: if we removed it we would have language negotiation for each subrequest, correct? If it's not a hard thing to achieve it would be a really nice feature as it would allow us to have a different language for each block. This would be really useful for instance to have the administrative toolbar always being displayed in the admin language instead of following the UI language which a site builder might not know.
This can totally be a follow-up, obviously :)
Comment #23
plachBtw, it seems Jose won't be available to review this for a while, hence I went ahead and manually tried the changes to locale config overrides and everything was looking good. Very nice work!
Comment #24
YesCT CreditAttribution: YesCT commentedJust a coding style review. Some of these are just nits, (patch fixes them). Went ahead and did it since there were at least a few @param 's that would have to have been fixed anyway to meet the core gate of having types. http://drupal.org/core-gates#documentation-block-requirements
This was over 80 chars, and needs a comma before the for example (e.g.) anyway.
Needs to be third person verb.
http://drupal.org/node/1354#classes
Contains \Drupal
http://drupal.org/node/1354#namespaces
Needs to start with third person verb.
Should end with a period.
classes have a empty line before the closing brace.
http://drupal.org/node/1354#classes
If going to change to Contains, might as well change to Contains \Dr..
type key word is bool
http://drupal.org/node/1354#functions
was the @var left out on purpose? I'm guessing not. so adding in.
Reads strange with the "to do load an"
add (optional) and |null in the type.
http://drupal.org/node/1354#functions
bool keyword instead of boolean
needs type and (optional)
I had a hard time finding the type.
Q1
Is it:
\Symfony\Component\HttpFoundation\Request
?
grammar of using i.e. (in other words) is a bit confusing, but I think it would be i.e.,
http://grammar.quickanddirtytips.com/ie-eg-oh-my.aspx
This is a bit strange to see a name space so simple.
Should be Contains \Drupal\locale\LocaleBundle.
(and with a period)
Registers ...
Patch coming right up.
Comment #25
YesCT CreditAttribution: YesCT commentedComment #26
YesCT CreditAttribution: YesCT commentedthere was no change to the function definition. But I noticed that the request parameter is optional. So it could be a separate issue to correct the @param's for language_from_url().
Comment #28
plach#25: 1899994.disentangle-language-init.25.patch queued for re-testing.
Comment #29
plachMinor nits on the nits, to be fixed on the next reroll.
Typo
Let's use a "use" statement, please.
This does not seems correct to me, at least in italian it wouldn't ;)
Comment #30
YesCT CreditAttribution: YesCT commentedOops!
In the improve comment on entity ng issue, it was recommended to just say "for example" instead of eg. We could do that.
How about (wrapped at 80 chars):
+ // Store the correct system path, in other words, the path minus the path prefix.
Comment #31
katbailey CreditAttribution: katbailey commentedThe problem is we'd have no way to set the request back at the end of a subrequest because there's no event we can listen to for that. I brought this up over in #1862202-36: Objectify the language system. Apparently fabpot is working on a potential solution: https://github.com/symfony/symfony/issues/5300
Re the $request param in
language_from_url()
, I've made it optional as well, just for consistency - although the fact is this function will never get called without a request (same goes forlanguage_from_user_admin()
). So I was nearly going to make it non-optional for both of them, but then it seemed that would be inconsistent with what happens inlanguage_negotiation_method_invoke()
- that's where these functions get called from and *its* request param is optional. And I don't want to go messing with that code in this patch - we'll figure it out in #1862202: Objectify the language system.As for the use of "i.e." it is perfectly correct here, but this comment is part of the temporary hack that we're removing in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence so it's not worth worrying about anyway.
Fixed the other things mentioned in #29.
Comment #32
plachSorry, I missed that one. Aside from the minor things below, which I fixed myself, this looks awesome, thanks!
Unused "use" statement (we can add it back when injecting configuration in the other issue).
Typo.
Missing "Defaults to".
Missing @param.
Comment #33
YesCT CreditAttribution: YesCT commentedinterdiff looks good.
Comment #34
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #35
plachSmall follow-up: subrequests are now broken when using URL language negotiation.
Comment #37
plachComment #38
Gábor HojtsySo in the patch that was committed, this is what seems to have been targeted to avoid this problem:
Is that not working (at the wrong place, should be removed?) or not enough? Adding comments there and in your added code would be useful to explain the situation.
Comment #39
plachThe reason is this one:
We basically need the same check also when updating the request path after removing prefixes, otherwise subrequests will inherit the main request path and an infinite loop will be started. Will add a comment about this ASAP.
Anyway, this should be lo longer needed once #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence is committed.That one will only let us split the prefix out in a cleaner way.Comment #40
katbailey CreditAttribution: katbailey commentedYes, good catch. The fix looks fine to me, just noticed one small thing in the test:
This is not used.
Comment #41
plachAnd here it is.
Shameless copy paste gone wrong ;)
Comment #42
katbailey CreditAttribution: katbailey commentedAssuming testbot approves, this is good to go.
Comment #44
plachComment #45
plachThis issue removed
hook_language_init()
.Comment #46
Gábor Hojtsy+1 to RTBC :)
Comment #47
catch#41: 1899994.disentangle-language-init.42.patch queued for re-testing.
Comment #48
plachRerolled + run tests locally.
Comment #49
catchThanks for the re-roll, committed/pushed to 8.x.
Comment #50
plachThanks!
Comment #51
plachComment #52
plachHere is a change record: http://drupal.org/node/1911596.
Comment #53
Gábor Hojtsy@plach: any way to hook into after the language process in Drupal 8 then? The changelog only says this was removed, period not what people can/should use instead.
Comment #54
plachI don't think there was a real use case for that, aside from variable localization. Moreover now language is initialized as soon as it is needed so there is no bootstrap phase to hook into.
Comment #55
Gábor HojtsyOk, added this note: "There is no facility anymore to hook into the end of language initialisation.".
Comment #56
Gábor HojtsyLooks like as far as languages go, we should go back to #1862202: Objectify the language system on this track unearthing @chx's initial patch and going from there :)
Comment #57
plachWith the change notice in place we can call this fixed.
Comment #58.0
(not verified) CreditAttribution: commentedSome minor corrections.