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

No events trigger when using exactMatch redirect #273

Closed JasonStainton closed 1 year ago

JasonStainton commented 1 year ago

Describe the bug

When using exactMatch redirects none of the resolve & resolved events gets triggered. All of the events are in the resolveRedirect() function of vendor/nystudio107/craft-retour/src/Retour.php and this doesn't get called with exactMatch redirects. It seems odd as the function is set up to handle exactMatch redirects and when I tweak the code to call existing methods it all seems to work as expected.

To reproduce

Steps to reproduce the behaviour:

  1. Create a redirect with the type exact match.
  2. Create a onEvent watcher for RedirectResolvedEvent on a custom module and just stick a dump/die in there.
  3. Navigate to the redirect URL.
  4. You are redirected without triggering the event.

Expected behaviour

The event should have triggered and returned the dd().

I've created a pull request of what I think it's meant to be doing if you want to give it a once-over. There's probably a reason it's not using resolveRedirect() that I am unaware of.

Versions

khalwat commented 1 year ago

Addressed in: https://github.com/nystudio107/craft-retour/commit/72af1f3e7a31a11bbc754f38bfe53893f51429e5 & https://github.com/nystudio107/craft-retour/commit/0f62cc8bb584ab7a9308b39bcb281d44b0d78b76

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.9",

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.11",

Then do a composer clear-cache && composer update

JasonStainton commented 1 year ago

Thanks for this, it was the other route I was thinking of but the code seemed like it was perfect not to be used 😄

@khalwat - I can see the before and after events but can't see the resolved event (which is the one I'm actually trying to target).

Thinking that one will need to sit in the findRedirectMatch() method to match how it's used in resolveRedirect()? Also, should that event trigger when a cached redirect is returned too? It has resolved to get a redirect after all.

I'll avoid the PR embarrassment this time 😅

khalwat commented 1 year ago

Ah my bad, I'll get that in there.

And no, those events shouldn't fire if they are cached.

khalwat commented 1 year ago

Addressed in: https://github.com/nystudio107/craft-retour/commit/8a2e4ed31a558de29223b09e735153756191a905 & https://github.com/nystudio107/craft-retour/commit/69b4a3e66cdc5ec471384cbe3cfba1ec2963d9a2

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.9",

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.11",

Then do a composer clear-cache && composer update