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
38 stars 26 forks source link

Potentially valid redirects being deleted #303

Closed BenParizek closed 3 months ago

BenParizek commented 4 months ago

Describe the bug

When adding a new redirect that redirects to the same URL as another redirect, the first redirect gets deleted.

To reproduce

  1. Create a functioning redirect from URL A to URL B.
  2. Add a new redirect from URL Z to URL A
  3. Observe the Redirect in #1 is now gone and only the #2 redirect from Z->A exists
  4. The redirect to URL B no longer exists

The code suggests it's cleaning up the first one to avoid a redirect loop, but in this case, it's not really a loop, just sloppy redirects.

Expected behaviour

Either:

  1. Both redirects work, resulting in two successive redirect hops by the browser
  2. Retour analyzes the chain and updates Z>A to become Z>B, resulting in one redirect hop by the browser
  3. Provide some warning message that prompts the user to realize the redirect chain and have the chance to decide the fate of URL B

Someone tells me that Retour used to behave something like #2 and thinks there may have been a change in behavior at some point.

Screenshots

Versions

khalwat commented 4 months ago

Which would be your ideal behavior here @BenParizek ?

BenParizek commented 4 months ago

The CMS author who called this out to me said they would expect:

khalwat commented 3 months ago

So I tested this by creating this redirect A (/foo) -> B (/bar):

Screenshot 2024-06-23 at 1 47 49 PM

Then I created a new redirect Z (/baz) -> A (/foo):

Screenshot 2024-06-23 at 1 49 10 PM

And it ends up creating a redirect from Z (/baz) -> A (/foo) that overwrites the first redirect A (/foo) -> B (/bar).

But what you want to end up with is Z (/baz) -> B (/bar) with it eliminating the original A (/foo) -> B (/bar) redirect ya?

khalwat commented 3 months ago

In the above commits, I removed the code that deletes redirects that have the redirectDestUrl of the redirect being saved as their redirectSrcUrl

So now both redirects will be in place, and work.

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.17”,

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.18”,

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.2”,

Then do a composer clear-cache && composer update