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.

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

Checks if the node have translation and if matches with the current l… #136

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

andrebonon
Copy link
Contributor

…anguage to bring the right content.

Copy link
Contributor

@podarok podarok left a comment

Choose a reason for hiding this comment

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

To fix code style and provide steps to test, please

if ($language && !$request_language || $request_language && $request_language->getId() != $alert_language) {
// Redirect to the same request URL with the language code added after the basePath.
$redirectUrl = $this->request->getSchemeAndHttpHost() . '/' . $langcode . $this->request->getRequestUri();
$response = new ModifiedResourceResponse(NULL, 303); // 303 See Other
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's stick to Drupal code style, please

Copy link
Contributor

Choose a reason for hiding this comment

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

This is resolved.

@andrebonon
Copy link
Contributor Author

Setup steps:

  • Add a new language: /admin/config/regional/language (take FR as example)
  • Enable content translation for Landing page (with LB) and Alerts CT: /admin/config/regional/content-language

Test steps:

  • create a translation for the homepage.
  • create a test page (/test-page) and translate it (/test-page-fr)
  • edit or create a new Alert and add a translation to it.

Expected results:

  • the EN version should be displayed in the /test-page
  • the FR version should be displayed in the /fr/test-page-fr
  • the alerts also are displayed for Homepage EN and Homepage FR

@@ -195,11 +208,34 @@ public function get() {
}

$uri = $this->request->query->get('uri');

// Extract the language code from the URI.
$langcode = substr($uri, 1, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrebonon is there a more bulletproof way to get the language? Do we know the url will always always look the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's delve into the underlying issue here. The initial request constructs the entire page based on the detected language, which, in this case, is determined through the URL path. However, the subsequent request to /alert does not include the language in the path, leading Drupal to default to the primary language instead of maintaining the language from the original request.

My attempts to override the language for the /alert request were unsuccessful.
Therefore, I opted to redirect the /alert request to /{lang}/alert, enabling Drupal to correctly identify the appropriate translation for the request.

It's important to note that this solution specifically impacts the Language detection URL when utilizing the path option. It doesn't affect the domain option, session, or any other existing detection modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the way to get the language, the other way I can think of would be using explode().
$path = explode('/', trim($uri, '/')); and get the $path[0].
That would help in cases we have custom lang codes or codes with 1 or 3 characters (what I guess don't follow ISO for lang codes).

Do you have any other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrebonon thanks for the detailed explanation. That all makes sense, and based on the situation I think your solution here makes sense. I understand that we have to rely on the URL for the language code, so let's move forward with your original solution.

Copy link
Contributor

@froboy froboy left a comment

Choose a reason for hiding this comment

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

I approve of the changes and the fix.

@podarok podarok merged commit 4343bf4 into open-y-subprojects:main Apr 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants