Skip to content
This repository was archived by the owner on Nov 3, 2022. It is now read-only.

Is this ready for production? #19

Closed
JobaDiniz opened this issue Sep 10, 2014 · 43 comments
Closed

Is this ready for production? #19

JobaDiniz opened this issue Sep 10, 2014 · 43 comments

Comments

@JobaDiniz
Copy link

I see that the nuget is tagged with pre-release.

Can I use this package? Anyone is using it?

@Dresel
Copy link
Owner

Dresel commented Sep 11, 2014

I'm successfully using it for some smaller MVC projects, but I might not have covered every use case someone might have. The documentation lacks a bit of information but otherwise I see no reason why not to evaluate it if this package fits your need.

I'm still waiting for the feedback of one of the main participants of the previous version (unfortunately he didn't respond for 2 months), which I wanted to integrate (+ more documentation) before I change the package to release.

If you are giving it a try, let me know the version you are using (MVC / WebAPI) and your use cases and feel free to ask any questions about the package.

@JobaDiniz
Copy link
Author

Well, I tried yesterday and couldn't get it to work, so I dropped it, because I have tight schedule.
I was using the latest asp.net MVC and attribute routing.
I configured all things: required and the optionals.

Added translations only for 1 route (to test), but when I typed /en-US/datasource, it gave 404 page.

I think this is a promising library, and maybe I'll use it later in the project.

@mdmoura
Copy link

mdmoura commented Sep 11, 2014

@Dresel
Sorry sorry for not providing feedback ... My fault ...
I was abroad and when I arrived I had my head under a lot of work.

@JobaDiniz
I have been using it on a web application for 2 months and it has been working fine.
I am developing a new Startup to be launched in December and my intention is to include Route Localization. I am just following its behavior on the website that is now live.

@Dresel
I just created a project which I pushed to Github. Is based on default ASP.NET MVC template.
The project https://github.com/mdmoura/RouteLocalization.Test addresses issues: #2 and #16

I will answer in those issues ...

I will use the test application I mentioned to keep testing RouteLocalization.

@JobaDiniz
Copy link
Author

I'd also like to know how well this library is integrated with https://github.com/maartenba/MvcSiteMapProvider which I extensily use in my projects.

@mdmoura
Copy link

mdmoura commented Sep 11, 2014

@JobaDiniz

I have tried MvcSiteMapProvider a long time ago but I found it to restrictive in a lot of situations. I am afraid I am not able to help you on this ...

@JobaDiniz
Copy link
Author

"Restrictive"? I think it is an awesome library that handles so much about menu, breadcrumbs and SEO. I can customize so many things.

@mdmoura
Copy link

mdmoura commented Sep 11, 2014

I didn't say it wasn't a good library ... It just said that when I tried I felt it was a restriction for what I needed on the projects I was working on. But I checked their Github's repository and it evolved a lot since the last time I used so ... Did you already tried it with Route Localization?

RouteLocalization replaces, or adds, routes to the route table according to localization setup.

@JobaDiniz
Copy link
Author

I didn't have time to test.
I'd like to have these sets of routes:

/{culture}/{controller}/{action}

If user types /en-US/datasource or /pt-BR/fonte-de-dados it will match.
If user types /datasource or /fonte-de-dadosit won't match.
However, if user types the site root / it will match root route. /en-US/ or /pt-BR/ will also match root route.

I find it hard to configure such things with your library. Maybe some use-cases in the wiki pages would be helpfull for early adopters.

@mdmoura
Copy link

mdmoura commented Sep 11, 2014

On the route configuration of your MVC application try the following configuration:

public class RouteConfig {

public static void RegisterRoutes(RouteCollection routes) {

  routes.IgnoreRoute("{resource}.axd/{*pathInfo}");

  routes.MapMvcAttributeRoutes(Localization.LocalizationDirectRouteProvider);

  // I am assuming that your default culture is pt-BR
  Localization localization = routes.Localization(x => {
    x.DefaultCulture = "pt-BR";
    x.AcceptedCultures = new HashSet<string>() { "pt-BR", "en-US" };
    x.AttributeRouteProcessing = RouteLocalization.Mvc.Setup.AttributeRouteProcessing.AddAsDefaultCultureRoute;
    x.AddCultureAsRoutePrefix = true;
    x.AddTranslationToSimiliarUrls = true;
  });

  localization.TranslateInitialAttributeRoutes().Translate(x => {

    // I am assuming that on your controllers actions you set the attribute routes in portuguese and here is the translation for english
    x.ForCulture("en-US").ForNamedRoute("home.index").AddTranslation("home");
  });

  // Instead of return new CultureInfo("pt-BR") you can detect the culture from browser, cookie, requested domain, etc. 
  CultureSensitiveHttpModule.GetCultureFromHttpContextDelegate = context => { return new CultureInfo("pt-BR"); };

  // Here I create a start route that redirects for localized home   
  RouteTable.Routes.MapRoute("Start", String.Empty, new { controller = "Home", action = "Start" });

  // Missing routes
  RouteTable.Routes.MapRoute("404", "{*url}", new { controller = "Error", action = "NotFound_" });

  // Register areas
  AreaRegistration.RegisterAllAreas();

Finally on home controller add the following:

[Route("inicio", Name = "home.index"), HttpGet]
public virtual ActionResult Index() {
  return View();
}

[HttpGet]
public virtual ActionResult Start() {
  return RedirectToAction(MVC.Home.Index());
} // Start

Let me know how it went.

@JobaDiniz
Copy link
Author

I couldn't get it to work:

public static void RegisterRoutes(RouteCollection routes, ICultureRepository cultureRepository)
        {
            routes.IgnoreRoute("{resource}.axd/{*pathInfo}");
            routes.MapMvcAttributeRoutes(Localization.LocalizationDirectRouteProvider);

            var cultures = cultureRepository.GetActiveCultures().ToList();

            var localization = routes.Localization(x =>
            {
                x.DefaultCulture = "pt-BR";
                x.AcceptedCultures = new HashSet<string>(cultures.Select(c => c.Language));
                x.AttributeRouteProcessing = AttributeRouteProcessing.AddAsDefaultCultureRoute;
                x.AddCultureAsRoutePrefix = true;
                x.AddTranslationToSimiliarUrls = true;
            });

            localization.TranslateInitialAttributeRoutes().Translate(x =>
            {

                // I am assuming that on your controllers actions you set the attribute routes in portuguese and here is the translation for english
               x.ForCulture("en-US")
                    .ForController<DatasourceController>()
                    .SetRoutePrefix("datasource")
                    .ForAction(a => a.Create())
                    .AddTranslation("create")
                    .ForAction("Edit")
                    .AddTranslation("edit/{id}");
            });

            // Instead of return new CultureInfo("pt-BR") you can detect the culture from browser, cookie, requested domain, etc. 
            CultureSensitiveHttpModule.GetCultureFromHttpContextDelegate =
    Localization.DetectCultureFromBrowserUserLanguages(new HashSet<string>(cultures.Select(c => c.Language)), "pt-BR");

            var localizationCollectionRoutes =
    Localization.LocalizationDirectRouteProvider.LocalizationCollectionRoutes.Select(
        x => (LocalizationCollectionRoute)x.Route).ToList();

            GlobalFilters.Filters.Add(new CultureSensitiveActionFilterAttribute(localizationCollectionRoutes));
[RoutePrefix("fonte-de-dados")]
    public class DatasourceController : Controller
    {
[Route]
        public ActionResult Index()
        {
            @ViewBag.Title = "Fontes de Dados";
            return View(items);
        }

        [Route("cadastrar")]
        public ActionResult Create()
        {
            return View();
        }

        [Route("editar/{id:int}")]
        [SiteMapTitle("Name")]
        public ActionResult Edit(int id)
        {
            return View(items.FirstOrDefault(i => i.Id == id));
        }

Tried: http://localhost:39231/pt-BR/fonte-de-dados (OK)
Tried: http://localhost:39231/en-US/datasource (404 not found)

@mdmoura
Copy link

mdmoura commented Sep 11, 2014

You aren't setting the translation for action Index in DatasourceController.

BTW, the action were marked as virtual because I am using T4MVC which makes the actions virtual so they can be overridden. Nothing to do with RouteLocalization.

@JobaDiniz
Copy link
Author

There is no translation for Index action... It is the root of Datasource:
pt-BR/fontr-de-dados/ and en-US/datasource/

There shouldnt be en-US/datasource/index

EDIT: okay, I added an empty translation and worked.

 x.ForCulture("en-US")
                    .ForController<DatasourceController>()
                    .SetRoutePrefix("datasource")
                    .ForAction(a=>a.Index())
                    .AddTranslation("")
                    .ForAction(a => a.Create())
                    .AddTranslation("create")
                    .ForAction("Edit")
                    .AddTranslation("edit/{id}");

@JobaDiniz
Copy link
Author

However, MvcSiteMapProvider isn't working at all.
http://localhost:39231/sitemap.xml returns:

<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
<url>
<loc>http://localhost:39231/</loc>
</url>
<url>
<loc>http://localhost:39231/#</loc>
</url>
</urlset>

@Dresel
Copy link
Owner

Dresel commented Sep 12, 2014

I think there are some capitalization issues - if you lowercase all your localization strings ("en-US" to "en-us") for example, does it work as expected (at least I get more routes returned then)?

I moved this to #20. This needs minor modifications on some string comparisions, will fix this later this weekend.

@mdmoura: Same goes for #2 / #16 - if I have time I will investigate them over the weekend.

@JobaDiniz
Copy link
Author

You're right. If i set all to lowercase, MvcSiteMapProvider works. Why is that? I'd like to be en-US and pt-BR

@Dresel
Copy link
Owner

Dresel commented Sep 12, 2014

The culture is used as key in RouteLocalization (to store the localized routes) and there are some string comparisions that must be fixed so that this will work. Then it will be possible to use en-US, etc.

@JobaDiniz
Copy link
Author

if (!acceptedCultures.Contains(userCultureInfo.Name.ToLower()))
                            {
                                continue;
                            }

                            // Culture found that is supported
                            cultureName = userCultureInfo.Name.ToLower();

This is the code that mistakenly compares using lowercase.

There is another code design that I want to point out: the HashSet. I find it odd to require a HashSet:

var localization = routes.Localization(x =>
            {
                x.DefaultCulture = "pt-BR";
//SEE WHAT I DID HERE?
                x.AcceptedCultures = new HashSet<string>(cultures.Select(c => c.Language)); 
                x.AttributeRouteProcessing = AttributeRouteProcessing.AddAsDefaultCultureRoute;
                x.AddCultureAsRoutePrefix = true;
                x.AddTranslationToSimiliarUrls = true;
            });

It would be much better to require only an IEnumerable.

@mdmoura
Copy link

mdmoura commented Sep 12, 2014

It would be much better to require only an IEnumerable.

Not really. Hashset does not allow duplicated elements, which makes sense in the case of accepted cultures, and the search for an element is faster.

@Dresel
Copy link
Owner

Dresel commented Sep 12, 2014

Yes this is one of the code fragments I meant - but not the only one. I think there are 3-4 code fragments that must be changed - in my oppionion it should be case-insensitive in the end.

Much better is a little subjective. I use the HashSet because the culture collection should be unique. I could expose IEnumerable as a public property and use the HashSet internally, but I guess this is just a question of personal flavour.

@JobaDiniz
Copy link
Author

Good for performance; got it.
What I meant is that I tend to expose the minimum required classes to users of my api. That's why I proposed to use IEnumerable, but now I understand that HashSet is required. Maybe instead of require a concrete implementation, maybe it could required the ISet interface.

@JobaDiniz
Copy link
Author

Hi, will you help me out here? I'm almost done with the initial configuration.
The problem now is configuring the following routes:

http://localhost:39231/en-us
http://localhost:39231/pt-br
http://localhost:39231/ (should match default pt-br culture)

The root route is Dashboard controller, Index method:

[RoutePrefix("painel")]
    public class DashboardController : Controller
    {
        public DashboardController(ICultureRepository cultureRepository)
        {

        }

        [Route("~/")]
        [Route]
        public ActionResult Index()
        {
            return View();
        }
    }

Translation:

 x.ForCulture("en-us")
                    .ForController<DashboardController>()
                    .SetRoutePrefix("dashboard")
                    .ForAction(a => a.Index())
                    .AddTranslation("");

Results:

http://localhost:39231/en-us -> not found 404
http://localhost:39231/pt-br -> ok
http://localhost:39231/ => not found 404

@Dresel
Copy link
Owner

Dresel commented Sep 13, 2014

If you register multiple routes on one action, RouteLocalization normally translates only the first route found ([Route] in your case), so you end up in

http://localhost/pt-br/painel (from [Route], created via TranslateInitialAttributeRoutes())
http://localhost/en-us/dashboard (from [Route], created via .AddTranslation(""))
http://localhost/pt-br/ (from [Route("~/")], created via TranslateInitialAttributeRoutes())

So to translate [Route("~/")] you have to call AddTranslation again (AddTranslationToSimiliarUrls will not work here, because the routes are different):

x.ForCulture("en-us")
    .ForController<DashboardController>()
    .ForAction(a => a.Index())
    .SetRoutePrefix("dashboard")
    .AddTranslation("");

x.ForCulture("en-us")
    .ForController<DashboardController>()
    .ForAction(a => a.Index())
    .AddTranslation("");

or

x.ForCulture("en-us")
    .ForController<DashboardController>()
    .ForAction(a => a.Index())
    .SetRoutePrefix("dashboard")
    .AddTranslation("")
    .SetRoutePrefix("")
    .AddTranslation("");

You could use named routes for those cases where multiple routes are declared for one route so it is easier to distinguish them. Now you have four routes:

http://localhost/pt-br/painel
http://localhost/en-us/dashboard
http://localhost/pt-br/
http://localhost/en-us/

If you want one route without RoutePrefix (http://localhost/), you could use the "old" way:

routes.MapRoute("EntryRoute", "", new { controller = "Datasource", action = "Entry" });

Note that this will only work to actions without an Route attribute on it. You could have one untranslated route and redirect to the translated one:

public ActionResult Start()
{
    return RedirectToAction("Index", new {culture = Thread.CurrentThread.CurrentCulture.Name.ToLower() });
}

Note that ToLower() can be removed after #20 is fixed. This will redirect to the translated Index route depending on the browser setting: when en-us is found to en-us, when pt-br is found to pt-br - otherwise to the default culture (pt-br).

@JobaDiniz
Copy link
Author

Thanks! It is working!

Awesome library. Keep up the good work.

@JobaDiniz
Copy link
Author

How do you think I can handle routes that users won't see (ajax requests)?
For example, this action on ApplicationController:

 public ActionResult CategoryByWorkgroup(int id)
        {
            var categories = categoryByWorkgroup.Where(c => c.EntityId == id);
            ViewBag.Title = categories.First().EntityName;
            return PartialView("_CategoryByWorkgroup", categories);
        }

What I did was add the following route:
routes.MapRoute("Default", "{controller}/{action}/{id}", new { id = UrlParameter.Optional });

Now I can make ajax requests, without passing culture, using RouteJS (https://github.com/Daniel15/RouteJs):

var modalUrl = Router.action("Application", "CategoryByWorkgroup", { id: id });
$.get(modalUrl).done(function(html){})

However, the Appliction/CategoryByWorkgroup returns html that isn't translated (I'm using Resource.resx in the html). I guess is because the Thread.Culture isn't being set, because I'm not using RouteLocalization for this particular route.

@Dresel
Copy link
Owner

Dresel commented Sep 13, 2014

The Thread.Culture will / can be set atm

a) By the GetCultureFromHttpContextDelegate - if you use Localization.DetectCultureFromBrowserUserLanguages the Thread Culture should at least be set to the default culture
b) After it can be overriden by the CultureSensitiveActionFilterAttribute if you are requesting a localized route

When requesting an unlocalized route you must somehow transfer / store the preferred localization.

One possibility I think of could be:

Set the current language in a cookie / session variable when a localized route is executed (you could do this in the CultureSensitiveActionFilterAttribute.CultureSelected Event). Use this setting over the browser setting in the GetCultureFromHttpContextDelegate (if cookie / session exists, use this culture - otherwise call the default DetectCultureFromBrowserUserLanguages delegate).

Does this help you?

@JobaDiniz
Copy link
Author

Nice, thanks. That worked (didn't know about CultureSelected event):

 CultureSensitiveHttpModule.GetCultureFromHttpContextDelegate =
                DetectCultureFromCookieOrBrowser(new HashSet<string>(cultures.Select(c => c.Language)), cultureRepository.Default.Language);

var cultureSensitiveFilter = new CultureSensitiveActionFilterAttribute(localizationCollectionRoutes);
            cultureSensitiveFilter.CultureSelected += CultureSelected;
private static Func<HttpContext, CultureInfo> DetectCultureFromCookieOrBrowser(HashSet<string> acceptedCultures, string defaultCulture)
        {
            return (httpContext) =>
            {
                var cookie = httpContext.Request.Cookies["_culture"];
                if (cookie != null)
                    return new CultureInfo(cookie.Value);

                return Localization.DetectCultureFromBrowserUserLanguages(acceptedCultures,defaultCulture).Invoke(httpContext);
            };
        }
private static void CultureSelected(object sender, CultureSelectedEventArgs e)
        {
            var cookie = new HttpCookie("_culture", e.SelectedCulture);
            HttpContext.Current.Response.Cookies.Add(cookie);
        }

What I didn't like thou, is that I had to use HttpContext.Current static method. That's really ugly in MVC.
The CultureSelected event could also pass the HttpContext as an event arg. I saw that you have httpContext instance in the code. :)

@Dresel
Copy link
Owner

Dresel commented Sep 13, 2014

You're right - I will patch that with the next commit.

@Dresel
Copy link
Owner

Dresel commented Sep 15, 2014

I just uploaded a new Nuget-Package:

Fixed the String.Comparison Issues
Changed HashSet to ISet
CultureSelectedEventArgs now have an Context Property (HttpContextBase for MVC, HttpRequestContext for WebAPI)

I also removed some old code, the CultureSensitiveActionFilterAttribute can now be created without passing the localization routes.

Please test if this solve your issues.

@JobaDiniz
Copy link
Author

Thanks, it is working.

I want to congratulate the contributors, because this project it is worth something. The fact that there isn't many watchers/stared/forks is somewhat weird.

I want to contribute to the project. I think there are some issues in the API you've built, for example, static methods. I also think there must be releases/tags, that is, a branching model structure, unit tests and even CI builds (http://www.appveyor.com/ is free for open source projects).

This project can grow.
Don't know what are your expectations about this project, but I really liked it. Is there a roadmap, an idea where you want it to go?

@Dresel
Copy link
Owner

Dresel commented Sep 16, 2014

Well every contribution is welcome, so feel free to add discussions, suggestions, issues or even pull requests.

For Releases / Tags I could use the same as the Nuget version, guess that would make sense. For branching I just used "experimental" when I tried new things out but there was no need for a more granular structure, since I was the only person developing for it. Same goes for CI. Unit tests are already in the project, so I use them locally before commiting a new version.

To say something about my initial intention: Historically this project was created because of AttributeRouting project being merged into the Asp.Net Web Stack (5.0), but they left out the localization part (http://attributerouting.net/#localizing-urls). As far as I know the AttributeRouting project is now dead - the last commit was over a year ago. RouteLocalization was therefore a lightweight extension for Asp.Net Mvc to support the localization features that already existed. In 5.1 they changed the attribute routing and until 5.2 it wasn't possible to modify routes, so RouteLocalization didn't work anymore. For 5.2 with the introduction of the DirectRouteProvider mechanism it was possible again and RouteLocalization was completely rewritten to support Mvc and Web.Api. Since then (4 months ago) the project was in the alpha stage because I wanted to integrate the feedback. I think you can do everything now you could do with the localization support of the former AttributeRouting project - and a little bit more :)

My "plans" for the near future were

  • Change project / nuget package to release (the old issues are closed so I guess it is about time)
  • Add more (rework) documentation / samples

Features were integrated on demand, so there is no bigger goal out there yet. I think the biggest issue now is the sparse documentation. Guess if its more easier / clear how to use it, more use cases and future features will derive.

What are your suggestions for the future directions of this project?

@JobaDiniz
Copy link
Author

Once in a long time ago I used AttributeRouting library (before MS merged into the asp.net) and it was awesome indeed. I didn't know they support route localization, nice.
Early this year, I started a project with MVC5 only to realize that MS didn't use all of the features. I even opened a issue asking that (mccalltd/AttributeRouting#283). Poor MS....

@mdmoura
Copy link

mdmoura commented Sep 16, 2014

Similar experience here ...

I started using Attribute Routing on an early stage of the project.
In fact the localization features resulted from a discussion between me and Tim McCall.

I also was surprised that localization was not included in MS AR and found RouteLocalization.

Unfortunately, I rarely find the time to go deeply in the code part ...
... but I try to provide as much feedback and suggestions as I can.

At the moment I have no suggestion about new features for RouteLocalization ...

In my opinion it covers most of the situations and it works fine ...
The documentation might be confusing for someone starting with it.

In the next few months I will integrate Route Localization in a new startup project.
I believe it will be a good test to detect any bug that still might exist ...

@brgrz
Copy link

brgrz commented Sep 17, 2014

I just started using it, after initial problems I made some progress and I'm now able to access my site through various localized routes, still some issues though with root routes and some specifics with regards to classic vs attribute routes (I'm actually migrating from classic routes to attribute routes and introducing localized routes at the same time). It seems to me this project has future in my projects. ;)

@brgrz
Copy link

brgrz commented Sep 17, 2014

btw i'm testing with RouteDebugger and I don't set those localized routes there in the Routes table but they seem to work anyhow, what is going on?

@mdmoura
Copy link

mdmoura commented Sep 17, 2014

I usual use Glimpse. Give it a try ...

@Dresel
Copy link
Owner

Dresel commented Sep 17, 2014

Glimpse is great, but won't help there either because RouteLocalization uses a LocalizationCollectionRoute as proxy route which contains all translations for one route.

Those Route "Debuggers" just print the property Url which is the url taken from the attribute route. They do not know (yet? :)) that there are multiple routes behind a LocalizationCollectionRoute.

@Dresel
Copy link
Owner

Dresel commented Sep 17, 2014

For a quick and dirty overview of your localized routes you could do something like this:

public virtual ActionResult PrintRoutes()
{
    List<string> routes = new List<string>();

    foreach (LocalizationCollectionRoute route in Localization.LocalizationDirectRouteProvider.LocalizationCollectionRoutes.Select(x => x.Route))
    {
        routes.AddRange(
            route.LocalizedRoutes.Select(
                localizedRoute => string.Format("{0} ({1})", localizedRoute.LocalizedUrl, localizedRoute.Culture)));
    }

    return Content(string.Join("<br />", routes));
}

@Dresel
Copy link
Owner

Dresel commented Sep 23, 2014

I updated the documentation - added more content and tried to structure it a little bit. If you have ideas & suggestions for more improvement let me know - you know better what a first-time user of this API might need to know or what he might find hard to understand.

@Dresel
Copy link
Owner

Dresel commented Sep 23, 2014

I have created a RouteLocalization Tab for Glimpse: http://www.nuget.org/packages/Glimpse.RouteLocalization.Mvc/

It is a simple copy & modification of the already existing Routes Tab but adds information of localized routes:

image

I think this might be useful for debugging.

@mdmoura
Copy link

mdmoura commented Sep 23, 2014

Great! Going to give it a try ...
Documentário look good.

@brgrz
Copy link

brgrz commented Sep 24, 2014

Docs are much more informative now and cover some special cases too (the ones which popped up in Issues).

All in all, very good work this project.

@mdmoura
Copy link

mdmoura commented Oct 9, 2014

Hi Dresel. Any ETA for the final version?
I have been testing Route Localization on a project and until now I didn't find any more problems.

@Dresel
Copy link
Owner

Dresel commented Oct 13, 2014

I wanted to do some minor modifications (see SelectingAndTranslatingRoutes - Adding neutral routes & Getting the LocalizationCollectionRoutes directly) before. I just released the first release version :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants