The Taxonomies module is using AliasPart but it does not depend on it

[MikeAlhayek]

Describe the bug

The Taxonomy module use migrations to define a new Taxonomy content type. The migration uses AliasPart to allow the user to provide alias for looking up the content items using predefined alias. However, then the Taxonomies feature is enabled, the Alias will not be enabled.

The same goes for Autoroute feature as well as TitlePart I am not sure if the intention here is to predefined these content-types with these optional parts.

To Reproduce

Steps to reproduce the behavior:

1. Make sure Alias feature is disabled.
2. Enable Taxonomies feature
3. Try to add/edit Taxonomy the alias part won't show up

Expected behavior

I would think in this case either we should add OrchardCore.Alias as dependency to the manifest . Or, define a new migration that would only be used when RequireFeatures("OrchardCore.Taxonomies","OrchardCore.Alias") to weld theAliasPartto theTaxonomy` content-type.

[Skrypt]

Here maybe the issue is that the Alias, Title, Autoroute feature should always be enabled when using Orchard Core CMS. Though, having the choice to disable these features makes no sense at all if you do use the CMS. So, the Taxonomy feature itself is not dependent on the Alias feature. Though, if a migration contains a Taxonomy Content-Type which also has an AliasPart then that migration should also enable the Alias feature. The Manifest of the Taxonomy module in that case has nothing to do with it.

Migrations are arbitrary and can sometimes miss some steps. It doesn't mean that the Taxonomy module should always enable the Alias feature.

[MikeAlhayek]

@Skrypt not sure why we would want to weld AliasPart if the Alias feature isn't enables. If the feature isn't enabled none of the Alias function will work. Here is the issue I ran into, I did not have the Alias feature enabled because I never used it in one tenant this far. I am not against using it here, just never explicitly needed to manually enable it. But, I needed to use Taxonomies. So I created a taxonomy item using code not UI, which worked as expected. However, the Alias info was never indexed since the Alias feature was not enabled. Since by default we are trying to provide a specific behavior, all functions that are needed to provide the full behavior should be included. Which is why I think we should use the Mainfest file to define dependencies needed to provide a full behavior.

But if we don't want to provide full behavior as what that code seems to intent, I would suggest at least to update the documents of the taxonomies to state that "Taxonomies by default provide functional support to Alias and Autoroute, However, these features must be explicitly enabled."

But I in my option, we should add dependencies to these feature in the Mainfest so we can provide full behavior for anyone that want to use Taxonomies. At the end of the day, they can chose to not utilize the provides functions but at least all provides functions will be fully functional.

[Skrypt]

What I'm saying is that if the migration requires an AliasPart and the feature is not enabled then the migration should include a part where it enables the AliasPart feature in Orchard Core. Though, since these recipe files are often exported from the Import/Export module they can often contain missing steps simply.

Here, when we import data, if we don't provide a step that says: enable the Alias feature then the AliasPart will still get imported as it should because your migration data includes it. Though, in the process of importing a recipe, we don't warn anyone about the fact that it is missing some ContentPart for it to work as intended.

So, here, if we provided a migration that is missing a step; I suggest we simply add the step in the migration/recipe. Though, if what you want is to be warned about potential data imports that could miss some ContentPart(s) then we would need to create a recipe analyzer that could provide such warning.

There is surely some more work needed to be done to make the recipe import report issues but at the same time if someone imports AliasPart data then common sense also applies.

[MikeAlhayek]

I understand. What you are saying makes sense and a recipe analyzer is a good idea to be able to provide feedback. But in the Taxonomies case, the feature is not using a recipe to define the content type. We create the Taxonomies directly using a migration. So When the feature is first enabled, we'll alter the definition directly so even if we have an analyzer this won't apply here... unless the migration eventually create a recipe in the memory in the back end and execute the logic that way which is not what I think is happening.

I think if we don't want to add dependency in the Mainifest "which I would understand", I would suggest we add a new start up and decorate it with [RequireFeature] attribute which is perfect for such scenario. So we would add something like this

[FeatureFeatures("OrchardCore.Taxonomies","OrchardCore.Alias")]
public AliasStartup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
        services.AddScoped<IDataMigration, TaxonomyAliasMigrations>();
    }
}

[FeatureFeatures("OrchardCore.Taxonomies","OrchardCore.Autoroute")]
public AutorouteStartup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
        services.AddScoped<IDataMigration, TaxonomyAutorouteMigrations>();
    }
}

[FeatureFeatures("OrchardCore.Taxonomies","OrchardCore.Title")]
public AliasStartup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
        services.AddScoped<IDataMigration, TaxonomytitleMigrations>();
    }
}

public class TaxonomyAliasMigrations : DataMigration
{
    private IContentDefinitionManager _contentDefinitionManager;

    public TaxonomyAliasMigrations(IContentDefinitionManager contentDefinitionManager)
    {
        _contentDefinitionManager = contentDefinitionManager;
    }

    public int Create()
    {
        _contentDefinitionManager.AlterTypeDefinition("Taxonomy", taxonomy => taxonomy
            .WithPart("AliasPart", part => part
                .WithPosition("2")
                .WithSettings(new AliasPartSettings
                {
                    Pattern = "{{ Model.ContentItem | display_text | slugify }}"
                })
            )
        );
    }
}

public class TaxonomyAutorouteMigrations: DataMigration
{
    private IContentDefinitionManager _contentDefinitionManager;

    public TaxonomyAutorouteMigrations(IContentDefinitionManager contentDefinitionManager)
    {
        _contentDefinitionManager = contentDefinitionManager;
    }

    public int Create()
    {
        _contentDefinitionManager.AlterTypeDefinition("Taxonomy", taxonomy => taxonomy
           .WithPart("AutoroutePart", part => part
                    .WithPosition("3")
                    .WithSettings(new AutoroutePartSettings
                    {
                        Pattern = "{{ Model.ContentItem | display_text | slugify }}",
                        AllowRouteContainedItems = true
                    })
            )
        );
    }
}

public class TaxonomyTitleMigrations: DataMigration
{
    private IContentDefinitionManager _contentDefinitionManager;

    public TaxonomyTitleMigrations(IContentDefinitionManager contentDefinitionManager)
    {
        _contentDefinitionManager = contentDefinitionManager;
    }

    public int Create()
    {
        _contentDefinitionManager.AlterTypeDefinition("Taxonomy", taxonomy => taxonomy
            .WithPart("TitlePart", part => part.WithPosition("1"))
        );
    }
}

Then the existing Migrations class would be changed to this

public class Migrations : DataMigration
{
    private IContentDefinitionManager _contentDefinitionManager;

    public Migrations(IContentDefinitionManager contentDefinitionManager)
    {
        _contentDefinitionManager = contentDefinitionManager;
    }

    public int Create()
    {
        _contentDefinitionManager.AlterTypeDefinition("Taxonomy", taxonomy => taxonomy
            .Draftable()
            .Versionable()
            .Creatable()
            .Listable()
            .WithPart("TaxonomyPart", part => part.WithPosition("4"))
        );

        // indexes will still be added here
    }
}

[Skrypt]

Ok, I understand your point. Will think about it.

[Piedone]

What makes this non-trivial is that Taxonomy attaches both AliasPart and AutoroutePart. This is curious to me, since I think only one is useful at a time, but I guess has some reason. Title though is definitely needed.

So, unless we want to figure out why both AliasPart and AutoroutePart are needed and break some people's apps when removing either of them, why not just add the two dependencies in the Manifest? And add the one for Title for sure.