Move MarkdownField and MarkdownFieldSettings to the OC.MarkdownField.Abstractions project

[MikeAlhayek]

I think both MarkdownField and MarkdownFieldSettings classes should be moved to the OrchardCore.Abstractions project to allow other projects to use them without the need to install OrchardCore.Markdown project.

[hishamco]

Why we need to used both MarkdownField and MarkdownFieldSettings without reference OC.Markdown?!! Is there any use case for that?

[sebastienros]

Not the main Abstractions project, but maybe a Markdown.Core. Then modules could reference it without referencing other modules.

[MikeAlhayek]

@sebastienros I am wondering why not the abstraction project in this case? IMO, having the interfaces and models in the abstraction module provides more reusability of the code. In fact, ContentField and ContentElement classes are currently in the abstraction project as I believe they should. There are many other cases where we place the models in the abstractions class like AdminSettings, AdminMenu, AdminNode... etc.

[ns8482e]

@CrestApps why move to abstraction? The field are specific to specific module and is not useful without the module so it’s better be in module itself.

Occasionally, I see need for accessing Part/Field but model of part/field is just sugar coated json

[ns8482e]

If your module is really depend on c# type of Field’s model definition then why not you just add reference of module as private asset?

[MikeAlhayek]

@ns8482e, some time you want to create a castable content part that uses the fields using something like this

public class SomeCustomPart : ContentPart
{
    public MarkdownField Description { get; set; }
   // other properties
}

The above model would be part of a abstraction/core project. Then in the module project, I would register that part like this

services.AddContentPart<SomeCustomPart>()

which will allow us cast it using something like this

var customPart = contentItem.As<SomeCustomPart>();

[ns8482e]

Not sure I understand,

but you can free to define Internal markdown model in your module without adding reference and json would cast it as you wish

we have such scenario where internal class is used in migration without directly referring the model

[MikeAlhayek]

@ns8482e The reason we are forced to used internal models is because these models and their settings are not part of the Abstractions class. If we move the model and the settings to the abstraction class, then we no longer need to define internal classes as a shadow to the actual class.

For example, if you look at the Taxonomies migration you'll see AliasPartSettings and AutoroutePartSettings just created as shadow because AliasPartSettings and AutoroutePartSettings are not part of the OrchardCore.Alias.Abstractions or OrchardCore.Autoroute.Core. The problem with creating internal models like that is what if the actual setting model change? I see no reason for doing such an approach where we can simply move the model and setting into the abstraction class and everything will just fall into place as it should.

[ns8482e]

All I am saying is, the model reference referred here is just json and it’s not necessary and/required as there are native c# equivalent is available.

IMHO it’s not an candidate to move to abstraction as it’s not concern to OC framework

[MikeAlhayek]

I don't understand. OC framework already have these module/settings as part of the abstraction projects in many cases. Why not here too? Either way, how do you go about creating a model in class that references the MarkdownField. Meaning how would you create the above SomeCustomPart class in a none Module project?

[sebastienros]

The discussion came to the recommendation of referencing the modules containing these fields from either user apps or modules.

Example:

This class in an app would reference the ContentFields module.

Person
{
  TextField Name;
  NumberField Age;
}

In this github solution we don't reference modules from modules, probably because we didn't want to introduce cyclic dependencies between modules. (@jtkech @deanmarcussen right?)

[deanmarcussen]

Yes, that was the original design choice @sebastienros

I don’t agree with moving them to an abstractions project, if we do that, eventually everything will get moved to an abstractions project

[hishamco]

IMHO we should document a code guide lines, including which should go into abstraction and which in implementation. I see some different styles in many modules, which may need to fix

[Skrypt]

So, what about keeping the ContentFields in the modules themselves and creating an abstraction project that contains an Interface that can be reusable for common ContentFields? Example: ITextField.

    public interface ITextField : IContent
    {
        public string Text { get; set; }
    }

    public class TextField : ContentField, ITextField
    {
        public string Text { get; set; }
    }

I understand that using the Interface afterward may cause issues if it's not used everywhere. Though that would not be without refactoring some code and probably having a major release that implies breaking changes.

Here, the ContentField class is part of an abstraction already. It is OrchardCore.ContentManagement.Abstractions. This is why we mainly should not have the ContentFields implementations inside another abstraction project.

The interface is redundant though.

@jtkech, @deanmarcussen I want to know your opinion on that one.

[MikeAlhayek]

@Skrypt what would you then do with the settings while being able to strongly typed. In other words, how would you set the settings of the TextField from migration?

[Skrypt]

Like Sebastien showed, by using the Interface. Without needing to reference the entire module. If the example is the Taxonomy migration.

https://github.com/OrchardCMS/OrchardCore/blob/c255456bf1d9093d13447e17485a23a1253f3e9b/src/OrchardCore.Modules/OrchardCore.Taxonomies/Migrations.cs#L31-L37

So, basically no OC Module reference from another one.

[MikeAlhayek]

I am guessing this is what you are referring to .WithSetting(nameof(IAliasPartSettings.Pattern), "{{ Model.ContentItem | display_text | slugify }}") May be we can even create a new builder using the interface to set the values. That should be valid too.

[jtkech]

@sebastienros Maybe not that cyclic dependencies would be a technical concern, but yes to keep things well modularized.

@Skrypt I will think about it, no closed opinion, personnaly for now I just reference the module when I need it ;)

[Skrypt]

Of course, just poking brains around. 😉