CrumpledDog / Umbraco-AzureCDNToolkit

Makes it easy to fully utilise the Azure CDN with a Umbraco website
13 stars 14 forks source link

Property Value Converter Error - IHtmlString #8

Open uniquelau opened 6 years ago

uniquelau commented 6 years ago

When AzureCDNToolkit:UseAzureCdnToolkit is set to true, I am seeing an error when the property type value converter.

[NullReferenceException: Object reference not set to an instance of an object.] Our.Umbraco.AzureCDNToolkit.ValueConverters.RteValueConverter.ConvertDataToSource(PublishedPropertyType propertyType, Object source, Boolean preview) +929 Umbraco.Web.PublishedCache.XmlPublishedCache.XmlPublishedProperty.get_Value() +58 Umbraco.Web.PublishedPropertyExtension.GetValue(IPublishedProperty property, Boolean withDefaultValue, T defaultValue) +73 Umbraco.Web.PublishedContentExtensions.GetPropertyValue(IPublishedContent content, String alias, Boolean recurse, Boolean withDefaultValue, T defaultValue) +82 Umbraco.Web.PublishedContentExtensions.GetPropertyValue(IPublishedContent content, String alias) +59 project.Logic.Model.Content.Article.get_ContentBody() in C:\git\project\Src\project.Logic\Model\Content\Article.cs:33

The Model looks like:

public IHtmlString ContentBody { get { return this.GetPropertyValue<IHtmlString>("contentBody"); }

When CDN tool kit is disabled the error goes away!

Umbraco 7.6.5 AzureCDNToolkit 0.2.0-alpha-000054 AzureCDNToolkit.Core 0.2.0-alpha-000050

idseefeld commented 5 years ago

@uniquelau I tried to reproduce this with a recent Umbraco version 7.12.3. I have added an default RTE property editor (alias = "contentBody") - images allowed. Umbraco.ModelsBuilder.ModelsMode is set to Dll. Content is set to some text and one image. I tested @Model.Content.ContentBody and @Model.Content.GetPropertyValue("contentBody") in the template. Both, Text an Image are shown as expected and no error occurred. Thus I guess this could be closed - right?

uniquelau commented 5 years ago

@idseefeld Looks promising, I will re-test within that solution, and then I'll close the ticket. Many thanks, Laurence

opc-clienti commented 5 years ago

@uniquelau Did you find a solution for this? I just installed the toolkit for the first time, and I'm seeing this too.

uniquelau commented 5 years ago

Hmm, I will check, which version of the tool kit are you using? And where does the value come (e.g. ModelsBuilder DLL, Ditto, etc)

opc-clienti commented 5 years ago

Umbraco 7.12.4 AzureCDNToolkit 0.2.0-alpha-000062 AzureCDNToolkit.Core 0.2.0-alpha-000050

The value comes from ModelsBuilder (PureLive). Thanks.

uniquelau commented 5 years ago

Ok, so I have found my solution that contains the behaviour.

I re-enabled the CDN, and can recreate the issue.

The same error occurs when I try to get a property on a specific page:

var prop = this.GetPropertyValue<IHtmlString>("contentBody");
return prop;       

I quickly tried setting the property manually, to see if that would help?

var x = new HtmlString("Will this work");                            
return x;            

And the answer is yes! The template will then render with no errors.

I'll have a look in more detail tomorrow, but my bet would be that a property has been added to the document type since it was published, so it cannot return the property value being requested ,and then it is blowing up.

But I'll will step through, and see what is actually going on tomorrow.

Failing that I've got a similar solution that does work, so we could compare them.

opc-clienti commented 5 years ago

The error only happens if the HtmlString contains an umbraco image, so that's why you don't get an error when setting the property manually.

If I understand correctly, the toolkit is trying to convert all umbraco images in RTE to absolute cdn urls. And in this logic, something is failing.

I'm using the toolkit on Umbraco Cloud, and it's worth noting that the AzureCDNToolkit tab in the developers sections shows no servers. So multiple things are broking in my scenario. The ResolveCdnUrl and GetCropCdnUrl are working fine though. I'll try reinstalling the package to see if that does any good.

The worst thing for me, is that I can only test this on the live environment, since the development environment is hidden behind ip blocking. The HEAD request the toolkit is sending to Azure probably never returns because of this blocking.

opc-clienti commented 5 years ago

After some hours of investigating I think I've found the root of the issue. The Umbraco site I'm working on is a couple of years old, and back then the RTE formatted images in another way. All the pages that are failing contains images like this: <img src="/media/1111/image.jpg?width=400&height=308" data-id="0"/>. It's the data-id part that makes them fail. If I remove the whole data-id attribute, everything works fine.

In RteValueConverter.cs I see that data-id is used in like 48, but as far as I can see, all the code is proper null checked, so I don't know what is breaking.

uniquelau commented 5 years ago

@opc-clienti Thank's for posting your findings.

In the end, I resolve the issue via brute force.

I completed a find and replace in the data, and change data-id to _data-id, and then everything works.

I tried stepping through and had a similar issue as you describe above, the null check doesn't seem to catch the null.

OlePc commented 5 years ago

@uniquelau That's one way of doing it. I ended up cloning the source code and making a customer build with a fix for the missing null check. I could make a PR, but I'm not sure how active the project is, and if it's ever getting merged. But since I installed the package through nuget, it tries to replace my custom .dll everytime I build the project. That's not good either.