Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BaseModel attribute for PublishedElementModel #205

Open
ViGiLnT opened this issue Jun 6, 2019 · 18 comments
Open

BaseModel attribute for PublishedElementModel #205

ViGiLnT opened this issue Jun 6, 2019 · 18 comments
Assignees
Labels
state/review Done, Being Reviewed type/feature Feature
Milestone

Comments

@ViGiLnT
Copy link

ViGiLnT commented Jun 6, 2019

In Umbraco 8.0.2, using ModelsBuilder 8.0.4 I'm unable to define a custom BaseModel because now generated Models may inherit from PublishedElementModel and PublishedContentModel. I have a document type which is an Element for NestedContent.

Can't we have another attribute for ElementBaseModel? Like:

[assembly: ModelsBaseClass(typeof(BaseModel))]
[assembly: ElementModelsBaseClass(typeof(ElementBaseModel))]

@zpqrtbnk zpqrtbnk self-assigned this Jun 6, 2019
@zpqrtbnk zpqrtbnk added state/backlog Backlog, To Do type/feature Feature labels Jun 6, 2019
@zpqrtbnk
Copy link
Collaborator

zpqrtbnk commented Jun 6, 2019

Oups. Valid point.

@pgregorynz
Copy link

Pretty vital that this one gets fixed ASAP as it is negating some of the benefits of using Models Builder. The base class functionality reduces the amount of Helper methods and common properties we need to define. We are stalled at present due to this issue.

@pgregorynz
Copy link

Have created pull request to at least ignore the element type from gaining the custom IpublishedContent base class. #211

pgregorynz added a commit to pgregorynz/Zbu.ModelsBuilder that referenced this issue Aug 8, 2019
Adds the ability to create a Element base class
@zpqrtbnk
Copy link
Collaborator

zpqrtbnk commented Aug 8, 2019

Thanks! Definitively going to review. Might take a few more days as I'm partially offline for 1 more week. Stay tuned!

@pgregorynz
Copy link

We have now also added in the attribute for base classing element types. Just needs a Unit test :)

@zpqrtbnk zpqrtbnk added state/in-progress Work In Progress and removed state/backlog Backlog, To Do labels Aug 23, 2019
@zpqrtbnk zpqrtbnk added this to the 4.0.0 milestone Aug 23, 2019
@zpqrtbnk zpqrtbnk added state/review Done, Being Reviewed and removed state/in-progress Work In Progress labels Aug 26, 2019
@zpqrtbnk
Copy link
Collaborator

Took most of the code from the PR + tweaked things & implemented tests. Now we would have:

  • [ContentModelsBaseClass(type)] indicates that the default base class for content models is type - this replaces the [ModelsBaseClass] attribute
  • [ContentModelsBaseClass(pattern, type)] indicates that the base class for content models with an alias matching pattern is type
  • [ElementModelsBaseClass(type)] and [ElementModelsBaseClass(pattern, type)] work the same, but for elements

Patterns can be a plain type name (eg page) but also support leading and trailing wildcards (eg *page or page*) and I am not sure we want to support more fancy patterns.

Patterns are ordered by length, longest first, before being applied, and the first one to match wins. So if you have both a pageFoo* and a page* pattern, the first one would apply for pageFooBar.

Thoughts?

@pgregorynz
Copy link

The pattern stuff is great! We actually have a version close to this on our fork that we were considering submitting that allowed for selective models that had the pattern so you could have multiple base models... Does your version allow for this also?

EG a catch all sort of base model, and then selective base models for other types eg "page*"

If that is the case this is awesome and is perfect.

@zpqrtbnk
Copy link
Collaborator

The pattern-less version [ContentModelsBaseClass(type)] actually registers type with a * pattern, which acts as a catch-all and, being the shortest one, matches last. So yes, you can define a catch-all + selective for some patterns.

Here I am assuming that patterns will mostly be "page*" or "*thing" and that we don't want/need to support crazy regex patterns.

@pgregorynz
Copy link

In our instance we have a PageBaseModel and a BlockBaseModel in the same project.
The builder works out which one to use when it is parsing the doctypes based on a pattern.

Changes can be seen here:

pgregorynz@f9124dc

The attribute on our class was:

[assembly: SelectiveModelsBaseClass(typeof(BlockBaseClass), "*Block")]

We will update to your version and make sure it does what I think it does and will let you know but it looks about right based on your description 👍

@zpqrtbnk
Copy link
Collaborator

[ContentModelsBaseClass("*Block", typeof(BlockBaseClass))] should do exactly this ;-)

@pgregorynz
Copy link

NICE! Perfect.. We will update our build of models builder and give it a whirl.

Overall I think this will make ModelsBuilder super flexible. Our version did, so having it official is awesome!

@euanrae
Copy link

euanrae commented Nov 28, 2019

was this feature ever released? I'm using Umbraco 8.1.3 and Umbraco.ModelsBuilder 8.1.0 and I can't see it?

Thanks

@zpqrtbnk
Copy link
Collaborator

Not released. A new version of MB is in-progress but, due to the september organization changes, I am only working on it when I have some time available. Back to full open-source mode: it will hopefully be ready, when it's ready ;)

@euanrae
Copy link

euanrae commented Nov 28, 2019

all good - thanks for letting me know (wanted to make sure it wasn't there and I was just being dense)!

zpqrtbnk pushed a commit that referenced this issue Apr 12, 2020
@Zweben
Copy link

Zweben commented May 15, 2020

I'd really like to take advantage of base models on my project. Is there any way I can use it at the moment with Umbraco 8.6, such as a stable fork that contains the fix, or a temporary workaround?

@dhymik
Copy link

dhymik commented May 21, 2020

I am also very interested....

@Andrei-
Copy link

Andrei- commented Jun 10, 2020

I would also love to use this, any updates?

@bielu
Copy link
Contributor

bielu commented Aug 22, 2020

@zpqrtbnk I checked that and I am using configuration, which replaces your orginal idea, and works perfectly fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/review Done, Being Reviewed type/feature Feature
Projects
None yet
Development

No branches or pull requests

8 participants