Added support for get culture information from request's route data i… #288

kichalla · 2016-09-25T00:57:52Z

…nformation.

[Fixes #122] Get segment from mapped route for CustomRequestCultureProvider

…nformation. [Fixes #122] Get segment from mapped route for CustomRequestCultureProvider

hishamco · 2016-09-25T02:49:50Z

Great, looks I need to close my issue #282 because I was planning to make a PR for this, unfortunately no one respond to that issue 😞

hishamco · 2016-09-25T02:53:16Z

src/Microsoft.AspNetCore.Localization.Routing/Microsoft.AspNetCore.Localization.Routing.xproj

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="utf-8"?>


Why you're creating a new project for Routing? It will be nice to follow the structure that's already used in the repo. I think adding RouteDataRequestCultureProvider.cs is appropriate choice

We want to avoid dependency of Routing in core localization package as it does not depend on routing.

Valid point

hishamco · 2016-09-25T02:55:04Z

....AspNetCore.Localization.Routing.Tests/Microsoft.AspNetCore.Localization.Routing.Tests.xproj

@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="utf-8"?>


Same thing here add we can adds the test to the Microsoft.AspNetCore.Localization.Tests project

hishamco · 2016-09-25T03:06:37Z

Can we add this to the https://github.com/aspnet/Localization/tree/dev/test/LocalizationWebsite for functional test, because what you did in the unit tests that you wrote is to configure the localization options inside app.UseRouter, practically we need to configure it before app.UseMvc, and this will lead to the issue that described here aspnet/Mvc#4952 HttpContext.GetRouteData() always will return null because the routing is not injected yet

hishamco · 2016-09-25T03:08:28Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                                    new CultureInfo("ar-YE")
+                                }
+                            };
+                            options.RequestCultureProviders = new[]


We can replace it with

options.RequestCultureProviders.Insert(0, new RouteDataRequestCultureProvider() { Options = options, RouteDataStringKey = "c", UIRouteDataStringKey = "uic" });

hishamco · 2016-09-25T03:10:18Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                                    new CultureInfo("ar-YE")
+                                }
+                            };
+                            options.RequestCultureProviders = new[]


Simplify this with

options.RequestCultureProviders.Insert(0, new RouteDataRequestCultureProvider() { Options = options });

hishamco · 2016-09-25T03:11:31Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                    {
+                        DefaultRequestCulture = new RequestCulture("en-US")
+                    };
+                    options.RequestCultureProviders = new[]


Same as my previous suggestion

hishamco · 2016-09-25T03:14:13Z

It's also possible to register the newly RequestCultureProvider here https://github.com/aspnet/Localization/blob/dev/src/Microsoft.AspNetCore.Localization/RequestLocalizationOptions.cs#L26 instead of adding it every time while we configure the options

hishamco · 2016-09-25T03:17:29Z

src/Microsoft.AspNetCore.Localization.Routing/RouteDataRequestCultureProvider.cs

+
+            if (!string.IsNullOrWhiteSpace(RouteDataStringKey))
+            {
+                culture = httpContext.GetRouteValue(RouteDataStringKey) as string;


Can we use ToString() here?

@hishamco good idea. @kichalla why didn't you make this change and do the same thing for uiCulture?

hishamco · 2016-09-25T03:18:54Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                                    new CultureInfo("ar-YE")
+                                }
+                            };
+                            options.RequestCultureProviders = new[]


Same as previous suggestion

hishamco · 2016-09-25T03:19:43Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+
+                Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+                var data = await response.Content.ReadAsStringAsync();
+                Assert.Equal($"{expectedCulture},{expectedUICulture}", data);


Pleas add new line here

hishamco · 2016-09-25T03:20:07Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                            {
+                                var requestCultureFeature = context.Features.Get<IRequestCultureFeature>();
+                                var requestCulture = requestCultureFeature.RequestCulture;
+                                return context.Response.WriteAsync(


Please add new line here

hishamco · 2016-09-25T03:20:28Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                                    new CultureInfo("ar-YE")
+                                }
+                            };
+                            options.RequestCultureProviders = new[]


Please add new line here

hishamco · 2016-09-25T03:20:40Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+
+                Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+                var data = await response.Content.ReadAsStringAsync();
+                Assert.Equal("en-US,en-US", data);


Please add new line here

hishamco · 2016-09-25T03:20:53Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                    {
+                        DefaultRequestCulture = new RequestCulture("en-US")
+                    };
+                    options.RequestCultureProviders = new[]


Please add new line here

hishamco · 2016-09-25T03:21:01Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+
+                Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+                var data = await response.Content.ReadAsStringAsync();
+                Assert.Equal($"{expectedCulture},{expectedUICulture}", data);


Please add new line here

hishamco · 2016-09-25T03:21:07Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                            {
+                                var requestCultureFeature = context.Features.Get<IRequestCultureFeature>();
+                                var requestCulture = requestCultureFeature.RequestCulture;
+                                return context.Response.WriteAsync(


Please add new line here

hishamco · 2016-09-25T03:21:26Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                                    new CultureInfo("ar-YE")
+                                }
+                            };
+                            options.RequestCultureProviders = new[]


Please add new line here

hishamco · 2016-09-25T03:21:53Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+using Microsoft.AspNetCore.TestHost;
+using Microsoft.Extensions.DependencyInjection;
+using Xunit;
+using System.Net;


Please sort the namesapces

dougbu · 2016-09-26T02:27:47Z

src/Microsoft.AspNetCore.Localization.Routing/RouteDataRequestCultureProvider.cs

+            if (culture == null && uiCulture == null)
+            {
+                // No values specified for either so no match
+                return Task.FromResult((ProviderCultureResult)null);


Use Microsoft.Extensions.TaskCache.Sources to avoid creating this Task in every request.

dougbu · 2016-09-26T02:29:16Z

src/Microsoft.AspNetCore.Localization.Routing/RouteDataRequestCultureProvider.cs

+        /// <see cref="RouteDataStringKey"/> will be used.
+        /// Defaults to "ui-culture".
+        /// </summary>
+        public string UIRouteDataStringKey { get; set; } = "ui-culture";


Use a new options class.

Experience here is in line with existing culture providers. So I do not want to do anything different with this new provider.

kichalla · 2016-09-26T11:20:41Z

It's also possible to register the newly RequestCultureProvider here https://github.com/aspnet/Localization/blob/dev/src/Microsoft.AspNetCore.Localization/RequestLocalizationOptions.cs#L26 instead of adding it every time while we configure the options

Adding the new provider in the linked location would cause that package to indirectly depend on Routing which we want to avoid.

kichalla · 2016-09-26T11:25:44Z

Can we add this to the https://github.com/aspnet/Localization/tree/dev/test/LocalizationWebsite for functional test, because what you did in the unit tests that you wrote is to configure the localization options inside app.UseRouter, practically we need to configure it before app.UseMvc, and this will lead to the issue that described here aspnet/Mvc#4952 HttpContext.GetRouteData() always will return null because the routing is not injected yet

If you take a look at the website, it does not depend on MVC (this is on purpose as in our build process we do not want to create a circular dependency between repositories). For the scenario you mentioned, I am planning to update an existing test in MVC which tests the scenario you mentioned: https://github.com/aspnet/Mvc/blob/dev/test/WebSites/FiltersWebSite/LocalizationPipeline.cs#L27

kichalla · 2016-09-26T11:29:02Z

🆙 📅

dougbu · 2016-09-26T16:33:18Z

src/Microsoft.AspNetCore.Localization.Routing/RouteDataRequestCultureProvider.cs

+
+            if (!string.IsNullOrWhiteSpace(RouteDataStringKey))
+            {
+                culture = httpContext.GetRouteValue(RouteDataStringKey) as string;


@hishamco good idea. @kichalla why didn't you make this change and do the same thing for uiCulture?

dougbu · 2016-09-26T16:35:11Z

src/Microsoft.AspNetCore.Localization.Routing/project.json

+  "dependencies": {
+    "Microsoft.AspNetCore.Localization": "1.1.0-*",
+    "Microsoft.AspNetCore.Routing": "1.1.0-*",
+    "Microsoft.Extensions.TaskCache.Sources": "1.1.0-*"


This must be a "type": "build" dependency.

dougbu · 2016-09-26T16:36:45Z

src/Microsoft.AspNetCore.Localization/project.json

@@ -23,7 +23,8 @@
    "Microsoft.AspNetCore.Http.Extensions": "1.1.0-*",
    "Microsoft.Extensions.Globalization.CultureInfoCache": "1.1.0-*",
    "Microsoft.Extensions.Localization.Abstractions": "1.1.0-*",
-    "Microsoft.Extensions.Options": "1.1.0-*"
+    "Microsoft.Extensions.Options": "1.1.0-*",
+    "Microsoft.Extensions.TaskCache.Sources": "1.1.0-*"


"type": "build"

Eilon · 2016-09-26T16:46:16Z

src/Microsoft.AspNetCore.Localization.Routing/RouteDataRequestCultureProvider.cs

+            string culture = null;
+            string uiCulture = null;
+
+            if (!string.IsNullOrWhiteSpace(RouteDataStringKey))


Don't check white space.

No description provided.

kichalla · 2016-09-26T17:40:12Z

🆙 📅

hishamco · 2016-09-26T20:10:31Z

src/Microsoft.AspNetCore.Localization/AcceptLanguageHeaderRequestCultureProvider.cs

@@ -33,7 +34,7 @@ public override Task<ProviderCultureResult> DetermineProviderCultureResult(HttpC

            if (acceptLanguageHeader == null || acceptLanguageHeader.Count == 0)
            {
-                return Task.FromResult((ProviderCultureResult)null);
+                return TaskCache<ProviderCultureResult>.DefaultCompletedTask;


I'm not sure if this change applicable in this PR, or may we need another PR, while this one specially for adding RouteDataCulureRequestProvider

@hishamco this is fine. Good in fact. We were cleaning up cacheable Tasks and @kichalla got ahead of the curve.

dougbu · 2016-09-26T22:27:31Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                {
+                    app.UseRouter(routes =>
+                    {
+                        routes.MapRoute(routeTemplate, (IApplicationBuilder fork) =>


You can subApp instead

We do call this a forked pipeline. But subApp is also commonly used.

dougbu · 2016-09-26T22:27:59Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                        var requestCultureFeature = context.Features.Get<IRequestCultureFeature>();
+                        var requestCulture = requestCultureFeature.RequestCulture;
+                        return context.Response.WriteAsync(
+                                    $"{requestCulture.Culture.Name},{requestCulture.UICulture.Name}");


nit: lessen the indenting

dougbu · 2016-09-26T22:28:23Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+                {
+                    app.UseRouter(routes =>
+                    {
+                        routes.MapRoute(routeTemplate, (IApplicationBuilder fork) =>


dougbu · 2016-09-26T22:29:47Z

test/Microsoft.AspNetCore.Localization.Routing.Tests/RouteDataRequestCultureProviderTest.cs

+
+namespace Microsoft.AspNetCore.Localization.Routing
+{
+    public class RouteDataRequestCultureProviderTest


The 3 tests in this class seem to share a lot of boilerplate. Can any of that be moved into helper methods to reduce the repetition and make the differences between the setups more obvious?

kichalla · 2016-09-30T17:02:50Z

861ae52

Added support for get culture information from request's route data i…

41f6fee

…nformation. [Fixes #122] Get segment from mapped route for CustomRequestCultureProvider

dnfclas added the cla-already-signed label Sep 25, 2016

hishamco reviewed Sep 25, 2016

View reviewed changes

dougbu reviewed Sep 26, 2016

View reviewed changes

Addressed feedback

8d70836

dougbu reviewed Sep 26, 2016

View reviewed changes

Eilon suggested changes Sep 26, 2016

View reviewed changes

Updated

0da2e8b

hishamco reviewed Sep 26, 2016

View reviewed changes

Eilon approved these changes Sep 26, 2016

View reviewed changes

dougbu approved these changes Sep 26, 2016

View reviewed changes

kichalla closed this Sep 30, 2016

kichalla deleted the kichalla/routedata-cultureprovider branch January 30, 2017 19:57

Added support for get culture information from request's route data i… #288

Added support for get culture information from request's route data i… #288

Uh oh!

Conversation

kichalla commented Sep 25, 2016

Uh oh!

hishamco commented Sep 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hishamco commented Sep 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hishamco commented Sep 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kichalla commented Sep 26, 2016

Uh oh!

kichalla commented Sep 26, 2016

Uh oh!

kichalla commented Sep 26, 2016

Uh oh!

dougbu Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kichalla commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu Sep 26, 2016 •

edited

Loading

dougbu Sep 26, 2016 •

edited

Loading