umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.49k stars 2.69k forks source link

Redirect URL management with manual CRUD in Umbraco 8 #5108

Closed runegronkjaer closed 1 year ago

runegronkjaer commented 5 years ago

We would like to make CRUD to the native Redirect URL management in Umbraco 8. It should be possible to manually create, update as well as delete entries.

We will supply the man hours for the feature. Only thing is, we don't wan't to put in the hours if it cannot become a part of the core afterwards.

How does that sound?

kjac commented 5 years ago

Sounds like an absolutely amazing idea if you ask me. Unfortunately I don't speak for HQ 😆

Suggestion: When creating and updating redirects it should be impossible to:

  1. Create circular redirects.
  2. Create redirects from an existing route.
runegronkjaer commented 5 years ago

@KennethJakobsen Great that you agree. We need this feature bad! How would those two suggestions work? It sounds like that would be an entirely new feature? This feature should only do CRUD on the existing features in my mind.

kjac commented 5 years ago

@runegronkjaer well... you're talking about manually creating new redirects and updating existing ones. So you to make sure the users can't setup redirects (either by creating or updating) that cause cyclic redirects in combination with the rest of the redirects.

Example: Umbraco has created a redirect from /mypage/ to /my-page/. The users should not be allowed to create a redirect from /my-page/ to /mypage/.

The users shouldn't be allowed to redirect from existing content either, as that would efficiently hide that content.

Example: Umbraco contains content on the route /my-page/. The users should not be allowed to create a redirect from /my-page/ to another route.

In principle the users shouldn't be allowed to create redirects unless the destination URL targets an existing page either.

Example: Umbraco contains no content on the route /mypage/. The users should not be allowed to create a redirect that targets /mypage/.

As a final remark, any redirects created or updated manually must be maintainable by the automatic redirect handling (as if they were created by the automatic redirect handling) to ensure we get no conflicts later on.

I hope it makes sense?

marcemarc commented 5 years ago

@runegronkjaer @kjac is it worth having a look at this PR? https://github.com/umbraco/Umbraco-CMS/pull/4265 and combining efforts with @Matthew-Wise ? Adding manual redirects to the core Redirect Url Management dashboard has been a thing for quite a while... http://tooorangey.co.uk/posts/301-redirection-in-umbraco-it-s-a-rum-do/

kjac commented 5 years ago

By the looks of it, #4265 is onto something good. If I'm reading that PR correctly one can ever only create redirects to existing content pages, thus eliminating the risk of circular redirects (since the target exists).

marcemarc commented 5 years ago

@kjac yes that was the key for HQ to consider it, eg they don't want the ability to be able create a redirect from 'any url' to 'any url' - but the idea to pick the content item, or to make the redirect from the context of the content item being edited, avoids the risks of crazy circular redirects, mainly due to the position of the RedirectUrlManagement ContentFinder in the ContentFinder queue. Anyway, the original PR that I did, 'got lost' in the pre-PR team days.. and Matthew has revived it! ... but be really great to see it get it in...

runegronkjaer commented 5 years ago

@kjac @marcemarc I can see you guys are talking about Umbraco 7. I should have been more specific that I/we need this feature for Umbraco 8 but it looks like the core of #4265 could be translated to U8.

I will update my original comment to Umb8.

Matthew-Wise commented 5 years ago

Thanks @marcemarc for pointing it my PR and all the pre work you did for it :)

@runegronkjaer the idea of any PR in v7 would get transferred to v8 especially if its a feature. As you say most of the code is transferable. So lets focus on getting the feature(s) correct in that PR before we move on to v8.

Matt

Matthew-Wise commented 5 years ago

As #4265 is now closed lets get a clear feature list that both HQ and the community can agree on:

I have complied a list of questions and some answers below:

Should this be part of the "info" content app or its own? Or something else?

What is a valid URL? Currently Anything that is a valid content url without a QueryString.

What access level needs to be required?

Personally I agree with @marcemarc "So if the editor has permissions to update and publish the page, this should grant them permissions to create a manual redirect."

I think once we have these questions answered we can get the basic's done.

Matt

Matthew-Wise commented 5 years ago

cc: @abjerner @andersburla @nul800sebastiaan as you commented on my PR and don't want to lose those minds :)

Nicholas-Westby commented 5 years ago

What is a valid URL?

Not sure I understand this question. Is this with regard to the source URL (where you redirect away from) or the destination URL (where you redirect to)? Or both?

If the source URL, I think virtually any URL should be allowed. Sometimes you are redirecting away from weird URL's on old sites.

If the destination URL, I think any URL that corresponds to a page, with an optional query string. The query string is useful if you want to do some tracking for analytics purposes (e.g., they go to /vanity, which redirects them to /some-page?utm_campaign=123), or if you want to redirect to a page with a specific filter site (e.g., /latest-articles redirects to /resources/blog?sort=latest).

Matthew-Wise commented 5 years ago

Sorry to clarify I was referring to the source URL the destination would be a content picker :)

hfloyd commented 5 years ago

So, just to clarify, you are suggesting that an editor should NOT be able to do something like: /some-page/ --> http://www.someothersite.com

I have seen clients who have moved a portion of their site to a third-party service (a jobs board, for instance) and they might have a valid use-case for something like that.

marcemarc commented 5 years ago

Hi @hfloyd yes, I think the idea is the core Redirect Url Management wouldn't cater for that particular use case - or at least not 'yet' ... the first baby step we're trying to get into the core, is for the ability for editors to create redirects to existing pages... which they have the ability to do at the moment, just by renaming the page to the redirect url and renaming it back again, this action creates two redirects - the editor then deletes the one they don't want - more here: http://tooorangey.co.uk/posts/301-redirection-in-umbraco-it-s-a-rum-do/ - so this shouldn't be controversial - the editors can do it now! and there is an agreement in the core team to pull this simple 'first step' in... but we've had two PR sit for a long time, for V7, (year and a half) = kinda stuck on the final UI, and how permissions might work... I believe it was just waiting a final nod from HQ, but the open issue on V7 has now been closed, and it's been encouraged to recreate for V8 - but as the final nod on the direction would still need to come from HQ... think @matthew-wise (who touched it last) is a little bit stalled... the idea I think with redirects to external resources would be to steer people towards the Skybrud Redirects package, or to create a redirect node in content tree... I don't think there is currently the appetite for a full 'all singing and dancing' redirects package maintained in the core... but that doesn't necessarily mean that one won't happen one day...

hfloyd commented 5 years ago

Thanks, @marcemarc for the detailed exposition.

I think that what I was suggesting is a smaller use-case, for sure, and your "workarounds" are totally valid. I just figured I'd put the suggestion out there explicitly so that everyone is on the same page about what the "v1" WILL and WILL NOT handle :-)

nul800sebastiaan commented 5 years ago

the idea I think with redirects to external resources would be to steer people towards the Skybrud Redirects package, or to create a redirect node in content tree... I don't think there is currently the appetite for a full 'all singing and dancing' redirects package maintained in the core...

Correct, when this is going to be used by content editors to set up redirects for incoming URLs we want it to be easy and safe. Redirects need to go to a content item in the tree, that can be picked by a content picker so we're sure it's linking to an existing node.

Additionally, adding query strings for tracking purposes does make sense. Could be an optional thing where the interface asks for a key and a value (which can be multiple key/value pairs). This is a nice-to-have though and can be added at a later time.

Is there any way we can help format the incoming URL?

Out of scope:

It's great for admins of the site to be able to add these types of redirects, but it is our opinion that these should exist as IIS rewrite rules in the web.config.

Sidenote: It's been suggested before but a very cool package for Umbraco would be to be able to edit the IIS rewrite rules from within Umbraco. For safety, that should probably only be possible for admins and only when debug="true" since.. hopefully on the live server this will not be the case and you should properly deploy a redirect rule, not just add rogue ones on live. Anyway.. let's not distract from the issue at hand.

Attackmonkey commented 5 years ago

As well as circular redirects, it would be good to handle redirect chains. For example.

This redirect is added: /old-page/ to /new-page/.

Later a redirect is added from /new-page/ to /even-newer-page/

We now have a chain if you go to /old-page/ which will redirect and then redirect again using the current logic. It would be better if the redirect manager picked up on the additional redirect and just followed the chain and redirected to the ultimate destination page in one redirect, or updated the old rules to point to the new page, removing the chain.

umbrabot commented 3 years ago

Hiya @runegronkjaer,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

Nicholas-Westby commented 3 years ago

@umbrabot still relevant The UI hasn't been altered to allow for creation or updates of redirects in Umbraco version 8.11.1.

runegronkjaer commented 3 years ago

@umbrabot still relevant +1

Zeegaan commented 1 year ago

As this is a feature request, I am moving it to our discussions board 😁