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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Apr 2, 2019
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
518aea9
fix locale url change, from default locale to other locale
kokspflanze Mar 21, 2019
973d0ef
Merge remote-tracking branch 'remotes/upstream/master' into feature/d…
kokspflanze Mar 21, 2019
963895c
cs fix
kokspflanze Mar 21, 2019
d464723
cs fix
kokspflanze Mar 21, 2019
966a7cb
fixing basePath with and without default
kokspflanze Mar 26, 2019
eecdcb9
cs fix
kokspflanze Mar 26, 2019
4ec2f8d
cs fix
kokspflanze Mar 26, 2019
2b47ecc
getter / setter removed
kokspflanze Mar 27, 2019
bf8e937
added more tests
kokspflanze Mar 29, 2019
0abec27
added tests for found
kokspflanze Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 59 additions & 13 deletions src/SlmLocale/Strategy/UriPathStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
use SlmLocale\LocaleEvent;
use Zend\Router\Http\TreeRouteStack;
use Zend\Router\SimpleRouteStack;
use Zend\Stdlib\RequestInterface;
use Zend\Uri\Uri;

class UriPathStrategy extends AbstractStrategy
Expand Down Expand Up @@ -103,7 +104,7 @@ public function detect(LocaleEvent $event)
return;
}

$base = $this->getBasePath();
$base = $this->getBasePath($request);
$locale = $this->getFirstSegmentInPath($request->getUri(), $base);
if (! $locale) {
return;
Expand Down Expand Up @@ -144,7 +145,7 @@ public function found(LocaleEvent $event)
}
}

$base = $this->getBasePath();
$base = $this->getBasePath($request);
$found = $this->getFirstSegmentInPath($request->getUri(), $base);

if ($this->router instanceof TreeRouteStack) {
Expand Down Expand Up @@ -180,7 +181,7 @@ public function found(LocaleEvent $event)
public function assemble(LocaleEvent $event)
{
$uri = $event->getUri();
$base = $this->getBasePath();
$base = $this->getBasePath($event->getRequest());
$locale = $event->getLocale();

if (! $this->redirectToCanonical() && null !== $this->getAliases()) {
Expand All @@ -193,30 +194,66 @@ public function assemble(LocaleEvent $event)
$path = $uri->getPath();

// Last part of base is now always locale, remove that
$parts = explode('/', trim($base, '/'));
array_pop($parts);
$parts = explode('/', trim($base, '/'));
$lastElement = count($parts) - 1;

$removeFirstLocale = true;
if (null !== $this->default &&
isset($parts[$lastElement]) &&
! in_array($parts[$lastElement], $event->getSupported(), true) &&
$parts[$lastElement] !== $this->default
) {
$removeFirstLocale = false;
}

if (true === $removeFirstLocale) {
// Remove first part
array_pop($parts);
}

$base = implode('/', $parts);

if ($base) {
$path = substr($path, strlen($base));
$path = substr(trim($path, '/'), strlen($base));
}
$parts = explode('/', trim($path, '/'));

// Remove first part
array_shift($parts);
$parts = explode('/', trim($path, '/'));
if (true === $removeFirstLocale) {
// Remove first part
array_shift($parts);
}

if ($locale === $this->default) {
$locale = '';
} else {
$locale .= '/';
}

$path = $base . '/' . $locale . implode('/', $parts);
$path = ($base ? '/' : '') . trim($base, '/') . '/' . $locale . implode('/', $parts);
$uri->setPath($path);

return $uri;
}

/**
* @return SimpleRouteStack
*/
public function getRouter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, i changed it in tests too

{
return $this->router;
}

/**
* @param SimpleRouteStack $router
* @return $this
*/
public function setRouter($router)
{
$this->router = $router;

return $this;
}

protected function getFirstSegmentInPath(Uri $uri, $base = null)
{
$path = $uri->getPath();
Expand All @@ -240,12 +277,21 @@ protected function getAliasForLocale($locale)
}
}

protected function getBasePath()
/**
* @param RequestInterface|null $request
* @return string|null
*/
protected function getBasePath(RequestInterface $request = null)
{
$result = null;
if ($this->router instanceof TreeRouteStack) {
return $this->router->getBaseUrl();
$result = $this->router->getBaseUrl();
}

if (null === $result && null !== $request && method_exists($request, 'getBasePath')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why router does not have baseUrl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check #40
i had the same problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$result = $request->getBasePath();
}

return null;
return $result;
}
}
25 changes: 19 additions & 6 deletions tests/SlmLocaleTest/Strategy/UriPathStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ public function testDetectWithBaseUrlReturnsRightPartOfPath()
$this->event->setResponse(new HttpResponse());

$locale = $this->strategy->detect($this->event);
$expected = 'en';
$this->assertEquals($expected, $locale);
$this->assertSame('en', $locale);
}

public function testFoundRedirectsByDefault()
Expand Down Expand Up @@ -391,6 +390,7 @@ public function testAssembleWithDefault()
$this->event->setLocale('en');
$this->event->setUri($uri);

$this->strategy->getRouter()->setBaseUrl('/nl');
$this->strategy->setOptions([
'default' => 'en',
]);
Expand All @@ -409,15 +409,13 @@ public function testAssembleWithDefaultNotMatching()
$this->event->setLocale('en');
$this->event->setUri($uri);

$this->strategy->getRouter()->setBaseUrl('/nl');
$this->strategy->setOptions([
'default' => 'fr',
]);
$this->strategy->assemble($this->event);

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests for found

}

public function testDisableUriPathStrategyPhpunit()
Expand All @@ -442,6 +440,21 @@ public function testDisableUriPathStrategyPhpunit()
$_SERVER['DISABLE_URIPATHSTRATEGY'] = false;
}

public function testAssembleWithDefaultMatchingCurrent()
{
$uri = new Uri('/foo/bar/baz');

$this->event->setLocale('en');
$this->event->setUri($uri);

$this->strategy->setOptions([
'default' => 'fr',
]);
$this->strategy->assemble($this->event);

$this->assertSame('/en/foo/bar/baz', $this->event->getUri()->getPath());
}

protected function getPluginManager($console = false)
{
$sl = new ServiceManager();
Expand Down