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.5k stars 2.69k forks source link

Rich text link picker not resolving correct variant when using .ValueFor() #13865

Closed joelmandell closed 1 year ago

joelmandell commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.2.1

Bug summary

Hi. I have a server prerendered Blazor components and I'm experiencing some problems with fetching content based on different cultures. The problem seems to be with the IVariationContextAccessor that is in the PublishedValueFallback, that is passed on to this.Value() in the generated models.

Case: Page is starting out in swedish language (default/fallback). Switch to english and getting properties from content.

On subsequent calls the language of the VariationContext is changed from "en" to "sv", even if parameter "en" is passed as parameter to GetValue() on IPublishedContent prop. I am not sure why this occurs, but browsing around the code and debugging, it seems to be because of caching (IRequestCache) of VariationContext (HybridUmbracoContextAccessor) on the PublishedValueFallback object. To further specify the issue... it succeeds to fetch the content from a richtext-property according to CurrentCulture, but if it links to an english page it picks the swedish page instead.

In order to get away with that problem I am implementing my own "T? Value", in the same namespace as the generated models (so that method is called instead of the one in PublishedElementExtensions). Using reflection I replace the VariationContext in PublishedValueFallback with a new one according to CultureInfo.CurrentCulture.TwoLetterIsoLanguage (that is changed by user selecting english instead of swedish) (and only if it is different then CurrentCulture)

I am available to respond to questions regarding this issue and clarifications. Without knowing for sure what the issue is, I am proposing an potential change: what if checking CurrentCulture against cached VariationContext. If they are not the same, update the cached VariationContext with a new one based on CurrentCulture?

Specifics

I might include an video at a later step here that take you through the steps (i.e. debugging and so on...).

Steps to reproduce

You have to have content in multiple languages. Setup two languages (i.e. danish and english). Have danish as default/fallback. A Settings-node with a Richtext (with links) property with vary by culture support.

Also add one Document Type as a page with vary by culture support (a simple view that prints out the richtext from settings-node).

Have a Server Prerendered Blazor Component that is rendered in the View for the simple start view/page.

Add content (with danish text) to the property on the settings node in danish first. Create a link to the danish page in that richtext. Set localization content for english on that property. Create a link to the english page in that richtext.

For example create a blazor component with two buttons, one for danish and one for english. In the Blazor component have it render a string for example div>@MyLocalizedSetting</div.

In the callback for danish button Set CurrentCulture to "da" and assign the settings property to MyLocalizedSetting. The text will be shown in danish and the link will be to the danish page.

In the callback for english button Set CurrentCulture to "en" and assign the settings property to MyLocalizedSetting. The text will be shown in english but the link will be to the danish page.

Expected result / actual result

Link in rendered text should be to the english page but is instead to the danish page (i.e. insert random language)...

github-actions[bot] commented 1 year ago

Hi there @joelmandell!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

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

joelmandell commented 1 year ago

I haven't delved deeper in the code base of Umbraco, but it seems to have with the sequence somewhere when picking {localLink:umb//document...}. Because the title of the a-tag is rendered according to the localization, but the link is pointed to the default/fallback-language. But it seems anyway to be connected with an cached VariationContextAccessor in the IRequestCache... πŸ€”

joelmandell commented 1 year ago

Here is a visual repro of the issue:

https://user-images.githubusercontent.com/109020/220210886-3b0f9e30-9656-4855-989e-7ebbcdc50090.mp4

Zeegaan commented 1 year ago

@joelmandell Great issue description, and love the video it really helped πŸ’ͺ This issue has already been reported here: https://github.com/umbraco/Umbraco-CMS/issues/12627 I will go ahead and close this for now, please feel free to re-open if you feel that is not the exact same issue 😁

joelmandell commented 1 year ago

Thank you @Zeegaan. The issue reported here seems to be of a different nature. The url that is picked (the generared localLink) is actually working and does not give 404. The issue is that it points to incorrect localized version.

If you have any hint where to start in the Umbraco codebase, I might debug and create a PR if so.

Zeegaan commented 1 year ago

@joelmandell I will have a quick look around then, I will report my findings πŸ•΅οΈβ€β™‚οΈ

Zeegaan commented 1 year ago

@joelmandell I am having trouble reproducing this, I might need that blazor components of yours πŸ€” Can you send me the component πŸ™ˆ LocalizedLinksWorksWIthoutBlazor

joelmandell commented 1 year ago

Yes. Here it is:

@using System.Globalization;
@using BlazorTest.Generated;
@using Umbraco.Cms.Core.Scoping;
@using Umbraco.Cms.Core.Web;
<h3>LanguageSwitcher</h3>

<button @onclick="SetSwedish" >Swedish</button>
<button @onclick="SetEnglish">English</button>
Content: @Content
<div>
    @((MarkupString)Content)
</div>
@code {
    [Inject]
    private IUmbracoContextFactory UmbracoContextFactory { get; set; }

    [Inject]
    private IScopeProvider ScopeProvider { get; set; }

    public string Content { get; set; }

    protected override void OnInitialized()
    {
        Content = "Initialized";    
    }

    private void SetSwedish()
    {
        Content = "Swedish";
        var sv = new CultureInfo("sv");
        CultureInfo.CurrentCulture = sv;
        CultureInfo.CurrentUICulture = sv;

        using (var scope = ScopeProvider.CreateScope(autoComplete: true))
        {
            using (var context = UmbracoContextFactory.EnsureUmbracoContext())
            {
                var settings = (Settings)context.UmbracoContext.Content.GetAtRoot().ElementAtOrDefault(1);
                Content = settings.Body.ToHtmlString();
            }
        }
    }

    private void SetEnglish()
    {
        Content = "English";
        var en = new CultureInfo("en");
        CultureInfo.CurrentCulture = en;
        CultureInfo.CurrentUICulture = en;

        using (var scope = ScopeProvider.CreateScope(autoComplete: true))
        {
            using (var context = UmbracoContextFactory.EnsureUmbracoContext())
            {
                var settings = (Settings)context.UmbracoContext.Content.GetAtRoot().ElementAtOrDefault(1);
                Content = settings.Body.ToHtmlString();
            }
        }
    }
}
joelmandell commented 1 year ago

@Zeegaan In your case (because I am "overriding" GetValue() that the generated models use accessing props) you might need to change settings.Body (or whatever your model prop are named) to use instead: Content = settings.ValueFor(x => x.Body, CultureInfo.CurrentCulture.TwoLetterISOLanguageName).ToHtmlString();

joelmandell commented 1 year ago

@Zeegaan πŸ‘‹ Here you have a full solution with a .bak-file of the db. ConnectionString in appsettings is set to a dummy server.

Username for /umbraco is joel.mandell [at] sublime.se Pw: !HowIsYourBlazorExp3rienceWellItDepends

UmbracoWithBlazor.zip

To add to my confusement. In the blazor component there is 4 buttons. If I click "English Umbraco Default" first and then click "English GetValue", my "fix" does not work. But If I first click "English GeValue" and then click "English Umbraco Default" it shows the correct href.

If it will help you to fully understand the issue, I am available for a video call, to share screen and talk about this in detail.

Zeegaan commented 1 year ago

@joelmandell Thanks for the details, I will start looking into this tomorrow 😁 Just wondering, what .NET version are you running ? πŸ€”

joelmandell commented 1 year ago

@Zeegaan Thank you. The sample in the zip is .NET7. But I have a project using .NET6 with this issue also.

Zeegaan commented 1 year ago

Alright so after looking into this, I was finally reproduce this issue, huge thanks @joelmandell for the details & the test project πŸ’ͺ πŸŽ‰ The issue here is that the link picker for the rich text editor just stores the link as a document reference: image What then happens when we use .ValueFor(x => x.VariantRichText, CultureInfo.CurrentCulture.TwoLetterISOLanguageName); Is that the link does not resolve to the correct culture, so that's where i would start debugging inside when looking for a fix 😁

I will take the liberty to rename this issue, as its really a bug with the Rich text link picker, and doesn't have anything to do with blazor πŸ˜…

How to replicate this issue

namespace Umbraco.Cms.Web.UI;

public class MyNotificationHandler : INotificationHandler { private readonly IUmbracoContextFactory _umbracoContextFactory;

public MyNotificationHandler(IUmbracoContextFactory umbracoContextFactory)
{
    _umbracoContextFactory = umbracoContextFactory;
}

public void Handle(ContentSavedNotification notification)
{
    using (var context = _umbracoContextFactory?.EnsureUmbracoContext())
    {
        var settings = (Root?)context?.UmbracoContext?.Content?.GetAtRoot().Last();
        var something = settings?.ValueFor(x => x.VariantRichText, "da");
    }
}

}

public class Mycomposer : IComposer { public void Compose(IUmbracoBuilder builder) => builder.AddNotificationHandler<ContentSavedNotification, MyNotificationHandler>(); }


- Set a breakpoint at `var something`
- Run the site, go save some content
- Hit the breakpoint and F10 once to populate the something variable
- See that the something variable contains a link to the "/" and not "/da"
github-actions[bot] commented 1 year ago

Hi @joelmandell,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

nul800sebastiaan commented 1 year ago

Sorry about that extra prompt to say this is up for grabs, my bad! 🀦 Will look at your PR soon!

Zeegaan commented 1 year ago

Fixed in https://github.com/umbraco/Umbraco-CMS/pull/13889 πŸš€