U4-7739 - Models Builder - Model class names overlap with other c# library names

Created by Shannon Deminick 14 Jan 2016, 12:49:49 Updated by Shannon Deminick 22 Jan 2016, 12:17:26

Tags: ModelsBuilder

Relates to: U4-7309

Relates to: U4-7738

There are some potential issues with naming content types to be the same as an existing c# class name (depending on namespace), for example Per says he's run into some issues when calling a doc type 'Stream'.

We need to investigate if this is an issue and potential fixes, for example we could do:

using MyModels = Umbraco.Models.PublishedModels

then reference the model always by MyModels.Stream.

Comments

Stephan 20 Jan 2016, 14:30:05

Generating a model named String is not a problem. However, whenever you use that model, there will be a conflict with System.String and you should make sure that you disambiguate properly. That's your responsibility as a dev.

However, there is one place where we would need to do something... it is when we generate the initial template for a content type... because ''then'' if we don't disambiguate then the template does not compile.


Stephan 20 Jan 2016, 14:35:20

Notes...

We can:

  1. Do nothing, template won't compile, people will have to understand
  2. Always generate with full type name, eg UmbracoViewPageglobal::Umbraco.Web.PublishedContentModels.String
  3. Always add a special using statement and UmbracoViewPage<ContentModels.String>
  4. Be clever and figure out whether there is a chance of a conflict

(4) can be done by pre-compiling the template etc, with Roslyn, would be great fun but time-consuming. (2) is ugly

Maybe (3) is best?


Shannon Deminick 20 Jan 2016, 15:15:59

3 is the best.


Sebastiaan Janssen 20 Jan 2016, 15:16:45

3 yeah


Stephan 21 Jan 2016, 16:04:29

Going for (3) so we'd generate views like:

@inherits Umbraco.Web.Mvc.UmbracoTemplatePage<ContentModels.Stream> @using ContentModels = Umbraco.Web.PublishedContentModels @

For ''every view'', ''all the time'', as soon as ModelsBuilder is enabled.

That way, ''even'' if ContentModels is an existing namespace, it will be re-declared, forced, whatever, here and there should be no possible collision. Sounds good?

Is "ContentModels" a nice-enough name? Should we have a (ModelsBuilder) config option so ppl can change it with whatever they like?

Oh and we all understand that... if you're not running PureLive models, the template will fail for as long as you have not re-generated models.


Stephan 22 Jan 2016, 09:44:19

PR for Umbraco: https://github.com/umbraco/Umbraco-CMS/pull/1046 (there will be a PR for ModelsBuilder)


Stephan 22 Jan 2016, 10:50:00

PR for ModelsBuilder: https://github.com/zpqrtbnk/Zbu.ModelsBuilder/pull/84 (requires that the Umbraco PR is merged first)

Using "ContentModels" as a default, though the Umbraco method has a modelsNamespaceAlias parameter that we could use in the future (via a ModelsBuilder option) to change that name.


Shannon Deminick 22 Jan 2016, 11:38:36

Have tested and run unit tests and it works. Will merge and then build a new MB temp build on Myget for umb to reference.


Priority: Task - Pri 2

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.4.0

Sprint: Sprint 7

Story Points:

Cycle: