Problem/Motivation

Core does not always check if external URL's are local (using UrlHelper::externalIsLocal) which has varied consequences. We can avoid adding the base_path to outbound URL's by checking if the language negotiation path is the same as the current path and avoid some problems.

See #3424701: Domain-based language negotiation should retain "destination" URL query argument for full context.

Steps to reproduce

  1. Setup two domains (.ddev/config.yaml additional_hostnames, I used "local" and "es.local").
  2. Install Drupal, enable media and language modules.
  3. Add a language for the second domain on admin/config/regional/language (I used Spanish). Configure detection on admin/config/regional/language/detection, edit "URL", select "Domain", and set the two domain URL's (I used local.ddev.site and es.local.ddev.site)
  4. Add a media document from admin/content/media
  5. Visit admin/content/files and hover over the "Delete" link. Notice that it uses the full URL.

Proposed resolution

Add an additional check in LanguageNegotiationUrl::processOutbound() that returns early if the base_url's match.

Issue fork drupal-3424720

Command icon Show commands

Start 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:

About issue forks

Comments

douggreen created an issue. See original summary.

douggreen’s picture

douggreen commented
Issue summary: View changes

douggreen opened merge request !6833

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.2.x-dev » 11.x-dev

Thanks for reporting this!

The merge request needs to be created against 11.x (our main development branch); it will be backported to supported versions once committed to 11.x. In some cases the easiest thing to do is close the previous merge request and create a new one against 11.x. Thanks!

immaculatexavier made their first commit to this issue’s fork.

immaculatexavier opened merge request !8326

immaculatexavier’s picture

immaculatexavier commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

vidorado made their first commit to this issue’s fork.

vidorado’s picture

vidorado commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I've added test coverage.

As a bonus, it turns out that we've fixed, at least partially, a bug stated in #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error, so I have had to change a line in BlockUiTest too.

smustgrave closed merge request !6833

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left 1 small comment on MR

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

vidorado’s picture

vidorado commented
Status: Needs work » Needs review

Thanks! I've replied to your comment in the MR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Been about a month without additional review. Will RTBC but leaving the one thread open.

sleitner made their first commit to this issue’s fork.

uri_frazier’s picture

uri_frazier
he/him
Primary language English
Location Portland, Oregon
commented

I'm not sure if this belongs here, or if there needs to be a it's own (new) issue:
I think this fix should apply to the actual domain as well, and not just the destination URL/domain.

Context:
I have a multilingual site that uses sub-domains (and domain-based language negotiation). I want admin users to be able to delete files
The "delete" link that is generated uses the English domain (e.g. website.com/file/123/delete) instead of the sub-domain (e.g. japan.website.com/file/123/delete) the user is currently logged into and clicked the original "delete" link from. So when clicking "delete", it tries to take users cross-domain to complete the process, but stops them with an "access denied" screen since they are not logged in under the English (default) domain.

This is also the same behavior/problem that occurs when trying to use the file_replace module and it's "replace" link.

  • catch committed c92b8429 on 10.5.x 
    Issue #3424720 by vidorado, douggreen, immaculatexavier, smustgrave,...

  • catch committed b089f9fb on 11.x 
    Issue #3424720 by vidorado, douggreen, immaculatexavier, smustgrave,...

  • catch committed 620d8896 on 11.1.x 
    Issue #3424720 by vidorado, douggreen, immaculatexavier, smustgrave,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

@uri_frazier I think that's probably better handled in its own issue - this is already fixing the reported bug (and #2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error was since closed as duplicate, so it's fixing two bug reports even if the same root cause), and it will be easier to review further changes against the new state of the code rather than changing things further here. If you could open that issue and link it back to here that would be great.

Committed/pushed to 11.x and cherry-picked to 11.1.x and 10.5.x, thanks!

  • longwave committed c0199541 on 10.5.x 
    Revert "Issue #3424720 by vidorado, douggreen, immaculatexavier,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 10.5.x-dev » 11.1.x-dev

Reverted this from 10.5.x as it caused a failure in BookMultilingualTest.

Unsure if this is a problem with just the test or a problem with the patch, this wasn't spotted in 11.x because book.module has moved to contrib.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.

xjm’s picture

xjm
she/her
Primary language English
commented

And fixing Dave's bot-eaten credit for the revert.