- 🇮🇳India mohit_aghera Rajkot
Oops, fixing phpcs issues and queuing again for bot.
The last submitted patch, 51: test-only-2883450-51.patch, failed testing. View results →
- Status changed to Needs work
almost 2 years ago 1:13am 5 February 2023 - 🇺🇸United States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.
Confirmed the issue in D10
Standard install
Enabled a 2nd language (german in my case)
Allow for content translation of basic page content type
Create a node selecting "not specified"
URL is node/1
Go to de/admin/content
URL of NodeA is /node/1
Applied patch
cleared cache
URL of NoadeA is de/node/1Moving to NW for the issue summary update that was requested in #43
- 🇮🇳India mohit_aghera Rajkot
Thanks for the review @smustgrave
I've tried to attempt issue summary update. Can you please have a look at it once.Keeping the issue tag for now, feel free to remove it after verification.
- Status changed to Needs review
almost 2 years ago 4:08am 5 February 2023 - Status changed to RTBC
almost 2 years ago 3:58pm 6 February 2023 The last submitted patch, 52: 2883450-52.patch, failed testing. View results →
- 🇮🇳India mohit_aghera Rajkot
Looks like random failures related to CKEditor5.
Triggering for re-test and moving it to RTBC again. - Status changed to Fixed
over 1 year ago 9:41am 17 March 2023 Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
over 1 year ago 5:23pm 17 July 2023 - 🇷🇺Russia kala4ek 🇷🇺 Novosibirsk
That issue is breaks the link that must be always neutral, like links insite sitemap.xml (if there are a lot of links the additional sitemaps are generated) and most probalby some more such cases.
Because the code was relied on "not applicable" language, but now the language applied anyway...
- 🇩🇪Germany arnalyse
We're running into an issue similar to the one kala4ek described.
The OpenID Connect module generated its redirect URIs like https://default/openid-connect/generic, but now includes the language, e.g. https://default/en/openid-connect/generic
This is a huge problem for us, as it breaks the SSO login for our users when upgrading to D10.1
An issue for this hals already been created: https://www.drupal.org/project/openid_connect/issues/3383036 ✨ Redirect URI has the language prefix in it (in D10) Needs reviewThe OpenID Connect module generates its links as follows:
return Url::fromRoute('openid_connect.redirect_controller_redirect', $route_parameters, [ 'absolute' => TRUE, 'language' => $this->languageManager->getLanguage(LanguageInterface::LANGCODE_NOT_APPLICABLE), ])->toString();
I'm not sure that
LANGCODE_NOT_APPLICABLE
should get a language as the definition ofLANGCODE_NOT_APPLICABLE
reads as follows:/** * The language code used when the marked object has no linguistic content. * * Should be used when we explicitly know that the data referred has no * linguistic content. * * See http://www.w3.org/International/questions/qa-no-language#nonlinguistic. */ const LANGCODE_NOT_APPLICABLE = 'zxx';
I'd like to help resolve this, so if anyone knows what the correct way to generate a url without a langcode should be from now on, please let me know, so I can generate a patch for the OpenID Connect module.
- 🇩🇪Germany Anybody Porta Westfalica
Thanks for the reports. Still I think it was correct to *fix* this and I think this is a new side-effect and issue we have to discuss. Perhaps in a separate issue linking this one to understand the history?
Just an idea: Perhaps the right way would be to also provide a language neutral URL for such contents?
And eventually need a flag to determine which kind of link we need?I think we can clearly see we can have both cases... this is a really complicated and very general task. Needs heavy discussion, I think.
In my mind it's clear that the current fix for LANG_CODE_NOT_APPLICABLE is wrong.
In the case of LANG_CODE_NOT_APPLICABLE no language prefix should be generated. No language prefix was generated before, it should not after since it will break a lot of stuff around the world. If you use LANG_CODE_NOT_APPLICABLE, you specifically don't want langcode, why would langcode be added.
See also https://www.drupal.org/project/drupal/issues/3385550 🐛 Language negotiation breaks updating Drupal 9 to 10 Needs work , I spent an hour trying to figure out why openid_connect breaks. I will include a patch in the latter issue.
- 🇩🇪Germany Anybody Porta Westfalica
@sboden not really sure it's a safe thing, but if it is, we need a follow up which doesn't only implement that but also adds proper documentation to inform users about the important differences (propably below the selection), I think that's the most important difference then.
But let's wait for core maintainers feedback here, I think?
So what I see as a "problem" is for following code (it's a small piece from an openid_connect library):
$language_none = \Drupal::languageManager() ->getLanguage(LanguageInterface::LANGCODE_NOT_APPLICABLE); $redirect_uri = Url::fromRoute( 'openid_connect.redirect_controller_redirect', [ 'openid_connect_client' => $this->getPluginId(), ], [ 'absolute' => TRUE, 'language' => $language_none, ] )->toString(TRUE);
Assume you don't know anything about the APIs being called and you just see this piece of code.
Do you expect "/nl" (in my case) to be added to the URL or not? That's the question and the problem I'm having. In the code above I set language explicitly to "not applicable" so I expect the "/nl" language prefix will not be added.
It did not add a language prefix before the fix, but it does now.
I also have no other easy way to get rid of the language prefix (unless we make a LANGCODE_ABSOLUTELY_NOT_APPLICABLE case, just kidding... don't do it). In my case above the generated URL is used for openid_connect and the "other side" just can't handle a language prefix.