mikebridge / Liquid.NET

.Net Port of the Liquid template language
MIT License
62 stars 15 forks source link

Ignore static properties in LiquidValueConverter #69

Closed robertwesterlund closed 6 years ago

robertwesterlund commented 6 years ago

The current implementation of the LiquidValueConverter includes static properties from classes, which is probably not intended (at least it was not expected behavior when we tried using it).

Example class:

public class ViewModel
{
    public string Title { get; set; }
    public static string OtherData { get; set; }
}

Code sample:

ViewModel.OtherData = "aaa";
var liquid = new ViewModel() { Title = "bbb" }.ToLiquid();

Expected result:

After executing the code sample, the liquid object should only include the Title property.

Observed result:

After executing the code sample, the liquid object includes both the Title property and the OtherData property.

mikebridge commented 6 years ago

This makes sense, although it would be a breaking change. But I think it's unlikely that anyone would intentionally want to serialize a static variable, so this is probably a good idea. Thanks!

robertwesterlund commented 6 years ago

Almost any "bug fix" is a breaking change if you don't have a specification to refer to, since you're changing an inadvertant behavior that someone might possibly be using. :)

That being said, I completely agree with you. It is a breaking change, hopefully not actually breaking anyone since I hope that noone actually uses this behavior. If you are using semantic versioning and want to be really safe you could increase the major version, but that might be a bit too much for such a small change.

If you are uncomfortable with the breaking change, perhaps it could be a good idea to wait with accepting the pull request ( #70 ) until you're ready to increment the major version number based on other changes. If you decide for that, give me a ping just before you want to accept the pull request, so that I can update it if there are any changes needed based on other changes from now until then.

mikebridge commented 6 years ago

I think the change agrees with the general spirit of liquid, which is to reduce the number of unexpected/insecure values that show up in the context (ruby heritage). I'll merge this in when I get a few minutes. I need to publish a new update with dotnet core anyway.

mikebridge commented 6 years ago

@robertwesterlund deployed this update as v 0.10.0 to nuget along with netstandard2.0 support, thanks!