Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #25933 -- Allowed an unprefixed default language in i18n_patterns. #5823

Closed
wants to merge 4 commits into from

Conversation

urbaniak
Copy link
Contributor

It's different than i18n_patterns by not prefixing the default language.

All other languages are prefixed similar to i18n_patterns.

It allows to add another language without changing the urls of the first
default language.

@gastlich
Copy link

+1

@charettes
Copy link
Member

Hi @urbaniak, please follow the contribution guidelines and file a new ticket.

IMHO we don't need to introduce another function and a simple prefix kwarg defaulting to True in i18n_patterns should do. This will also require tests and a release note if it was to be accepted.

@urbaniak urbaniak force-pushed the i18n_patterns_unprefixed branch 2 times, most recently from 26a3803 to f9571b3 Compare December 13, 2015 18:51
@urbaniak
Copy link
Contributor Author

Simplified this as you suggested.

Opened the ticket on code.djangoproject.com - https://code.djangoproject.com/ticket/25933#ticket

I'll add tests, docs and release notes if that solution will be acceptable.

Setting I18N_PREFIX_DEFAULT_LANGUAGE to False will stop prefixing the
default language urls.

It's useful for adding additional languages to site without translation
cause it doesn't require any url migrations (/news/ becomes /en/news/
in current implementation).
@urbaniak
Copy link
Contributor Author

Updated it once again, it now uses a setting called I18N_PREFIX_DEFAULT_LANGUAGE.

It's much smaller and simpler change now.

@charettes
Copy link
Member

We try to avoid adding new settings.

I think the kwarg approach was more appropriate as it allowed multiple instance of i18n_patterns to be used with different values for prefix. It also avoid relying on global state and make the implementation more contained.

If you're a fan of settings nothing prevent you from defining a I18N_PREFIX_DEFAULT_LANGUAGE in your project and passing it's value to i18n_patterns in your urls definition.

@urbaniak
Copy link
Contributor Author

It's not possible (according to docs) to use multiple i18n_patterns.

Adds the language code prefix to every URL pattern within this
function. This may only be used in the root URLconf, not in an included
URLconf.

Adding a param there was confusing IMO cause then the i18n_patterns takes only *args as urls right now, so this additional param with some urls looks like that

urlpatterns += i18n_patterns(
    url(_(r'^contact/$'), views.contact),
    url(_(r'^about/$'), views.about),
    prefix_default_language=True,
)

It's also cleaner IMO cause right now we don't need to pass the prefix_default_language from i18n_patterns to the LocaleRegexURLResolver.

Here's the previous patch version:

diff --git a/django/conf/urls/i18n.py b/django/conf/urls/i18n.py
index 056b957..fed1a17 100644
--- a/django/conf/urls/i18n.py
+++ b/django/conf/urls/i18n.py
@@ -4,7 +4,7 @@ from django.core.urlresolvers import LocaleRegexURLResolver
 from django.views.i18n import set_language


-def i18n_patterns(*urls):
+def i18n_patterns(*urls, prefix_default_language=True):
     """
     Adds the language code prefix to every URL pattern within this
     function. This may only be used in the root URLconf, not in an included
@@ -12,7 +12,10 @@ def i18n_patterns(*urls):
     """
     if not settings.USE_I18N:
         return urls
-    return [LocaleRegexURLResolver(list(urls))]
+    return [LocaleRegexURLResolver(
+        list(urls),
+        prefix_default_language=prefix_default_language,
+    )]


 urlpatterns = [
diff --git a/django/core/urlresolvers.py b/django/core/urlresolvers.py
index d37a401..8103b64 100644
--- a/django/core/urlresolvers.py
+++ b/django/core/urlresolvers.py
@@ -12,6 +12,7 @@ import re
 from importlib import import_module
 from threading import local

+from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured, ViewDoesNotExist
 from django.http import Http404
 from django.utils import lru_cache, six
@@ -461,15 +462,23 @@ class LocaleRegexURLResolver(RegexURLResolver):
     Rather than taking a regex argument, we just override the ``regex``
     function to always return the active language-code as regex.
     """
-    def __init__(self, urlconf_name, default_kwargs=None, app_name=None, namespace=None):
+    def __init__(
+        self, urlconf_name, default_kwargs=None, app_name=None, namespace=None,
+        prefix_default_language=True
+    ):
         super(LocaleRegexURLResolver, self).__init__(
             None, urlconf_name, default_kwargs, app_name, namespace)
+        self.prefix_default_language = prefix_default_language

     @property
     def regex(self):
         language_code = get_language()
         if language_code not in self._regex_dict:
-            regex_compiled = re.compile('^%s/' % language_code, re.UNICODE)
+            regex_compiled = (
+                re.compile('', re.UNICODE) if language_code == settings.LANGUAGE_CODE and
+                not self.prefix_default_language else
+                re.compile('^%s/' % language_code, re.UNICODE)
+            )
             self._regex_dict[language_code] = regex_compiled
         return self._regex_dict[language_code]

@charettes
Copy link
Member

It's not possible (according to docs) to use multiple i18n_patterns.

The doc states that you can't use i18n_patterns in included URLconf. It's not common to have multiple root URLconf but it's a pattern used in multi-host scenarios to route path from different hosts to completely different URLconf. I guess the documentation should be fixed to note that it may only be used in a root URLconf and LocaleMiddleware fixed to take into account request.urlconf when determining whether or not i18n_patterns are used.

While I agree the kwarg approach requires more adjustments than a global setting I'm not convinced it hurts readability that much.

@timgraham timgraham changed the title Add i18n_patterns_unprefixed. Fixed #25933 -- Allowed an unprefixed default language in i18n_patterns. Dec 14, 2015
That's the prefered way of configuring i18n_patterns to not prefix the default language.
@urbaniak
Copy link
Contributor Author

Updated to the i18n_patterns param if that's more appropriate and have higher chances got merged upstream.

If that's ok and it has chances to get merged - I'll update docs and release notes later this week.

@timgraham
Copy link
Member

The ticket is accepted -- will you have a chance to complete this soon or should we close the pull request for now?

@urbaniak
Copy link
Contributor Author

urbaniak commented Feb 1, 2016

Fixed one issues that I've found in meantime and updated docs.

That version of django is currently running on https://wipply.com, with default language set to en and additional pl language (which is not yet linked anywhere, but /pl/ is working).

@timgraham
Copy link
Member

Branch needs to be rebased so it merges cleanly and tests are missing. See our patch review checklist and please send a pull request when you have a patch that meets all the requirements. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants