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

404 stats overview always shows link relative to CP url, not to the site url #281

Closed lenvanessen closed 5 months ago

lenvanessen commented 1 year ago

Describe the bug

When you visit the Retour dashboard, all the URLS that are listed are relative path urls, and link to the CMS url. This can cause multiple issues, mostly when you have multi-site with different domains, or when you run your CP panel on a separate url from your site.

In our case, a user landed on: abc.com/404-page

It was stored as /404-page

So far, all good, but I'd expect Retour to use the site's base URL to construct the 404 link in the dashboard. Right now, it doesn't prefix any domain and our CP is running on abc.nl.

Versions

khalwat commented 1 year ago

Just to make sure I can reproduce and fix the issue, can you provide me with:

thanks!

lenvanessen commented 1 year ago

Hi @khalwat,

Here's the screenshot, at the top you can see I have selected "carvitas" for which the domain should be carvitas.com. I'd expect this link to use the domain for the selected site, but it's not, it just appends the 404 url to the cp panel url

Scherm­afbeelding 2023-06-27 om 16 52 41 Scherm­afbeelding 2023-06-27 om 16 54 05

furioursus commented 1 year ago

Can confirm that this is happening to me, as well. It seems to be an issue purely when the control panel URL is run from a different address.

khalwat commented 1 year ago

Interesting. So the code is in there already to determine whether it's an absolute URL or not:

      let absoluteUrl = new RegExp('^(?:[a-z]+:)?//', 'i');
      if (!absoluteUrl.test(url) && !url.includes('$')) {
        url = Craft.getSiteUrl(url);
      }

...but it looks like it's considering anything with a leading / to be an absolute URL.

khalwat commented 1 year ago

As it turns out, the issue is not with the RegEx as I first thought, but rather in the Craft.getUrl() JavaScript function (which is called by Craft.getSiteUrl()... if the path you pass in has a leading / it considers it an absolute URL and just returns it:

    // Return path if it appears to be an absolute URL.
    if (path.search('://') !== -1 || path[0] === '/') {
      return (
        path +
        (!$.isEmptyObject(params) ? `?${$.param(params)}` : '') +
        (anchor ? `#${anchor}` : '')
      );
    }

ref: https://github.com/craftcms/cms/blob/develop/src/web/assets/cp/src/js/Craft.js#L450

khalwat commented 1 year ago

Addressed in: https://github.com/nystudio107/craft-retour/commit/30bb435087fbb02ace8bbd67b94091eb3148d952 & https://github.com/nystudio107/craft-retour/commit/110433947c608bd349d235142bf7369f0278ca7d

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

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

Then do a composer clear-cache && composer update

lenvanessen commented 9 months ago

@khalwat for us this issue is not fixed. If switch the site selector to anything other than the main site, it will still link all links to the primary site. Even though you expect the selected site from the site-menu to be the domain name.

lenvanessen commented 9 months ago

Additional info, we don't use different domains for our CP. We use the primary domain for our CP (otherwise you get licencing issues and have to log-in for each site.

khalwat commented 5 months ago

This should be fixed now for all versions:

Craft 3: https://github.com/nystudio107/craft-retour/releases/tag/3.2.16

Craft 4: https://github.com/nystudio107/craft-retour/releases/tag/4.1.17

Craft 5: https://github.com/nystudio107/craft-retour/releases/tag/5.0.1