fix locale url change, from default locale to other locale #105

kokspflanze · 2019-03-21T22:35:58Z

sry had a little bug, with locale url change generation

current url: /
url for en: /en

current url: /currency/foobar
url for en: /en/foobar // wrong currency was missing

that i fixed with this change

…efault_locale # Conflicts: # tests/SlmLocaleTest/Strategy/UriPathStrategyTest.php

coveralls · 2019-03-21T22:59:27Z

Coverage increased (+1.3%) to 84.211% when pulling 0abec27 on kokspflanze:feature/default_locale into ddc7f55 on basz:master.

svycka

what if this will be used with basepath? also, this strategy became a bit complex and hard to follow and I think needs refactor.
But I don't have time for this unless you can do it I will merge this if it works for you

kokspflanze · 2019-03-22T07:26:03Z

what you mean with basePath?

if you access the page with directories? like page stay in /page/public and you have to call it with page/public ?
because that was not working before, but i want to add the support for this too^^

svycka · 2019-03-22T07:32:25Z

Yes, I mean https://exampe.com/base/path/en/foo/bar I am not sure this worked before. I don't use this maybe you are right it does not work but would be nice if you could make it work

svycka · 2019-03-22T07:40:09Z

If it will be too hard to support basepath I guess we can throw an exception if base bath is set and we don't support it if it does not work anyway. Anyway, I don't know if anyone using this.

kokspflanze · 2019-03-22T07:46:55Z

i was using it^^ and first i was searching for the error^^
i will add a way to support it no way, i hope until end of next week.

svycka · 2019-03-26T07:18:24Z

@kokspflanze should I merge this or you are still working with this working?

kokspflanze · 2019-03-26T07:40:33Z

@svycka let it open, as reminder for me todo the base stuff =)
i will do this this week, not sure today or friday.

kokspflanze · 2019-03-26T22:00:36Z

resolve now
#40
#47

svycka · 2019-03-27T06:42:18Z

tests/SlmLocaleTest/Strategy/UriPathStrategyTest.php

-        $actual   = $this->event->getUri()->getPath();
-
-        $this->assertSame($expected, $actual);
+        $this->assertSame('/en/foo/bar/baz', $this->event->getUri()->getPath());


strategies should not change base path. Should have been /nl/en/foo/bar/baz instead of /en/foo/bar/baz we can't modify base path as it is just folder name and not an application route.

nl is the language, it has to be changed to en

but also it is base path $this->strategy->getRouter()->setBaseUrl('/nl'); baseurl is often alias or folder name where the application is installed and changing it will result in not found response. @kokspflanze what if you set $this->strategy->getRouter()->setBaseUrl('/my-app'); what url it will generate?

@basz am I right here?

i will add a test for this (when im home)

Document_root /my-app
Request /my-app/nl/...

added 2 tests for this, with default language to other language and without default language

install your app in '/var/www/my-app'
set your document_root to '/var/www'
than you should access your app via 'http://192.1.1.1.1/my-app/public'

thats all

so as I understand baseurl is my-app/public and you can't change it. But in tests you changing it.
let's say if you would have app in my-app/en instead of my-app/public now you would redirect to my-app/nl and it does not exist. Does make sense?

Do I still not understand something? please rename your public folder so mething like nl or someting and test

added more tests, like testAssembleWithBasePathWithMatchingLanguageName

if you rename your public directory to nl and you use the default configuration, sure it would not work and mostly bug.
but it works without the default configuration, but i dont think that someone is doing this kind of stuff.

i also added the testAssembleWithBasePathWithMatchingLanguageName in master and i get my-app/nl/en/nl/foo/bar/baz instead of /my-app/nl/en/foo/bar/baz. the problem also exist with the public directory.

Okay, I looked deeper and now understand why you append language and change base path. As I understand that because you testing assemble event but then maybe you can also add tests for found event? Because found event is used for redirect and also alters baseUrl.

added tests for found

svycka · 2019-03-27T06:50:38Z

src/SlmLocale/Strategy/UriPathStrategy.php

+            $result = $this->router->getBaseUrl();
+        }
+
+        if (null === $result && null !== $request && method_exists($request, 'getBasePath')) {


why router does not have baseUrl?

check #40
i had the same problem

then we can remove $this->router->getBaseUrl(); if we can get it from request?

yes and no, to get it from the router is much better, as to use it from the request, should just be a fallback.

svycka · 2019-03-27T06:51:49Z

src/SlmLocale/Strategy/UriPathStrategy.php

+    /**
+     * @return SimpleRouteStack
+     */
+    public function getRouter()


don't add gettters/setters we already have it in the constructor

done, i changed it in tests too

added test for base

svycka

Also maybe we could make all strategies final. Would reduce BC break chances.

svycka · 2019-03-28T06:50:13Z

tests/SlmLocaleTest/Strategy/UriPathStrategyTest.php

-        $actual   = $this->event->getUri()->getPath();
-
-        $this->assertSame($expected, $actual);
+        $this->assertSame('/en/foo/bar/baz', $this->event->getUri()->getPath());


I still don't understand how changing baseUrl works for you. As I understand if you change baseUrl you should not be able to access your app at all. unless you have another app installed with that URL or have an alias to the previous one.

because I think correct behavior should be like this:

baseUrl: / default: nl locale: en request: /foo/bar redirects to /en/foo/bar baseUrl: / default: de locale: en request: /nl/foo/bar redirects to /en/foo/bar baseUrl: /base/url/en default: nl locale: en request: /base/url/en/foo/bar redirects to /base/url/en/en/foo/bar yes it's .../en/en/.. because /base/url/en is base path and we can't change it baseUrl: /base/url/en default: nl locale: nl request: /base/url/en/foo/bar does not redirect to default locale

Can you share your apache or nginx config for this baseUrl? I would like to understand what you want to achieve.

svycka · 2019-04-02T07:57:35Z

okay 🚢

kokspflanze added 4 commits March 21, 2019 23:31

fix locale url change, from default locale to other locale

518aea9

Merge remote-tracking branch 'remotes/upstream/master' into feature/d…

973d0ef

…efault_locale # Conflicts: # tests/SlmLocaleTest/Strategy/UriPathStrategyTest.php

cs fix

963895c

cs fix

d464723

svycka reviewed Mar 22, 2019

View reviewed changes

fixing basePath with and without default

966a7cb

kokspflanze added 2 commits March 26, 2019 23:04

cs fix

eecdcb9

cs fix

4ec2f8d

svycka reviewed Mar 27, 2019

View reviewed changes

svycka requested changes Mar 27, 2019

View reviewed changes

getter / setter removed

2b47ecc

added test for base

svycka reviewed Mar 28, 2019

View reviewed changes

kokspflanze and others added 2 commits March 29, 2019 18:28

added more tests

bf8e937

added tests for found

0abec27

svycka changed the base branch from master to develop April 2, 2019 07:54

svycka self-assigned this Apr 2, 2019

svycka merged commit c085ed7 into basz:develop Apr 2, 2019

kokspflanze deleted the feature/default_locale branch April 2, 2019 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix locale url change, from default locale to other locale #105

fix locale url change, from default locale to other locale #105

kokspflanze commented Mar 21, 2019

coveralls commented Mar 21, 2019 •

edited

Loading

svycka left a comment

kokspflanze commented Mar 22, 2019

svycka commented Mar 22, 2019

svycka commented Mar 22, 2019

kokspflanze commented Mar 22, 2019

svycka commented Mar 26, 2019

kokspflanze commented Mar 26, 2019

kokspflanze commented Mar 26, 2019 •

edited

Loading

svycka Mar 27, 2019

kokspflanze Mar 27, 2019

svycka Mar 27, 2019

kokspflanze Mar 27, 2019

kokspflanze Mar 27, 2019

kokspflanze Mar 28, 2019

svycka Mar 28, 2019

kokspflanze Mar 29, 2019

svycka Apr 1, 2019

kokspflanze Apr 1, 2019

svycka Mar 27, 2019

kokspflanze Mar 27, 2019

svycka Mar 27, 2019

kokspflanze Mar 27, 2019

svycka Mar 27, 2019

kokspflanze Mar 27, 2019

svycka left a comment

svycka Mar 28, 2019

svycka commented Apr 2, 2019

fix locale url change, from default locale to other locale #105

fix locale url change, from default locale to other locale #105

Conversation

kokspflanze commented Mar 21, 2019

coveralls commented Mar 21, 2019 • edited Loading

svycka left a comment

Choose a reason for hiding this comment

kokspflanze commented Mar 22, 2019

svycka commented Mar 22, 2019

svycka commented Mar 22, 2019

kokspflanze commented Mar 22, 2019

svycka commented Mar 26, 2019

kokspflanze commented Mar 26, 2019

kokspflanze commented Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svycka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svycka commented Apr 2, 2019

coveralls commented Mar 21, 2019 •

edited

Loading

kokspflanze commented Mar 26, 2019 •

edited

Loading