umco / umbraco-nested-content

Nested Content for Umbraco 7.1 to 7.6
https://our.umbraco.org/projects/backoffice-extensions/nested-content
MIT License
45 stars 35 forks source link

Make DetachedPublishedContent public #23

Closed jbreuer closed 9 years ago

jbreuer commented 9 years ago

Currently DetachedPublishedContent is internal. I'm trying to see if I can register DetachedPublishedContent into the PublishedContentModelFactory for the Models Builder, but currently I can't do that.

Maybe use the same class for NestedContent and the Doc Type Grid Editor?

leekelleher commented 9 years ago

Hi @jbreuer - we're using DetachedPublishedContent when converting the Nested Content property value. I can't see how that fits into using the model-factory.

I thought the model-factory only worked with proper nodes?

jbreuer commented 9 years ago

I want to test if the model factory can also work with DetachedPublishedContent. If you look here you can see how it registers types: https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/master/Zbu.ModelsBuilder/Umbraco/ConfigurePublishedContentModelFactory.cs#L28

I want to see what happens when I just try to add DetachedPublishedContent in there too. It might be possible based on this twitter discussion: https://twitter.com/zpqrtbnk/status/621665913885093888

mattbrailsford commented 9 years ago

I'd say create a fork, make it public in your fork, and run your tests on your custom build to see if it will actually work first as I'd rather not make it public to find it doesn't work anyway and then we can't make it internal again. Once we know it works, we can discuss then whether to make it public or not.

mattbrailsford commented 9 years ago

Let us know how you get on though, as will be interesting to know if it does work.

jbreuer commented 9 years ago

I made it public and tried to register it. Now getting this exception:

Type Our.Umbraco.NestedContent.Models.DetachedPublishedContent is missing a public constructor with one argument of type IPublishedContent.

So it needs a constructor that accepts IPublishedContent. Not sure how that should work.

leekelleher commented 9 years ago

@jbreuer Did you try adding a .ctor overload? Then (I think) it would be a case of populating the private fields.

jbreuer commented 9 years ago

Hmm haven't tried that yet. Will do some more tests.

Maybe not related, but this also just works:

var items = this.GetPropertyValue<IEnumerable<IPublishedContent>>("sliderNestedContent"); return items.Select(x => { var slider = new Slider(x); return new HomeSlider() { Title = slider.Title, Text = slider.Text, Image = slider.Image }; } ).ToList();

In new Slider(x); Slider is the model generated by the models builder and x is the DetachedPublishedContent. So a DetachedPublishedContent can be converted to a generated file.

leekelleher commented 9 years ago

@jbreuer I still can't see how the model-factory fits into this scenario.

To my knowledge the model-factory only works with the content-cache (maybe media too?), so when you get an IPublishedContent from the cache, the model-factory will run via the Umbraco core's internal PublishedContentExtensionsForModels.CreateModel extension method.

@zpqrtbnk - please correct me if I'm wrong here?

Since Nested Content is a property-editor, the value doesn't come from the content-cache - as in it is not a "node" in the tree. The value will only be retrieved and converted (to IPublishedContent) when .GetPropertyValue<T> is called. (At that point, the model-factory could be called, but that could get messy? and is not something we've considered yet)

I can see what you're trying to do, but I don't know enough about ModelBuilder's internal mechanics to give any good advice.

From Nested Content's perspective, the property-value-converter will return an IPublishedContent object collection, (ignoring that is actually an internal DetachedPublishedContent), then implementers can do whatever they would do with that IPublishedContent collection.

I hope this makes sense?

jbreuer commented 9 years ago

What you're saying makes sense and I just wanted to see if somehow it could work, but I don't think it can currently. I'll wait @zpqrtbnk his reaction, but probably I'll just need to keep working with the IPublishedContent. That it can be converted to a strongly typed model just by passing it into the constructor of that model already makes it a lot more easy.

leekelleher commented 9 years ago

@jbreuer With your example of the Slider model. If the constructor already accepts an IPublishedContent, then that would be fine. (You say this already works?)

On a side-note, not to start a "mapper war", but Ditto can already handle the scenario that you want.

jbreuer commented 9 years ago

Yes the constructor already accepts an IPublishedContent so that works. I haven't looked at Ditto yet, but I definitely will.

zpqrtbnk commented 9 years ago

The factory is called by Umbraco via the internal PublishedContentExtensionsForModels.CreateModel() extension method, ie anything that comes from the cache goes through that method. And the factory works by creating an instance of your model class, passing the original (cached) IPublishedContent to the ctor. That's why it wants a public .ctor(IPublishedContent content). Detached/nested contents do not come from the cache and therefore the factory is not involved. We could open that CreateModel() method but it would still be your responsibility to invoke it when creating the contents, and to ensure that the factory knows about the content types.

Does that all makes sense / helps?

TBH the whole detached/nested picture at the moment does not look nice to me. It's definitively something that needs love from Core to work correctly. That's probably the next thing I'll be working on but at the moment stabilizing the content cache comes first. There is a bit of internal code in there, dating from some experiments we've done with Matt.

What would help a lot is if you guys could come up with a list of: this is what we'd want Core to do and then everything would be so nice. Otherwise I'll do that analysis. Just not right now.

zpqrtbnk commented 9 years ago

@leekelleher: the models factory works with anything that's IPublishedContent, and that includes at the moment anything that comes from the content cache, the media cache, and the member objects you get from the UmbracoHelper. Basically, anything that has a "content" type and user-defined properties.

And please, don't mention the war.

jbreuer commented 9 years ago

The whole detached/nested idea has also been discussed at the retreat. We talked about IPublishedEntity. Creating an interface for virtual nodes. So it's already on the roadmap :-).