nystudio107 / craft-retour

Retour allows you to intelligently redirect legacy URLs, so that you don't lose SEO value when rebuilding & restructuring a website
https://nystudio107.com/plugins/retour
Other
39 stars 26 forks source link

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

Open MoritzLost opened 7 months ago

MoritzLost commented 7 months ago

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.

Screenshot 2023-11-23 at 11 38 18

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

khalwat commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

MoritzLost commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

Screenshot 2023-11-28 at 17 55 37

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

Screenshot 2023-11-28 at 17 56 31

khalwat commented 7 months ago

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 commented 7 months ago

@khalwat That would be great, thanks Andrew!

MangoMarcus commented 7 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 2 months ago

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

khalwat commented 2 months ago

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