Umbraco should allow for custom ModelsBuilders

[bielu]

I looked into code of Umbraco how is handled actually swapping of Models Builders and I noticed, it is hard coded to use IsLegacyModelsBuilderInstalled(), which could actually caused issues even with future development of further development of Our.ModelsBuilder, which is continuation of project started by Stephan. From my perspective that should should be refactored and made more open for extension as from my perspective hard coding name of assemble to check if something is installed is a bug maker. I think that will be nice if in future it will be more than one project which could replace Embed ModelBuilder...

Umbraco version

I am seeing this issue on Umbraco version: 8.6.2

[Shazwazza]

We had to make this 100% forward/backward compatible and this was the only way to do this. Moving forward we have options. Feel free to come up with something! But this must be entirely backwards compat too.

[bielu]

I looked into changes which @zpqrtbnk made in Our.ModelsBuilders, and actual implementation in HQ Embed Models Builder forced to use:

    [DisableUmbracoModelsBuilder]

    // disable the embedded MB that ships with the CMS
    [Disable(typeof(Embedded.Compose.ModelsBuilderComposer))]

Which I am feeling is really bad approach from CMS which we trying to extend, I will do some thinking around here but that Disable part for me looks really messy and not something what I or probably other developer would to use.

[Shazwazza]

Yes but like I said, we couldn't go back in time to change the CMS to support the old models builder, this was the only way to fully be backards compatible with the old MB, the embedded MB and switching back to the old MB. So whether or not it was a 'bad approach' it was the only approach possible. Moving forward we can see what might make sense but considering we need to 100% support people running the old MB and from previous upgrades, i don't suspect we'll be able to change how this approach works since we cannot go back in time to change the integration for the old MB.

[bielu]

@Shazwazza, I learned one thing it is always another way to do something, it means it is probably possible to find way to do that differently :) ,question only how much time and effort it takes :). It is why I am going to look into that and maybe find different way. :) I really still don't understand why MB wasn't just integrated at all into Umbraco but yeah it causing today more issues than I expected as Embed version missing a lot features which I was using quite often.

[Shazwazza]

yes that is true :)

I can re-cap some info about MB and decisions:

• Embedding MB in the core did not take away any functionality from people because the existing external MB still completely worked and still works today including all functionality that was there.
• We did not want to port all of the external MB into the core for a few reasons:
  • We could not support some of the features in the external MB that were already there including things like the VS extension
  • There was plenty of existing issues and feature request for the external MB that we can not commit to fixing/implementing
  • Umbraco HQ did not want to be a bottleneck for what the community wanted to do with MB. Its started as a community project and it will continue as a community project and will be able to iterate much faster than if it was purely an HQ project.
  • The only parts of MB that were put into the core were the bare minimum bits required to run Umbraco

It took a while for the external MB to progress to the community project that it should be but it's getting there which is great. The next external MB could/should also integrate much better with the core now that the core contains the basic functionality and actually knows about MB whereas before it didn't whatsoever.

[bielu]

It took a while for the external MB to progress to the community project that it should be but it's getting there which is great. The next external MB could/should also integrate much better with the core now that the core contains the basic functionality and actually knows about MB whereas before it didn't whatsoever.

In same it will be harder to deal with that as new external one need to check if it old one installed and disable embed one as I show earlier :) also If I would like create totaly custom model builder if I am going with same logic here: I need to check Umbraco.MB Umbraco.Embed.MB Our.MB Which is starting being quite hard in maintain as I think it should way in Umbraco to determine that. But in same if I am creating new custom MB I could be "lazy" and just deregister only "Embed" one and put responsibility on user, but still I think it would be nicer if Embed "would" at least little smarter to find any MB installed :).

[Shazwazza]

Yes I agree, the integration for 'new' MBs should be nice but we still have to maintain compat with the old MB without changing it so there's gonna need to be some hacks in the CMS. As a new MB project though it would be better that the CMS takes care of that for you and you don't have to worry about it.

[nul800sebastiaan]

To sum up: yes, it would be nice to make Umbraco more extensible for future modelsbuilder forks (which is a hypothetical problem for now).

So, short term: if we need to update the list of dlls we need to check for the new MB version to work, let's do that now and we can ship it in the next patch release.

Long term: fixing this to make it properly pluggable would be great. It's not something that has our immediate focus so a review might take a while.

[umbrabot]

Hi @bielu,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

[umbrabot]

Hiya @bielu,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face: