search

umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

ModelsBuilder : Bring back attributes to control models generation #7734

Closed dawoe closed 4 years ago

dawoe commented 4 years ago

One of the features that has been dropped with integration of Models builder into core is controlling how models are generated by using attributes : https://our.umbraco.com/Documentation/Reference/Templating/Modelsbuilder/Control-Generation-vpre8_5

For me this was the most powerful feature of MB and I'm sad to see it go. I know I can use the community package to bring to have this functionality. But there is no guarantee that is package will be maintained in the future.

I know there is the ability to extend with custom properties, but this is not ideal. Take the example where you doctype has a SeoTitle property. If this is not filled you wanted it to fallback to the page name. You don't want to put this logic in each and every view so you want to do this on the model.

So with the old models builder I would tweak the model like this

 public partial class TextPage
    {        
        [ImplementPropertyType("seoTitle")]
        public string SeoTitle
        {
            get
            {
                return this.Value<string>("seoTitle", fallback: Fallback.ToDefaultValue, defaultValue: this.Name);
            }                
        }
    }

But with the new intergrated models builder this is not possible anymore. You need to add a custom property.

public partial class TextPage
    {
        public string BrowserTitle
        {
            get 
            { 
               if(!this.SeoTitle.IsNullOrWhiteSpace())
                {
                    return this.SeoTitle;
                }

                return this.Name;
            }
        }
    }

So we could the same, but now you have 2 properties : SeoTitle and BrowserTitle. And it needs to be clear to developers which property they need to use.

Is this something HQ is considering to put back in ? If so, with some pointers in the right direction, I am up for doing the work.

We could start small. Just bring back the attributes to tweak property generation. Not the ones that will change the model.

Dave

nul800sebastiaan commented 4 years ago

We currently have no plans to add more features to the core Modelsbuilder implementation and recommend using the community package instead (I know.. that's not what you wanted to hear 🙂).

I'll put this on the list of ideas for when we're looking to work on new features around core Modelsbuilder again.

dawoe commented 4 years ago

We currently have no plans to add more features to the core Modelsbuilder implementation

Why not ? I have already seen a similar issue where somebody is missing this feature ( #7495 )

This is one of the features that made ModelsBuilder really powerful. Not everybody is running in PureLive mode and was depending on these more "advanced" features. For those people the core version is a step backwards.

So pointing people to a unmaintained* community package feels plain wrong to me. Why do I say unmaintained. Last release is from April 2019 and there have been no commits since august 2019.

We have a RFC process in place, but for some reason unknown to the community, this wasn't used when Modelsbuilder was integrated. Then we could have had this discussion upfront.

Dave

nul800sebastiaan commented 4 years ago

Why not?

We're satisfied with the version we have right now and there's a wonderful community package that can help users who need more features than we're offering right now.

unmaintained

It is my understanding that if CMS makes changes that impact the community package we will make sure it doesn't break.

bielu commented 4 years ago

@nul800sebastiaan I know you trying your best but here are my thinks after reading that issue.

We're satisfied with the version we have right now and there's a wonderful community package that can help users who need more features than we're offering right now.

But people who are using you product are not happy about that, and I think that Attribute should be in Core anyway... It is one of basic features of ModelsBuilder and shouldn't be cut out just because the core version is limited...

It is my understanding that if CMS makes changes that impact the community package we will make sure it doesn't break.

Hmmm 8.5, 8.5.1, 8.5.2, where there was care about not breaking community package (Models Builder from Stephan). Umbraco really often breaking packages by not documenting changes, introducing breaking changes in minor releases or even not flagging breaking changes at all( changes in BackOffice, Umbraco Indexers etc).

nul800sebastiaan commented 4 years ago

I'll repeat the above, we have no intention of adding features to the core modelsbuilder at the moment and if you need more features then there is a package full of advanced goodness available.

Umbraco really often breaking packages

Would love examples of this please. We do the very best we can with the tooling and the people we have right now and we always make sure to fix mistakes when they are made. I don't know about packages breaking since v8.2 came out, so I'd love to hear what we're missing there.

dawoe commented 4 years ago

@bielu @nul800sebastiaan

The attribute is actually still there. But the check for seeing if you use in your own partial classes is not.

https://github.com/umbraco/Umbraco-CMS/blob/41dc8d915bdcadb628e6dde9422586310ac66a1b/src/Umbraco.ModelsBuilder.Embedded/ImplementPropertyTypeAttribute.cs

I just ran into a issue where the attributes would have been very handy with the embedded models builder. From the backoffice you can create a doctype and put a property on it with same alias as the doctype alias. You can't generate your models in this case, because a class can't have a property with the same name. With the attribute I could have solved this in how the models are generated.

Yes I know I could change the doctype or property alias. But we often create our doctypes before we start developing, because editors can already start with entering content. In that case changing a doctype or property alias will result in the loss of content.

I still don't get the hesitation of putting this back in. Even when people are willing to put their time and effort in it.

This would be the same that you suddenly remove the option to put in external urls in MultiUrlPicker in core. And point people to the old community package if they want that.

I know HQ point of view is that modelsbuilder always has been a community package and not a part of core. But to the community (at least for me and several others) it always was a part of core since 7.4

I get that HQ does not want to support the Visual Studio extension. But ModelsBuilder in core should have the same functionalities as it did before.

Dave

nul800sebastiaan commented 4 years ago

I can't tell you anything else then I've said before. At this moment we have no plans to make changes to the core modelsbuilder implementation. Sorry.

As mentioned before, this issue is on the list of ideas for the future when we start working on modelsbuilders features again.

dawoe commented 4 years ago

So if I make the PR, this has no change of being merged in ? Just want to make sure I don't waste any of my time.

Dave

bielu commented 4 years ago

I don't know about packages breaking since v8.2 came out, so I'd love to hear what we're missing there.

I have a few examples, with change done in 8.4 when Umbraco moved to use Angular components with the controller declared as an internal function which doesn't allow any more to extend controllers. It wasn't even flag as breaking change... :)

nul800sebastiaan commented 4 years ago

@dawoe

So if I make the PR, this has no change of being merged in ? Just want to make sure I don't waste any of my time.

Correct, this is not currently marked as up for grabs so we're not soliciting PRs for it.

@bielu

when Umbraco moved to use Angular components with the controller declared as an internal function

You'll have to remind me what that change was? I want to mark anything that is actually breaking as such, so an issue number or PR number would be great to have!

bielu commented 4 years ago

@nul800sebastiaan it will be hard to list all of them as it is a lot of them, as example: https://github.com/umbraco/Umbraco-CMS/commit/503fd1887ecb5b1d3529eb8b68eedf41b74050a6 https://github.com/umbraco/Umbraco-CMS/commit/201c31395cf8cf9d3d6e7df0a4627a9d2af74563

nul800sebastiaan commented 4 years ago

@bielu We're going quite a bit off-topic here, but I don't understand why these are breaking changes and what packages they would break?

I am not being argumentative, I just don't have enough knowledge. Can you explain how you're trying to extend the controllers and what is now no longer possible for you? And do you know of any packages that this has actually broken?

bielu commented 4 years ago

We're going quite a bit off-topic here, but I don't understand why these are breaking changes and what packages they would break?

We definitely went off-topic here, should we create a separate issue?

here you have a longer conversation about that: https://twitter.com/callumbwhyte/status/1207630294154649600

dawoe commented 4 years ago

@nul800sebastiaan

Getting back on topic. Strange that you communicate that the embedded version works exactly the same as before, but in practice it doesn't :

From the 8.5 release blog post :

PureLive is still the default mode when you install Umbraco 8.5 and works exactly the same as always and if you want to switch to AppData, this is also done in the same way before by changing the setting in web.config.

Still don't get why HQ forces users to install a external package to get the same functionality you had from Umbraco 7.4 to 8.4.1 out of the box.

nul800sebastiaan commented 4 years ago

@dawoe Right! I apologize for being a complete ModelsBuilder "noob"! From your previous comments I failed to pick up on the fact that this is actually a breaking change, if you had code like that before in the modes supported in core MB then obviously it will break.

Now I get the issue, thanks for clarifying! This should absolutely be fixed and I would LOVE it if you wanted to spend some time to figure out which moving parts are not wired up right now for the attribute to kick in. It might be an oversight instead of a deliberate removal, but as I'm not familiar enough with MB I can't be sure. If, in your investigation, notice that it's a lot more than "just" updating a few classes then let us know, don't spend a whole evening on it resulting in a massive PR (I also don't wish to waste your time!).

@bielu Let's take it to email, it's not an issue tracker topic. sj@umbraco.dk

JasonElkin commented 4 years ago

@nul800sebastiaan, @dawoe I started looking at the changes in ModelsBuilder.Embedded. Bringing this feature back as it was will need a non-trivial amount of code (and dependencies).

I can totally see why @Shazwazza didn't include it in ModelsBuiler.Embedded hence suggesting this feature be resurrected differently in #7506

nul800sebastiaan commented 4 years ago

This led to some more discussions at HQ as well, and we found the following:

I'm sorry that this all had to take such a big detour before coming to these conclusions!

dawoe commented 4 years ago

Hmm I don't see why you would need the roslyn compiler for this. I thought that was only needed for the dll mode.

I think the docs are not correct : https://our.umbraco.com/documentation/Reference/Templating/Modelsbuilder/Understand-And-Extend#extending

If the custom partial class provides a constructor that has the same signature as the generated one, it will be detected and no constructor will be generated, as that would be redundant and would not compile.

But if I look in the code I don't think this is taken in to consideration : https://github.com/umbraco/Umbraco-CMS/blob/41dc8d915bdcadb628e6dde9422586310ac66a1b/src/Umbraco.ModelsBuilder.Embedded/Building/TextBuilder.cs#L196

bielu commented 4 years ago

@dawoe actually it is required because you want to find member which use that attribute and you will need use CodeAnalyses to that as Stephan is doing that here: https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/bf0da79aee8f52ec153bbb200131d1914ee95b2b/src/Umbraco.ModelsBuilder/Building/CodeParser.cs but you don't whole package CodeAnalyses, but only csharp analyses

nul800sebastiaan commented 4 years ago

@dawoe Quite right about the docs! We're working on getting them updated. 👍

Note: it's not ONLY that we would need to add a dependency on Roslyn. I'm sure that it can somehow be hacked in reflection, but that is also not where we would ever want to go for this feature.