Issue #28 open

Using localeurl with other apps that works with full URL's

Bojan Mihelac
created an issue

Some applications already generate URL with language code prefix and except full URL, with language prefix to work properly. For example FeinCMS is one, but I believe other similliar apps could have similiar issues with localeurl.

To make it easier for this apps to work together I am proposing following feature:

  1. If //request.LANGUAGE_CODE// is already present, localeurl would not try to set it and strip a language prefix from //request.path_info//.

  2. If URL generated with //reverse// already have language prefix (one of the code from //LANGUAGES// setting) //localeurl// patched //reverse// function would not try to additionally process returned URL.

If there is a concern that this could introduce backward incompatible changes, setting could be introduced that allow developer to control this feature.

What do you think?

Bojan Mihelac

Comments (10)

  1. Carl Meyer repo owner

    Hi, thanks for taking the time to make this proposal.

    I'm not sure I understand why this feature would be useful. What is the purpose of using django-localeurl at all if you already have a system that prefixes reversed URLs with a language code and sets the request language code? It seems like this system would be an alternative to django-localeurl, and there would be no reason to mix them. Why would the request language code sometimes be already set when the request reaches LocaleURLMiddleware, and sometimes not?

  2. Bojan Mihelac reporter

    Hi Carl, thanks for quick answer.

    I guess the problem I am tring to solve is mixing reusable apps that are handling 'locale' urls themselves with other apps that do not know anything about locale urls.

    I will try to explain in more details use case of using django-localeurl and FeinCMS app that is part of a website.

    #urls.py
    urlpatterns = patterns('',
        (r'^admin/', include(admin.site.urls)), #app with django-localeurl enabled  
        # other apps wiith django-localeurl enabled
    )
    urlpatterns += patterns('',
        url(r'', include('feincms.urls')),
    )
    

    With given urls.py, admin and other applications would behave as expected with django-localeurl installed.

    FeinCMS Page model holds a structure of pages on website and cache their URL in ``_cached_url`` attribute. Typical structure for urls on multilingual website pages looks something like this:

      /en/ #English root page
      /en/about_us/
      ...
      /de/ # Deutsch root page
      /de/uber_uns/
      ...
    

    First problem is that FeinCMS catch-all url pattern for '/en/about_us/' url would receive stripped path (/about_us/) and as such Page would not be found. Simplified code for view handler where it looks for requested page is:

    page = Page.objects.get(_cached_url=request.path)
    

    Second problem is that Page model defines get_absolute_url method returns value of _cached_url. django-localeurl patched reverse function will prepend lang prefix to it making it incorrect.

    >>> about_us_page.get_absolute_url()
    '/en/en/about_us/'
    

    Both problems can be solved with using custom handler that will prepend lang code again and ABSOLUTE_URL_OVERRIDES, but I thought that wider solution would be if django-localeurl can handle similar cases out of the box.

    If you think this could be useful feature, I can write and propose a patch for it.

    Bojan

  3. Carl Meyer repo owner

    Hi Bojan - I appreciate the added detail. There are still some things that aren't clear to me:

    1. If the Page model defines get_absolute_url to just return the value of _cached_url, it doesn't seem that it would use reverse for that? So where does the localeurl monkey-patch actually come into effect?

    2. You propose to modify the localeurl middleware to only strip request.path if the request has no language code set already. How would the request get a language code set early enough (during or before the request middleware phase), and only for those URLs which will end up resolving to FeinCMS Page objects? Does FeinCMS provide its own request middleware that sets language code based on request.path and only does so for paths that will end up resolving to FeinCMS?

  4. Bojan Mihelac reporter

    1. Page model does actually uses permalink decorator around get_absolute_url so monkey-patched reverse method come in place.

    2. I am not sure this idea would work for FeinCMS anymore (and cannot think of other use cases for this). For this to work request middleware would have to decide if current URL is related to FeinCMS page or is not, most probably executing some db queries that would be unnecessary overhead. It is more efficient to have custom handler that just adds language prefix back when catch-all url pattern is reached.

    So now I guess my original proposition can be reduced to: if URL generated with reverse already have language prefix (one of the code from LANGUAGES setting) localeurl patched reverse function would not try to additionally process returned URL.

    Does that sounds reasonable?

    regards, Bojan

  5. Carl Meyer repo owner

    Very sorry I didn't get to merging this sooner - had a very busy couple of months.

    As I look at it now, I'm having second thoughts about adding yet another setting for what still feels like a very unusual case.

    As an alternative approach, what if I fixed #19 so that the monkeypatching is not automatic but has to be done explicitly? Then you could easily just use your own monkeypatching function with whatever specialized behavior you need. This seems more elegant and general as a way of handling edge cases like this.

    My apologies again for the time you spent working on the pull request. Let me know if you think the alternative approach would be adequate for your situation.

  6. Bojan Mihelac reporter

    Hi Carl.

    No problem, this is new feature proposition and while I could always work around it, I felt it could be also solved inside django-localeurl.

    Fixing #19 (Require explicit monkeypatching) and not merging this pull request does seem reasonable for me. As patched ``reverse`` method is tiny, it would be simple to implement more specific one and use it instead of "default" one.

  7. Log in to comment