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

In the Core version, there doesn't seem to be a GetPropertyValueType method #23

Open
zpbd opened this issue Jun 13, 2017 · 3 comments
Open

Comments

@zpbd
Copy link

zpbd commented Jun 13, 2017

Hi. After upgrading to Umbraco 7.6.2 and removing the package, it became clear that the virtual Type GetPropertyValueType(PublishedPropertyType propertyType) property is no longer a thing.

We were using this in a MultiNodeTreePickerConverter similar to the example here (Example 1 step 2).

public override Type GetPropertyValueType(PublishedPropertyType propertyType)
{
	if (IsPropertySpecific(propertyType, _pageTilePropertyAliases))
	{
		return typeof (IEnumerable<PageTile>);
	}

	if (IsPropertySpecific(propertyType, _featuredNewsArticlesPropertyAliases))
	{
		return typeof(IEnumerable<NewsArticlePage>);
	}

	return base.GetPropertyValueType(propertyType);
}

We had modified this example so that there was actually a dictionary of aliases and types, so we had a nice quick way of hooking up new MNTP properties.

I'm aware that you can use an attribute on the class now to control the outputted type. But is there a way of continuing to do this without having to create lots of individual converters for each type of MNTP implementation?

@Jeavon
Copy link
Owner

Jeavon commented Jun 13, 2017

@zpbd to continue to work in this way you can continue to use this package, it will work for all non "v2" editors, if you are using "v2" UDI editors you would need to create your own version of the "v1" editors.

I need to have a discussion to see if we can get the methods in Umbraco turned back to being virtual as I think this is a good reason...

@zpbd
Copy link
Author

zpbd commented Jun 13, 2017

Looks like we will keep using the v1 editors and the package for now then as I like the solution we have.

Thanks for the lightning reply!
Zac

@zpbd
Copy link
Author

zpbd commented Jun 15, 2017

Hi again Jeavon, I have a performance question that I was hoping you would have some insight on.

So coming back to this, I decided it would be better for the long term to try and use the v2 MNTPs with the core. I settled on a way of doing it so that I essentially only need to add two lines of code for each of the alias / type combinations of which there may be many:

[PropertyValueTypeAndAlias("cat", typeof(CatPage))]
public class MultiNodeTreePickerConverterCat : AbstractMultiNodeTreePicker2Converter { }

[PropertyValueTypeAndAlias("cats", typeof(IEnumerable<CatPage>))]
public class MultiNodeTreePickerConverterCats : AbstractMultiNodeTreePicker2Converter { }

in combination with a new attribute that inherits from PropertyValueTypeAttribute

[AttributeUsage(AttributeTargets.Class, Inherited = false)]
public class PropertyValueTypeAndAliasAttribute : PropertyValueTypeAttribute
{
   public PropertyValueTypeAndAliasAttribute(string alias, Type type) : base(type)
   {
        Alias = alias;
   }
   public string Alias { get; }
}

and an AbstractMultiNodeTreePicker2Converter that checks this attribute alias:

public abstract class AbstractMultiNodeTreePicker2Converter : MultiNodeTreePickerPropertyConverter
{
    public override object ConvertSourceToObject(PublishedPropertyType propertyType, object source, bool preview)
    {
        ... code to create the typed data
    }

    public override bool IsConverter(PublishedPropertyType propertyType)
    {
        return propertyType.PropertyEditorAlias == "Umbraco.MultiNodeTreePicker2" && propertyType.PropertyTypeAlias == GetType().GetCustomAttribute<PropertyValueTypeAndAliasAttribute>(false).Alias;
    }
}

This appears to work - ModelsBuilder is generating properties with the correct types and I am able to retrieve the values. But would there be any extra load from having, say, a dozen MultiNodeTreePickerPropertyConverter implementations (one for each alias/type)? Does Umbraco need to run IsConverter against every single property when retrieving the data in order to determine which property converter to use, or is this done once in the application and cached somehow so that it knows which converter to use?

With the previous setup, it was a simple dictionary lookup regardless of how many alias/type combinations there were, whereas potentially now there could be dozens of these converters, and if they all need to be cross-checked against every single property, I could see that being an issue.

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

No branches or pull requests

2 participants