Language negotiation in core changed from Drupal 9 to 10, breaking things.
I have a piece of code in an openid_connect module to generate an internal URL, like this:
$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);
Which in Drupal 9 generated something as "https://mysite.com/openid_connect/acm-idm". I have language negotiation active, but the LanguageInterface::LANGCODE_NOT_APPLICABLE killed the language prefix.
I updated to Drupal 10 and then the behaviour changes: I get as output "https://mysite.com/nl/openid_connect/acm-idm" (note the "nl" prefix).
Looking at the code I end up in "Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl.php" in function "processOutbound":
In Drupal 10 there's code in there like this:
if (!isset($options['language']) || ($options['language'] instanceof LanguageInterface && in_array($options['language']->getId(), [
LanguageInterface::LANGCODE_NOT_SPECIFIED,
LanguageInterface::LANGCODE_NOT_APPLICABLE,
]))) {
$language_url = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_URL);
$options['language'] = $language_url;
}
In Drupal 9 the in_array() part above is not present.
If the language option I set is "LanguageInterface::LANGCODE_NOT_APPLICABLE" I do not want the language prefix to be added. If the LanguageInterface::LANGCODE_NOT_APPLICABLE is removed from the array above all is ok again.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | interdiff-6_9.txt | 1.16 KB | gauravvvv |
| #9 | 3385550-9.patch | 1.16 KB | gauravvvv |
| #6 | 3385550-06.patch | 1.06 KB | sboden |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !4815
Comments
Comment #2
cilefen commentedComment #3
longwaveThis was changed in #2883450: Missing url prefix on language neutral content
Comment #4
longwaveI get why LANGUAGE_NOT_SPECIFIED might need a default language prefix adding, but I do wonder if LANGCODE_NOT_APPLICABLE should have one at all.
Comment #5
sboden commentedLANGCODE_NOT_APPLICABLE should in my mind not have a language prefix: I have language negotation active on my site, but openid_connect that is used for authentication is very "touchy" about the URLs that you can use.
So in Drupal 9 my custom code generated "https://mysite.com/openid_connect/acm-idm" and after upgrading to Drupal 10 that is magically changed to "https://mysite.com/nl/openid_connect/acm-idm", which breaks openid_connect.
Comment #6
sboden commentedPatch that fixes the LANGCODE_NOT_APPLICABLE issue.
Comment #7
cilefen commented@sboden Please do not test on every platform configuration for a small bug fix that is unrelated to database. Test bots have a cost.
Comment #8
longwaveThe in_array() can be removed now there is only one value to check.
Comment #9
gauravvvv commentedRemoved in_array and fixed CC failure in #6. please review
Comment #10
sboden commentedPatch #9 fails: when you remove in_array() the line should be changed to a '=='.
Something as:
Comment #11
smustgrave commentedPer #10 and will need a test case to show the problem.
Comment #14
lauriiiI now see that there is a valid use case for generating language neutral URLs. Since we don't know the intent in
\Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::processOutboundat least without adding an API, I'm wondering if the right way to fix this would be to revert #2883450: Missing url prefix on language neutral content , and fix the original bug in\Drupal\Core\Entity\EntityBase::toUrland other instances that are generating URLs? It feels strange that\Drupal\Core\Entity\EntityBase::toUrlwould have to care about the interface language but at the same time, it seems like that it needs to care about it because that's where we know which behavior we want.Comment #15
longwave@lauriii What do you think about just dropping the LANGCODE_NOT_APPLICABLE check? The original issue only mentions "language not specified", which to me means we could try to select a default, but "not applicable" means there doesn't need to be a language at all.
Comment #16
lauriiiI think you're right. We could just drop
LANGCODE_NOT_APPLICABLEfrom there. However, if an entity hasLANGCODE_NOT_APPLICABLEwe may still want to generate the link with the current language to retain the UI language. I'm fine if we move that to a follow-up to address this first.Comment #18
shabbir commentedHave fixed the tests and now i guess its in a mergeable format.
Comment #19
lauriiiComment #20
tuutti commentedWe've been using this for a couple of months now and it seems to work.
Comment #21
quietone commentedI'm triaging RTBC issues. I read the IS and the comments. The issue summary is incomplete, particularly there isn't a proposed resolution. I have added the standard template but it should at least have an updated 'proposed resolution'.
#16 mentions moving some work to a follow up. I am not sure if that is still needed but I have added the tag and adding it to the remaining tasks so that gets considered.
Leaving at RTBC.
Comment #22
longwaveOpened #3414318: Decide what to do with interface language for LANGCODE_NOT_APPLICABLE entities as followup. Plus, this already has tests, so removing the tag.
Comment #23
longwaveCommitted f028872 and pushed to 11.x. Thanks!
Unsure whether this is eligible for backport. One on hand it seems like a serious bug for the URLs that it affects, but on the other hand some sites may already be used to the new buggy behaviour. Will discuss with the other committers.
Comment #25
quietone commentedThe follow up was created.
Comment #26
quietone commentedI talked with longwave about this and he suggesting tagging for release notes.
Comment #27
longwaveLet's just mark this as fixed in 10.3.0, as there is a minor behaviour change let's not introduce it in a patch release. Anyone who is affected by this can patch, but with only 13 followers it seems this issue is not that widespread a problem.