fix preferred translation #1244
Conversation
There was a problem hiding this comment.
To be a bit more specific in a nested loop, this should preferably be break 1;
BTW, this screams to be unit tested.. If we can extract the foreach and usort into a separate method, we could perfectly test the alphabetical order and language preference.
This comment has been minimized.
This comment has been minimized.
tvdijen
force-pushed
the
fix-disco-language
branch
2 times, most recently
from
7c597df to
8b733bc
Compare
|
I think I've made a good start here, but I'm not really good with unit tests, so it would be great if someone can pick up from here |
tvdijen
force-pushed
the
fix-disco-language
branch
from
8b733bc to
4fe7630
Compare
tvdijen
force-pushed
the
fix-disco-language
branch
from
4fe7630 to
8f641a9
Compare
| * @param array $idpList | ||
| * @return array | ||
| */ | ||
| private function getPreferredTranslations(array $idpList, array $tryLanguages): array |
There was a problem hiding this comment.
I think this is quite misleading. What this method is doing is returning a list of IdPs, sorted by name, where their names are translated based on the preferred translation for the given request. I don't think getPreferredTranslations() correctly describes that, but instead it sounds like it's just returning the preferred translations for some given IdPs.
I would actually suggest refactoring this code. There's a lot of repetition in there, and testing is quite hard, so we could benefit from routing to feed requests and get responses out of it.
I'm going to merge @m0ark's commit into 1.18, and we can keep working on this meanwhile.
|
I agree with Tim here. We should do this properly and add unit tests. However, it's quite difficult to get some tests working, so I've prioritized getting the fix into a stable release (1.18.1, out just now), and then I'm leaving this PR open as a reminder that we need to get back to this. |
tvdijen
force-pushed
the
master
branch
3 times, most recently
from
1c686ab to
eb20457
Compare
tvdijen
force-pushed
the
master
branch
2 times, most recently
from
08ebb9c to
64fca25
Compare
|
Fixed in #1512. |
4 participants
Preferred language selection is broken in discovery service. Currently it uses the last valid translation instead of the first valid.