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.36k stars 2.64k forks source link

VariationContext not set correctly after upgrading from 13.3.0 #16617

Closed jemayn closed 1 week ago

jemayn commented 1 week ago

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

13.3.0 - 13.3.2

Bug summary

After upgrading to 13.3.1 and then 13.3.2 we've noticed on a site that the node we get in the FindContent method of an IVirtualPageController no longer gets the correct culture context.

Specifics

Happens from 13.3.1 and up on virtually routed pages, it works fine on regular Umbraco node routes.

I've uploaded a test repo that I've used to reproduce here if that is helpful: https://github.com/jemayn/culture-bug-umbraco

Steps to reproduce

If I have a structure like this in the backoffice on two languages where the names are different:

image

VirtualController:

using BugTesting.Models;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.ViewEngines;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.Web;
using Umbraco.Cms.Web.Common.Controllers;
using Umbraco.Cms.Web.Common.PublishedModels;

namespace BugTesting.Controllers;

public class VirtualProductController : UmbracoPageController, IVirtualPageController
{
    private readonly IUmbracoContextAccessor _umbracoContextAccessor;
    private readonly IPublishedValueFallback _publishedValueFallback;

    public VirtualProductController(
        ILogger<UmbracoPageController> logger,
        ICompositeViewEngine compositeViewEngine,
        IUmbracoContextAccessor umbracoContextAccessor,
        IPublishedValueFallback publishedValueFallback) : base(logger,
        compositeViewEngine)
    {
        _umbracoContextAccessor = umbracoContextAccessor;
        _publishedValueFallback = publishedValueFallback;
    }

    public IPublishedContent? FindContent(ActionExecutingContext actionExecutingContext)
    {
        if (!_umbracoContextAccessor.TryGetUmbracoContext(out var umbracoContext))
        {
            return null;
        }

        var rootContent = umbracoContext.Content?.GetAtRoot();

        var productPlaceholderPage = rootContent?.FirstOrDefault()?.Children(x => x.ContentType.Alias == ProductPlaceholder.ModelTypeAlias)?.FirstOrDefault();

        if (productPlaceholderPage is null)
        {
            return null;
        }

        return new ProductViewModel(productPlaceholderPage, _publishedValueFallback);
    }

    [Route("p/{sku}")]
    [Route("{lang}/p/{sku}")]
    public async Task<IActionResult> Index([FromRoute] string sku)
    {
        if (CurrentPage is not ProductViewModel productViewModel)
        {
            return NotFound();
        }

        productViewModel.Sku = sku;

        return View("~/Views/ProductPlaceholder.cshtml", productViewModel);
    }
}

Viewmodel:

using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Web.Common.PublishedModels;

namespace BugTesting.Models;

public class ProductViewModel : ProductPlaceholder
{
    public ProductViewModel(IPublishedContent content, IPublishedValueFallback publishedValueFallback) : base(content, publishedValueFallback)
    {
    }

    public string? Sku { get; set; }
}

View:

@model BugTesting.Models.ProductViewModel
@{
    Layout = "Master.cshtml";
}

<h1>Product sku: @Model.Sku</h1>

<p>Culture: @Model.GetCultureFromDomains()</p>
<p>Pagename: @Model.Name</p>

Then on 13.3.0 I get this result: image

And on 13.3.2 I get: image

So while it consistently can get the correct culture from the GetCultureFromDomains() method, the variationContext is wrong and it retrieves the english value for name and other properties.

It works if I add VariationContextAccessor.VariationContext = new VariationContext(Model.GetCultureFromDomains()); at the top of the view, but not too keen on adding that in multiple views 🙂

Expected result / actual result

I'd expect it to set the culture of the content to the current culture as it did before the change.

github-actions[bot] commented 1 week ago

Hi there @jemayn!

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:

nikolajlauridsen commented 1 week ago

Hey @jemayn, thanks a lot for the detailed description, but I'm having some issues running your test site, I'm getting some uSync error.

However, this sounds like the same issue as #16555 which should have been closed by #16572, which is actually out in RC2 now.

I'll go ahead and close this for now, however please do feel free to comment and I'll re-open it if this turns out to not be the case 😄

jemayn commented 1 week ago

@nikolajlauridsen that's great - I had a look for similar issues but I must have missed yours in the sea of v14 ones 😅

I tested on my project with 13.4.0 RC2 and it fixed the problem indeed, so guess we will just wait for that one 🙂

nikolajlauridsen commented 1 week ago

That's great, glad to hear it, fortunately, 13.4 is going out tomorrow, so you won't have to wait for long 😄

kasparboelkjeldsen commented 1 week ago

From someone who was just bitten hard by this, here is what I'm dying to know, and isn't obvious for me from the above - we only experience this in Edge and Brave and not in Chrome. How can localization hit differently on two browsers both in incognito on something I'd think would be a 100% serverside operation ?

-oh no, it's -not- fixed! Still a difference on edge and chrome on 13.4.0-rc2 !