Automatically created redirects don't work if the base URL includes a path

[MoritzLost]

Describe the bug

We have two sites, German (base URL @web) and English (base URL @web/en). We use the option Create Entry Redirects to automatically create redirects when an entry's URL changes. However, when we change the slug on an entry in English, this doesn't work correctly, because the generated redirect doesn't include the /en in either the legacy or the destination path.

Probably related to #241

To reproduce

1. Set up two sites with the base URLs as described above. Activate the Create Entry Redirects option in the Retour settings.
2. Create an entry with slug foo (in a section where the URL is based on the slug) in the English site. Now that entry has the URL /en/foo.
3. Change the slug to foobar. Now the entry has the URL /en/foobar.
4. The automatically created redirect doesn't work, so going to /en/foo (the previous URL) shows a 404 page instead of redirecting to the new location.

Both the legacy URL and the destination are incorrect, because they don't include the /en segment.

Expected behaviour

Automatically generated redirects should always work correctly, by taking into account the site's base URL.

More broadly, I would prefer (and expect) if I could create redirects relative to the base URL. That would bring the plugin in line with core functionality. For example, section URl settings are always relative to the base URL. In other words, I would prefer if the redirect as shown above was the correct way to enter this redirect for the English site, and the matching engine automatically took the base URL into account. Though I understand this would be a big breaking change.

If that change isn't feasible, the automatically generated redirects need to be fixed so they are based on the actual URL, including any path segments of the site's base URL.

Versions

• Plugin version: 4.1.14
• Craft version: 4.5.11

[khalwat]

You need to set the Entry Redirects URL Match Type to Full URL for it to work as expected with your setup.

I generally agree with you that things should probably be relative to their base URL, but as you noted, it would be a breaking change. Also technically, the /en/ segment is part of the URL itself, even though as a Craft-ism we don't think of it that way.

[MoritzLost]

@khalwat Thanks for the clarification! We've activated that setting and it's working.

However, this feels more like a workaround. Having the URL in all redirects makes the list of redirects harder to scan and edit, and it also makes the redirects less portable. If we want to change the domain or move data between environments, the redirects won't work any more. I much prefer having redirects without the domain.

The current behaviour still feels like a bug. If I have that setting turned off, I still expect the automatically created redirects to work correctly, regardless of my base URL. I had a quick look, maybe Events::handleElementUriChange could be adjusted to use the full URL instead of the URI? And then either strip the domain or leave it, depending on the setting?

[khalwat]

I wouldn't classify it as a bug. But I'd agree it's less than ideal.

[MoritzLost]

@khalwat Hm, can something be done about it? How about my suggestion?

Not sure why this is not a bug to be honest. The two options (automatic entry redirects and full URL matching) have nothing to do with each other. From a user perspective, there's no reason those two options shouldn't work together. I don't want to presume how difficult this behaviour change would be to implement, but at least logically, I don't see any reason why it can't be done. Right now, it looks like the redirects just use the entry URI, which results in incorrect redirects if the base URL includes path segments. Instead of that, the redirects need to use the full URL, and then either include or leave out the domain depending on that setting. This change would also be backwards-compatible, since the current behaviour with these two options is broken, so nobody can be using.

[khalwat]

It's not a bug because it does work, it just isn't ideal.

It works the way it does because outside of the Craft world, URLs that include language segments, the /en/ or whatever prefix is actually part of the URI itself.

Often times people are importing redirects from other sources outside of Craft, perhaps they used to have them in their Nginx config or elsewhere, and they would include that prefix in there.

Additionally, there's a whole lot to be concerned about when changing something fundamental like this; the worst case is we break things for people's existing sites just to make things a little more logical from a UX perspective.

In general, I agree with you that some better way to do all of this is warranted. It just will not be trivial to balance the concerns mentioned above when doing so.

[MoritzLost]

@khalwat I feel like we're not talking about the same issue. Your answer seems to be in reference to my suggestion that redirects could be edited and evaluated relative to the base URL. As I said, I understand that this would be a major breaking change and is probably not worth it, since it would break a lot of existing redirects and workflows. I just included this as an idea of a possible solution in 'my ideal world'.

However, the thing I'm calling a bug is that the plugin creates incorrect redirects within the current mode of operation. The plugin operates on URL, not entry URIs, that's fine. But then it also needs to create redirects using full URLs. In my example above, I change a slug, which changes the URL of the entry from /en/foo to /en/foobar. However, Retour creates a redirect from /foo to /foobar. Neither of those URLs exist, and /en/foo still shows a 404 error instead of redirecting.

So I'm not asking to change anything about how Retour displays and matches redirects. I'm just asking to change the way it creates redirects for existing entries. Right now, if you have a site with a base URL that includes a path segment, and use the settings mentioned above, this functionality is broken. Fixing this issue would not be a BC break. As the functionality is currently broken for that combination of settings, nobody can use it like this. Or maybe they are and don't realize that their redirects aren't working, in which case fixing this bug would be a vast improvement.

To summarize, this is the redirect the Retour currently creates:

The function that creates this redirect when an entry is saved needs to be adjusted to it instead creates the redirect like this:

[khalwat]

Ah my bad, I was misunderstanding what you were saying. Apologies for that!

If you specifically are talking about the automatic redirects for entries only, I'll see if we can't do something about that.

[MoritzLost]

@khalwat That would be great, thanks Andrew!

[MangoMarcus]

We've run into the same bug with automatically added entry redirects.

If I change the a slug from contact to contact-us on our US site, Retour adds a redirect from /contact to /contact-us for the US site.

This doesn't work though because the full url including the site's base url is /us/contact-us.

The label for the "Legacy URL Match Type" field to me implies that it shouldn't need to include the base url:

Should the legacy URL be matched by path (e.g. /new-recipes/) or by full URL (e.g.: http://example.com/de/new-recipes/)?

So I suppose when Retour attemps to match the route/us/contact it should actually look for a static redirect of /contact under the relevant site (or "all sites"), not /us/contact

[svondervoort]

Subscribing to this issue since we are currently experiencing the same thing with automatically generated redirects on slug changes for a multisite (language) setup.

[khalwat]

Have not forgotten about this, it's on the list for the next update to Retour.

I've been working on porting Retour to Craft 5, which is now done. Thanks for your patience. https://github.com/nystudio107/craft-retour/releases/tag/5.0.0-beta.1

[denisyilmaz]

@khalwat do you know when/if this feature will be coming to Retour (for Craft CMS v4)?

[khalwat]

Yes, it will be backported to both Craft 3 and Craft 4 versions of Retour (as well as Craft 5).

[denisyilmaz]

@khalwat any updates when this feature is coming?

[khalwat]

This issue only affects the automatic redirects that are created when entry slugs are changed, which is a feature that can be disabled, and does not affect the core operation of the plugin.

That's why it hasn't been a high priority, but I agree, it should be addressed. It's next on the list, likely after I return from holiday (but possibly before).

[khalwat]

I fixed this at a higher level. What Retour does now is it will find the path prefix that is used by a Site's baseUrl and strip it from the incoming 404 path, and then use that in the rest of the redirect matching chain.

So for example, if the request URL is http://example.com/es/blog/this-does-not-exist, the actual path is /es/blog/this-does-not-exist but Retour will reduce it to /blog/this-does-not-exist and then do the redirect matching.

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-retour": "dev-develop as 3.2.20”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-retour": "dev-develop-v4 as 4.1.20”,

Then do a composer clear-cache && composer update

…..

Craft CMS 5:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-retour": "dev-develop-v5 as 5.0.4”,

Then do a composer clear-cache && composer update

[denisyilmaz]

@khalwat Thanks for that update. I tested this in a current Craft CMS 5 project but am a bit confused about the change.

I have the following setup:

Entry with two urls:

• de: /news/some-news
• en: /en/news/some-news

When querying for retourResolveRedirect via GraphQL I get the correct (empty) response as there are no redirects yet:

{
  deSite: retourResolveRedirect(uri: "/news/some-news", site: "de") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
  enSite: retourResolveRedirect(uri: "/news/some-news", site: "en") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
  enWithLangUriSite: retourResolveRedirect(uri: "/en/news/some-news", site: "en") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
}

Result:

{
  "data": {
    "deSite": null,
    "enSite": null,
    "enWithLangUriSite": {
      "redirectHttpCode": null,
      "redirectSrcUrl": null,
      "redirectDestUrl": null
    }
  }
}

When I change the english slug (!) now for that entry the result for the same query changes to this:

• de: /news/some-news
• en: /en/news/some-news-in-english

Result:

{
  "data": {
    "deSite": null,
    "enSite": {
      "redirectHttpCode": 301,
      "redirectSrcUrl": "/news/some-news",
      "redirectDestUrl": "/news/some-news-in-english"
    },
    "enWithLangUriSite": {
      "redirectHttpCode": null,
      "redirectSrcUrl": null,
      "redirectDestUrl": null
    }
  }
}

Which is the same behaviour as @MoritzLost mentioned in his initial issue. Maybe I misunderstood the changes you. made and they are only for the Craft CMS routing logic and not include any change to the GraphQL endpoint.

For me the expected behaviour for the GraphQL endpoint would still be like this:

retourResolveRedirect(uri: "/en/news/some-news", site: "*") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
    siteHandle
 }

{
  "data": {
    "retourResolveRedirect": {
      "redirectHttpCode": 301,
      "redirectSrcUrl": "/en/news/some-news",
      "redirectDestUrl": "/en/news/some-news-in-english",
      "siteHandle": "en"
    }
  }
}

[khalwat]

@denisyilmaz The change should trickle down to GraphQL as well. When you're passing in site is that the actual handle to the site? Because that's what Retour is expecting. Alternatively, you can use siteId and pass in the Site's ID.

The * as a parameter for site doesn't do anything, though.

Your query should be just this:

{
  deSite: retourResolveRedirect(uri: "/news/some-news", site: "de") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
  enSite: retourResolveRedirect(uri: "/news/some-news", site: "en") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
}

...because the created redirect will not contain the language prefix, but it will be created for just that site where the slug was changed.

Regardless, the response you're getting back isn't right (it should be including the path prefix). That's a flaw I just fixed and pushed.

Try the above again with the composer update and it will pull down the latest changes. Let me know how you go.

[khalwat]

I also just pushed a change to address concerns from @MoritzLost : it will now only check for the stripped path prefix URL if a redirect wasn't found that included it. This is to preserve backwards compatibility if you already have some Path Only redirects in place that include the Site path prefix.

[khalwat]

@MoritzLost @MangoMarcus @denisyilmaz @svondervoort lmk how you go with this... will cut a release only after it's working in your real-world scenarios.

[denisyilmaz]

@khalwat After updating and testing with the following query i got a "internal server error" from RetourResolver.php:
(note: without the lang prefix its working, but i guess this request should not throw anyway)

{
  retourResolveRedirect(uri: "/en/news/some-news") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
}

{
  "errors": [
    {
      "debugMessage": "Undefined array key \"redirectDestUrl\"",
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "file": "/var/www/html/vendor/nystudio107/craft-retour/src/gql/resolvers/RetourResolver.php",
      "line": 79,
      "trace": [
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/ErrorHandler.php",
          "line": 79,
          "call": "yii\\base\\ErrorHandler::handleError()"
        },
        {
          "file": "/var/www/html/vendor/nystudio107/craft-retour/src/gql/resolvers/RetourResolver.php",
          "line": 79,
          "call": "craft\\web\\ErrorHandler::handleError()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 623,
          "call": "nystudio107\\retour\\gql\\resolvers\\RetourResolver::resolve()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 549,
          "call": "GraphQL\\Executor\\ReferenceExecutor::resolveFieldValueOrError()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 1195,
          "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 264,
          "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 215,
          "call": "GraphQL\\Executor\\ReferenceExecutor::executeOperation()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/Executor.php",
          "line": 156,
          "call": "GraphQL\\Executor\\ReferenceExecutor::doExecute()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/GraphQL.php",
          "line": 161,
          "call": "GraphQL\\Executor\\Executor::promiseToExecute()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/GraphQL.php",
          "line": 93,
          "call": "GraphQL\\GraphQL::promiseToExecute()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/services/Gql.php",
          "line": 526,
          "call": "GraphQL\\GraphQL::executeQuery()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/controllers/GraphqlController.php",
          "line": 195,
          "call": "craft\\services\\Gql::executeQuery()"
        },
        {
          "call": "craft\\controllers\\GraphqlController::actionApi()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/InlineAction.php",
          "line": 57,
          "function": "call_user_func_array()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Controller.php",
          "line": 178,
          "call": "yii\\base\\InlineAction::runWithParams()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Module.php",
          "line": 552,
          "call": "yii\\base\\Controller::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 350,
          "call": "yii\\base\\Module::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 649,
          "call": "craft\\web\\Application::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 312,
          "call": "craft\\web\\Application::_processActionRequest()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Application.php",
          "line": 384,
          "call": "craft\\web\\Application::handleRequest()"
        },
        {
          "file": "/var/www/html/web/index.php",
          "line": 12,
          "call": "yii\\base\\Application::run()"
        }
      ]
    }
  ],
  "data": {
    "retourResolveRedirect": null
  }
}

[denisyilmaz]

@khalwat I tested your example query but got some weird result:

{
  deSite: retourResolveRedirect(uri: "/news/some-news", site: "de") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
  enSite: retourResolveRedirect(uri: "/news/some-news", site: "en") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
}

Both results use the "english" uri, i double checked and the german entry does not have the -english ending.

{
  "data": {
    "deSite": {
      "redirectHttpCode": 301,
      "redirectSrcUrl": "/news/some-news",
      "redirectDestUrl": "/news/some-news-in-english"
    },
    "enSite": {
      "redirectHttpCode": 301,
      "redirectSrcUrl": "/news/some-news",
      "redirectDestUrl": "/en/news/some-news-in-english"
    }
  }
}

Then i just queried for the deversion:

{
  retourResolveRedirect(uri: "/news/some-news", site: "de") {
    redirectHttpCode
    redirectSrcUrl
    redirectDestUrl
  }
}

which resulted in the same error from above:

{
  "errors": [
    {
      "debugMessage": "Trying to access array offset on null",
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "file": "/var/www/html/vendor/nystudio107/craft-retour/src/gql/resolvers/RetourResolver.php",
      "line": 79,
      "trace": [
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/ErrorHandler.php",
          "line": 79,
          "call": "yii\\base\\ErrorHandler::handleError()"
        },
        {
          "file": "/var/www/html/vendor/nystudio107/craft-retour/src/gql/resolvers/RetourResolver.php",
          "line": 79,
          "call": "craft\\web\\ErrorHandler::handleError()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 623,
          "call": "nystudio107\\retour\\gql\\resolvers\\RetourResolver::resolve()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 549,
          "call": "GraphQL\\Executor\\ReferenceExecutor::resolveFieldValueOrError()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 1195,
          "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 264,
          "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
          "line": 215,
          "call": "GraphQL\\Executor\\ReferenceExecutor::executeOperation()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/Executor/Executor.php",
          "line": 156,
          "call": "GraphQL\\Executor\\ReferenceExecutor::doExecute()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/GraphQL.php",
          "line": 161,
          "call": "GraphQL\\Executor\\Executor::promiseToExecute()"
        },
        {
          "file": "/var/www/html/vendor/webonyx/graphql-php/src/GraphQL.php",
          "line": 93,
          "call": "GraphQL\\GraphQL::promiseToExecute()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/services/Gql.php",
          "line": 526,
          "call": "GraphQL\\GraphQL::executeQuery()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/controllers/GraphqlController.php",
          "line": 195,
          "call": "craft\\services\\Gql::executeQuery()"
        },
        {
          "call": "craft\\controllers\\GraphqlController::actionApi()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/InlineAction.php",
          "line": 57,
          "function": "call_user_func_array()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Controller.php",
          "line": 178,
          "call": "yii\\base\\InlineAction::runWithParams()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Module.php",
          "line": 552,
          "call": "yii\\base\\Controller::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 350,
          "call": "yii\\base\\Module::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 649,
          "call": "craft\\web\\Application::runAction()"
        },
        {
          "file": "/var/www/html/vendor/craftcms/cms/src/web/Application.php",
          "line": 312,
          "call": "craft\\web\\Application::_processActionRequest()"
        },
        {
          "file": "/var/www/html/vendor/yiisoft/yii2/base/Application.php",
          "line": 384,
          "call": "craft\\web\\Application::handleRequest()"
        },
        {
          "file": "/var/www/html/web/index.php",
          "line": 12,
          "call": "yii\\base\\Application::run()"
        }
      ]
    }
  ],
  "data": {
    "retourResolveRedirect": null
  }
}

[khalwat]

@denisyilmaz fixed, sorry about that. Do composer update above again to get the latest fix.

[khalwat]

As for the entry not existing in another language, I'll double-check that with some testing tomorrow. It's strange that doing the query individually works (forget about the Exception error, that's been fixed), but seemingly does not work when done in tandem.

[MoritzLost]

@khalwat Just gave the updated version a try – thanks for the adjustment!

Unfortunately, there are breaking changes that will break our existing redirects.

I have a multisite with two sites, both using language prefixes in their base URL:

• German (domain.tld/de/)
• English (domain.tld/en/) (Primary site)

The live site has a lot of redirects that use absolute paths (including the language prefix), since that was the best way till now to set up those redirects.

Those redirects are now broken. Take this redirect:

Thanks to the fallback behaviour of matching the full path, the redirect still matches the path, but the target includes the language prefix twice:

❯ curl -I -k https://domain.test/en/foo
HTTP/2 301 
location: https://domain.test/en/en/foobar

So we would need to change all redirects to use the new syntax, and this client has ~400 redirects.

Redirects for all sites also don't seem to work correctly:

❯ curl -I -k https://domain.test/dienstleistungen/individual-entwicklung/
HTTP/2 301 
location: https://domain.test/en/de/service

The target is in the German site, but it's adding the English base URL to the target. In this case, it should just use the destination as-is.

---

Devil's advocate here: I know I suggested making redirects relative to the site's base URL, but it seems like there are a lot of edge-cases with this. If a redirect is added to All sites, it's unclear what the destination will be. It might also be confusing that all redirects and targets always start with /, even though in this case it's not the start of the path. Maybe it would be better to keep the previous behaviour and just adjusting the automatically created redirects to include the full URL?

Otherwise, the fallback behaviour would need to apply to both the source and the target path, so the language prefix is not duplicated, though I'm not sure how that would work. For redirects that are set to All sites, the redirect should probably not add any prefixes (or base URLs) to the target path.

[khalwat]

@MoritzLost Well, the issue regarding your "devil's advocate" section is that in addition to trying to solve this issue, we have people that are making redirects like this:

From: /some/url
To: /some/other/url

...and expecting that this will work site-wide on a multi-site setup with sites that have a language prefix. This is also honestly how I feel it should have worked to begin with.

The trick now is making it work whilst not breaking existing redirects like yours that tried to work around this original design decision.

[khalwat]

Okay @MoritzLost @denisyilmaz I think I have it worked out, I've tested the matrix of cases that you presented, and they are working.

If you could give it a whirl and do the composer update again to pull down the changes, that'd be awesome.

[samhibberd]

@khalwat tested on our Craft 4 install and working as expected 👍

[MoritzLost]

@khalwat Thanks for the update! I tested a bunch of existing redirects and most of them are working correctly now.

I found one issue if the Legacy URL Pattern ends in a trailing slash:

The redirect doesn't apply, regardless of whether I request the URL with or without the trailing slash:

❯ curl -I -k https://domain.test/de/produkte/
HTTP/2 404 

❯ curl -I -k https://domain.test/de/produkte
HTTP/2 404 

When I leave out the trailing slash in the Legacy URL Pattern (/produkte), it works correctly:

❯ curl -I -k https://domain.test/de/produkte
HTTP/2 301 
location: https://domain.test/de

❯ curl -I -k https://domain.test/de/produkte/
HTTP/2 301 
location: https://domain.test/de

Looks like trailing slashes don't work at all any more?

---

@MoritzLost Well, the issue regarding your "devil's advocate" section is that in addition to trying to solve this issue, we have people that are making redirects like this:

From: /some/url To: /some/other/url

...and expecting that this will work site-wide on a multi-site setup with sites that have a language prefix. This is also honestly how I feel it should have worked to begin with.

The trick now is making it work whilst not breaking existing redirects like yours that tried to work around this original design decision.

Yeah, definitely a tough problem! As far as I can tell, the redirect now tries to identify the target site from the redirect path, and merges the base URL with the target path? That's pretty cool.

One issue I can see is that a base URL is now always added to the target path. This works well for redirects for specific sites, but might be an issue for redirects for all sites. Take a redirect like this (for All sites):

• Old: /some-location
• New: /new-location

In the current released version, this works as expected:

❯ curl -I -k https://domain.test/some-location
HTTP/2 301 
location: https://domain.test/new-location

But in the dev version, it looks like the module falls back to the primary site's URL, so the redirect now goes to https://domain.test/en/new-location instead (English is the primary site in this project).

Not an issue in the project I'm currently testing, but might be a breaking change for some projects. There might be reasons to have a redirect that doesn't go to a particular site, e.g. if you have a path that's independent of Craft on your site and that doesn't use language prefixes. Maybe redirects for All sites could be handled differently, and not merge the base URL into the target path? Not sure if that would interfere with the other cases, though.

[khalwat]

@MoritzLost Thanks for testing it out!

I found one issue if the Legacy URL Pattern ends in a trailing slash:

I just tested this, and it's working as expected. So my guess is you might have a webserver rule of some kind (or potentially a Craft setting) that might be stripping the trailing slash from the request?

One issue I can see is that a base URL is now always added to the target path. This works well for redirects for specific sites, but might be an issue for redirects for all sites. Take a redirect like this (for All sites):

So it should be using the prefix for whatever site is handling the current request. It sounds like Craft is using the default site by default, which makes sense.

Unclear whether this will be an issue or not.

[MoritzLost]

@khalwat Thanks for checking, just tested this again, it only happens in a specific setup. In the following example, the base URL for the German (Deutsch) site is domain.test/de.

When I include the language prefix /de/ in the Legacy URL Pattern, it works:

❯ curl -I -k https://domain.test/de/produkte/
HTTP/2 301 
location: https://domain.test/de/test

But when I leave out the language prefix (but still set the Redirect to German), the redirect does not apply:

❯ curl -I -k https://domain.test/de/produkte/
HTTP/2 404 

Or is this the intended behaviour now?

So it should be using the prefix for whatever site is handling the current request. It sounds like Craft is using the default site by default, which makes sense.
Unclear whether this will be an issue or not.

Just looked into it again, it might be an issue for some use-cases. In our case, we have domain.test/de and domain.test/en for German and English, respectively. Visiting domain.test redirects to the primary site (English in this case). But some paths that I want to redirect (e.g. from the previous website) will not include a language prefix. In this case, Craft can't match the requested URL to a path, so it falls back to the primary site.

That makes it impossible to redirect from /some-location to /other-location without Retour appending a language prefix. I can redirect to /de/other-location, which will be used as is (does Retour figure out I want to go to the German site in this case? Pretty cool!). But I can't go to /other-location, because this will end up as /en/other-location. So it could mess with static routes, or other apps/systems that are served from a subdirectory. Not a super common use-case, but might be important enough to handle as a special case?

[khalwat]

Thanks for checking, just tested this again, it only happens in a specific setup. In the following example, the base URL for the German (Deutsch) site is domain.test/de.

@MoritzLost you're right, I found the place where it was stripping the trailing slash as part of the path normalization, which caused the redirect to not work. Fixed in the above commit, feel free to composer update nystudio107/craft-retour to give it a whirl!

That makes it impossible to redirect from /some-location to /other-location without Retour appending a language prefix. I can redirect to /de/other-location, which will be used as is (does Retour figure out I want to go to the German site in this case? Pretty cool!). But I can't go to /other-location, because this will end up as /en/other-location. So it could mess with static routes, or other apps/systems that are served from a subdirectory. Not a super common use-case, but might be important enough to handle as a special case?

I'm going to give a think on this, thanks!

[MoritzLost]

@MoritzLost you're right, I found the place where it was stripping the trailing slash as part of the path normalization, which caused the redirect to not work. Fixed in the above commit, feel free to composer update nystudio107/craft-retour to give it a whirl!

@khalwat Thanks, that worked!

Except for the issue regarding site-less redirects, everything looks good to go 👍