Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Feature/guessing language via accept language header #184

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

skoegl
Copy link
Member

@skoegl skoegl commented Aug 20, 2019

With this patch it would also be possible to guess the users preferred language via Accept-Language client header.

@skoegl skoegl changed the base branch from master to develop August 20, 2019 15:38
@skoegl
Copy link
Member Author

skoegl commented Aug 27, 2019

Hey again,

I would really like to push it into the 2.5 branch. It's not the most efficient code yet, but it's the most accurate one right now for guessing the preferred language a client would like to see. The X
-Appengine-Country only catches the country to language mapping of a client, but not the prefered language.

Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this pull request, please change the two obsolete lines and move the definition of langSet into the else-branch.

__init__.py Outdated
if len( tmppath )>0 and tmppath[0] in conf["viur.availableLanguages"]+list( conf["viur.languageAliasMap"].keys() ):
self.language = tmppath[0]
return( path[ len( tmppath[0])+1: ] ) #Return the path stripped by its language segment
langSet = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is obsole: You're setting langSet to True and return from the function afterwards.

__init__.py Outdated
@@ -360,18 +360,40 @@ def selectLanguage( self, path ):
elif conf["viur.languageMethod"] == "url":
tmppath = urlparse.urlparse( path ).path
tmppath = [ urlparse.unquote( x ) for x in tmppath.lower().strip("/").split("/") ]
langSet = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be moved into the else part.

__init__.py Outdated
else: # This URL doesnt contain an language prefix, try to read it from session
if session.current.getLanguage():
self.language = session.current.getLanguage()
elif "X-Appengine-Country" in self.request.headers.keys():
langSet = True
if not langSet and "Accept-Language" in self.request.headers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The control-flow is botched. Instead of carrying the marker langSet, just return path in L371. Same thing at the end of this if block - if found return path.

__init__.py Outdated
if acceptLangHeader:
acceptLangHeader = acceptLangHeader.split(",")
for possibleLang in acceptLangHeader[:7]:
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for try/except here. As you are not interested in regionEtc just call lng = possibleLang.split("-")[0] - which will always work

@skoegl
Copy link
Member Author

skoegl commented Sep 3, 2019

In a follow up PR I would also like reformat the complete file - it is still not using our viur coding style...

__init__.py Outdated
self.language = lng
return path
elif ";" in lng:
lng, weight = lng.split(";")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more small thing. This Line can also fail. And you don't use weight at all. So also use lng.split(";")[0] - which doesn't break if there are more than two colons in that string

tsteinruecken
tsteinruecken previously approved these changes Dec 4, 2019
Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an entry in the CHANGELOG.md

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

Successfully merging this pull request may close these issues.

4 participants