Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed url redirect creation issue #3837

Closed
wants to merge 3 commits into from

Conversation

bernardmoes
Copy link

@bernardmoes bernardmoes commented Dec 6, 2018

Issue: #3186
Previous PR: #3286

This pull requests resolves the issue of the wrong original URL being used for creating redirects when having implemented custom URL providers.

For the creation of redirects, the source of the original url was determined by the route property of the content. Using the url property instead makes the creation of redirects also compatible with custom url providers.

Tested using the custom url provider sebastiaan created when testing the previous pull request. Thanks @nul800sebastiaan

public class CustomUrlProvider : IUrlProvider
    {
        public string GetUrl(UmbracoContext umbracoContext, int id, Uri current, UrlProviderMode mode)
        {
            var content = umbracoContext.ContentCache.GetById(id);

            if (content != null && content.DocumentTypeAlias == "product")
            {
                return   content.Name.ReverseString().ToUrlSegment().EnsureStartsWith("/");
            }

            return null;
        }

        public IEnumerable<string> GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current)
        {
            var content = umbracoContext.ContentCache.GetById(id);

            if (content != null && content.DocumentTypeAlias == "product")
            {
                var urls = new List<string>();

                urls.Add(content.Name.ToUrlSegment().EnsureStartsWith("/"));

                return urls;
            }

            return null;
        }
    }

    public class CustomUrlContentFinder : IContentFinder
    {
        public bool TryFindContent(PublishedContentRequest contentRequest)
        {
            if (contentRequest == null)
                return false;

            var uri = contentRequest.Uri;

            var segments = uri.Segments.Where(x => x != "/").ToList();

            if (segments.Any() && segments.Count == 1)
            {
                var urlPart = segments.First();

                var products = UmbracoContext.Current.ContentCache.GetByXPath("//product[@isDoc]").ToList();

                var matchedProduct = products.FirstOrDefault(x =>
                    x.Name.ReverseString().ToUrlSegment() == urlPart || x.Name.ToUrlSegment() == urlPart);

                if (matchedProduct != null)
                {
                    contentRequest.PublishedContent = matchedProduct;
                    return true;
                }
            }

            return false;
        }
    }

    public class UrlProviderRegistration : ApplicationEventHandler
    {
        protected override void ApplicationStarting(UmbracoApplicationBase umbracoApplication, ApplicationContext applicationContext)
        {
            ContentFinderResolver.Current.InsertTypeBefore<ContentFinderByNotFoundHandlers, CustomUrlContentFinder>();
            UrlProviderResolver.Current.InsertTypeBefore<DefaultUrlProvider, CustomUrlProvider>();
        }
    }

Copy link
Contributor

@dawoe dawoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bernardmoes

Thanks for another attempt on fixing the issue. Here is what I found during testing

  • Redirects created before this fix appplied still work
  • Redirects are created correctly for pages with the a custom content finder example
  • Redirects created with this fix applied for pages without custom content finder don't redirect anymore. I found out that this because they are created with a trailing / in the database. This because addTrailingSlash is set to true in umbracoSettings.config. Redirects created with this setting to false are getting redirected
  • Setting useDomainPrefixes to true in umbracoSettings.config will create the redirect with the absolute url in the db. The redirect does not work. And the dashboard and info tab throw errors in the backend.

Despite the issues I think we are actually pretty close

  • Redirects need to be created without trailing slash
  • Redirects should be created with a relative url

Could you have a look at that

Dave Community Pull Request team

@dawoe dawoe self-assigned this Dec 7, 2018
@bernardmoes
Copy link
Author

@dawoe Thanks for your review. Somehow didn't notice those issues during my testing. I just commited some changes which should resolve all the issues you mentioned. I thoroughly tested this time:

  • All previously made redirects are still working
  • Formatting of the old url in the database is consistent now. Always relative and without trailing slash.
  • Works with and without custom url providers
  • Tested different domainprefix and trailingslash settings

Looking forward to your approval

Copy link
Contributor

@dawoe dawoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bernardmoes

I tested your changes and now everything works as expected.

Thanks for this PR

Dave Community Pull Request team

@dawoe dawoe assigned nul800sebastiaan and unassigned dawoe Dec 10, 2018
@zpqrtbnk zpqrtbnk changed the base branch from dev-v7 to v7/dev March 31, 2019 16:33
@bernardmoes
Copy link
Author

@nul800sebastiaan Why is my pull request being ignored? It's been here for over 6 months and no reply, not comment, no nothing.

@nul800sebastiaan
Copy link
Member

Very sorry @bernardmoes - love the idea of this PR but it kinda fell of my radar after a while 🙈

Please note we've been having a bigger focus on v8 for the past few months so v7 has been getting lower priority.

I need some help from others at HQ to make sure this is the correct fix and it's not going to break anything for anyone.

It's on the todo list and I'll get back to you (it will take a bit of time though).

@nul800sebastiaan
Copy link
Member

As an update: I've talked to @zpqrtbnk about this and he has concerns about the way this is being fixed and it potentially being a breaking change. More feedback to come.

@umbrabot
Copy link

Hiya @bernardmoes!

Thanks for creating this pull request. It might have been a while since you heard from us since a lot of things are happening at the moment.

Right now, we're in the process of building v9 and we now shifting our focus to exclusively work on v8 and v9. From now on Umbraco 7 is in security mode, which means we will no longer release accept new issues nor pull request unless they are severe security issues. You can read all about this in our long term support and end of life policy.

This means that this PR will be closed and no longer looked at by the team. We realize this might be disappointing and we do apologize for not coming to a proper conclusion on this one. We hope you understand that we need to focus our active development somewhere and in that means our efforts will go to the current and the future versions of Umbraco.

If it makes sense to look at this still for version 8, or even version 9, we'd love it if you reported it as a new issue so it can get the proper attention.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants