Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OEL-1167: Theme the content language switcher #94
OEL-1167: Theme the content language switcher #94
Changes from 33 commits
8c6ef00
54fb4b1
4633bca
3ed54e2
ec1ab50
d2d4536
a369c61
af44289
ab65900
a58da02
e812d79
8e06e95
7f61d64
52f27eb
862198b
6bdc691
b90af4f
d6111df
d0341e0
44095f0
c956a83
bdeaf6a
7eef2e1
7e6ce8a
d7fc007
94185c9
1b27d53
7bf25c4
1586043
777b1ae
de3b993
5eaf632
bc274a4
e5b2f37
34998d5
023a770
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a
@var
doc on the$multilingual_helper
service above, then you don't need one here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice you are using
'und'
string literal instead of a constant.In Drupal 7 I would have told you to use
LANGUAGE_NONE
.Now this has changed, see LANGUAGE_NONE changed to LANGUAGE_NOT_SPECIFIED, LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE added
Not sure if we also need to support other possible values.
Perhaps safer to just check
isset($languages[$current_content_language_id])
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could introduce a local var for
$translation->language()->getId()
, to avoid the repeated call, e.g.$current_content_language_id
.We could also check whether
$languages[$current_content_language_id]
exists.Would be strange if it does not. Perhaps if a site language is deleted, renamed or disabled, while translations still exist? I don't think it will happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the scenario where this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I was considering to suggest early return pattern here (invert the if to get rid of indentation).
But then we would have to drop the early return when we add the
$variables['expandable_id']
.This is why I prefer to use early return only in functions with a single responsibility - which is often not the case for preprocess.
So ok to keep it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if language is 'und', will the message "This page is not available in ***." make sense for all cases?
I think we should settle for something that works, we have no time to investigate all possible scenarios.
If you say you want to keep the current version, let's go with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that these entries are already wrong in 1.x.
Also in two other files:
Imo we should have a separate branch to fix all of the entries, and then we rebase.
But I can accept if we want to do this here to move forward.