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

Bug/resolve locale #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

qpautrat
Copy link

Resolve locale from current host
See #128

@pitpit
Copy link

pitpit commented Oct 13, 2015

+1

Can this PR be merged please ?

@acasademont
Copy link
Collaborator

Hi @qpautrat thanks for the patch, could you please rebase with the new master?

@qpautrat
Copy link
Author

qpautrat commented Dec 2, 2015

@acasademont Done !

@acasademont
Copy link
Collaborator

@qpautrat thanks but i'm afraid there must be something wrong here, in your PR there are also some of my commits and the diff is not clear. Can you please squash and make sure there's only 1 commit and the diff is clear? Thank! :D

@qpautrat
Copy link
Author

qpautrat commented Dec 2, 2015

@acasademont Is this ok now ? I'm really sorry, i'm not used to do that, kind of beginner here. 😕

@@ -192,6 +192,12 @@ private function matchI18n(array $params, $url)
$params['_route'] = substr($params['_route'], $pos + strlen(I18nLoader::ROUTING_PREFIX));
}

foreach($this->hostMap as $locale => $host) {
if ($host == $this->context->getHost()) {
$this->context->setParameter('_locale', $locale);
Copy link

Choose a reason for hiding this comment

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

you should break the loop here once the locale is found

Copy link
Author

Choose a reason for hiding this comment

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

Thanks !

@acasademont
Copy link
Collaborator

Thanks! Could you fix the small thing I mentioned and squash all the commits please?

@qpautrat
Copy link
Author

qpautrat commented Dec 3, 2015

@acasademont Done.

@pitpit
Copy link

pitpit commented Sep 14, 2018

what about this PR ? Could it be merged please ?

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