Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

Make UmbracoPropertyAttribute constructor for ValueResolver overridable #117

Closed
jamiepollock opened this issue Sep 21, 2015 · 14 comments
Closed
Assignees

Comments

@jamiepollock
Copy link

Hey all,
As discussed on the forum, It'd be nice reuse the functionality of UmbracoPropertyAttribute when using writing new value resolvers. For example converting an existing property into a different value (eg. IEnumerable<IPublishedContent> to a single IPublishContent.

As this isn't already in the core, Lee outlined this line would need to change.

Thanks,
Jamie

@leekelleher leekelleher self-assigned this Sep 21, 2015
@leekelleher leekelleher added this to the 0.9.0 milestone Sep 21, 2015
@mattbrailsford
Copy link
Collaborator

Very interesting question. I'm wondering if this could actually fall into the realms of being an Umbraco Property Value Converter? which raises the question, at what point do we recommend PVC's over VR's?

It feels to me like if all you wanna do is change the type coming from UmbracoProperty, this could be when we suggest PVC's, although within Umbraco, these are global and can't be declared on a per mapping basis. I wonder though if within the MNTP PVC you can't detect what the type requested in the conversion is, and if it's just IPublishedContent then do a .FirstOrDefault on the list?

I do mix the use of PVC's when using Ditto, but I've never really given it thought on when to recommend them over VR's.

Wadda you think?

@leekelleher
Copy link
Collaborator

A custom PVC makes sense. It gets the value in the format that you want it, at the correct part of the pipeline. I agree that ValueResolvers are more appropriate when you have no (or little) control over the PVC.

The only downside I recall is that the code to replace it can be a pain in the backside. But I haven't tried it recently, (it might have been improved?)


With regards to adding an overloaded constructor to UmbracoPropertyAttribute, I like that we can make it extensible. What do you think @mattbrailsford?

@mattbrailsford
Copy link
Collaborator

@leekelleher I'm undecided, as it could open it up to abuse, depending on where we say you should draw the line.

If we are in agreement that the PVC should be the right place, I don't think this issue is a strong enough argument to say open it up yet so I'd be tempted to leave it until a more compelling one comes along?

@mattbrailsford
Copy link
Collaborator

My gut feeling is, if it's coming from a single umbraco property, then UmbracoPropertyAttribute along with PVC's should be used. VR's should be used if the value is calculated, or resolved from multiple locations, or a non-umbraco location? In these scenarios, there should be no need to inherit the UmbracoPropertyAttribute/Resovler.

@JimBobSquarePants
Copy link

@jamiepollock What happens in this case if you add a TypeConverterAtrribute assigning the DittoPickerConverter to the property? If any TypeConvrters are used Ditto will check to see if you are expecting a single generic TypeParam and return that instead of an enumerable collection. It also does the reverse.

This should solve your issue without the need for extra code.

@leekelleher
Copy link
Collaborator

@JimBobSquarePants - @jamiepollock mentions on his original form post about using a custom TypeConverter, he wants to get the single IPublishedContent prior to that.

Although this does suggest that a post-conversion-handler could be used?

So many options!

@JimBobSquarePants
Copy link

@leekelleher The question begs then.. @jamiepollock what extra behaviour is required in your converter that DittoPickerConverter doesn't already offer? It should handle converting to any Enumerable or single item from any of the built in pickers.

Yeah So many options!

@jamiepollock
Copy link
Author

Hey gents, thanks for the open discussion on this matter. Looking at the original use case, I wouldn't want anything implemented which could be open to abuse.

@JimBobSquarePants I guess there isn't anything a DittoPickerConverter couldn't do, that's the desired behaviour I'm looking for. My only question is how do I use that?

@leekelleher
Copy link
Collaborator

leekelleher commented Sep 22, 2015 via email

@jamiepollock
Copy link
Author

@jamiepollock
Copy link
Author

The code above is a general TypeConverter when dealing with multiple items. This code is based on stuff that predates custom Value Resolvers.

@JimBobSquarePants
Copy link

That answer looks sound to me. Great work guys 👍

@leekelleher
Copy link
Collaborator

I'm gonna close this one off. @mattbrailsford I agree about the PVC usage.

@leekelleher leekelleher removed this from the 0.9.0 milestone Sep 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

4 participants