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

Sort unpreferred languages by root page priority #309

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
2 participants
@aschempp
Copy link
Contributor

aschempp commented Jan 31, 2019

My case was this:

  • folderUrl is enabled
  • my browser prefers de as language
  • I have four pages, which are fetched in this order from the DB
    1. Alias foo with language en
    2. Alias foo with language fr
    3. Alias foo with language zh
    4. Alias foo/bar with language en

Because none of the languages is in my browser preferences, the items are currently not re-sorted. So Page 1 is never compared to page 4, which means they are not sorted by alias.

I have not yet created a unit test, it probably needs a new test method and can't be accomplished within the existing data providers.

@leofeyer leofeyer added the defect label Jan 31, 2019

@leofeyer leofeyer added this to the 4.7.0 milestone Jan 31, 2019

@leofeyer leofeyer merged commit 1751b20 into contao:4.7 Feb 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 88.499%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 1, 2019

Thank you @aschempp.

@@ -421,7 +421,7 @@ private function sortRoutes(array &$routes, array $languages = null): void
return -1;
}
return $pageB->rootIsFallback ? 1 : 0;
return $pageB->rootIsFallback ? 1 : strnatcmp((string) $pageA->rootSorting, (string) $pageB->rootSorting);

This comment has been minimized.

@leofeyer

leofeyer Feb 1, 2019

Member

Hm, PageModel::$rootSorting is actually an integer. Is strnatcmp() really correct here?

This comment has been minimized.

@aschempp

aschempp Feb 1, 2019

Author Contributor

I thought about this as well, but it's easier than a three-way comparison (x > y ? -1 : (y > x ? 1 : 0))

This comment has been minimized.

@leofeyer

leofeyer Feb 1, 2019

Member

Isn't this what the new spaceship operator <=> is for?

This comment has been minimized.

@aschempp

aschempp Feb 1, 2019

Author Contributor

wooot! 🎉 never heard of that thing, sure is the right choice!

This comment has been minimized.

@leofeyer

leofeyer Feb 1, 2019

Member

Changed in ff24fc3.

@aschempp aschempp deleted the aschempp:bugfix/routing-language-sort branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment