OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.39k stars 2.38k forks source link

Recipes Improvements #16229

Open MikeAlhayek opened 4 months ago

MikeAlhayek commented 4 months ago

One of the issues with recipes is that we currently tie a recipe to a specific feature when a module has multiple features. This should not longer be a limitation.

Since #15793 was merged, we should be able to tie a service to a feature with no problems. With that, we should be able to register multiple instanced of the RecipeDescriptor where each RecipeDescriptor will have info needed to locate the recipe file.

Then we probably, would remove IRecipeHarvester and it's implementation since this info will be in the IoC.

github-actions[bot] commented 4 months ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

Piedone commented 4 months ago

Recipes don't need a feature (like in a root web project), or a feature to be enabled to be available (since a recipe in a module may also enable features in that module, what many do).

So, I don't see tying a recipe to a feature as useful, but on the contrary, it would break important scenarios.

MikeAlhayek commented 3 months ago

Recipes don't need a feature (like in a root web project), or a feature to be enabled to be available (since a recipe in a module may also enable features in that module, what many do).

That is valid for startup recipe but not for partial recipes that show up on the recipe admin page. But even for startup recipe, They should be tied to a feature. this way if you don't want some of the startup recipe to show up, then you won't make them available. One way this could be done is using a feature that is always enabled to enable the startup recipes only.

So, I don't see tying a recipe to a feature as useful, but on the contrary, it would break important scenarios.

We should be able to cover all scenarios even after assign them to feature. As mentioned above, one can create always-available feature to expose the all or subset of the startup recipes. In some case, you may want to limit the available startup recipes based on the environment. So in that case, you can have multiple features in the same module that would provide you with subsets of the startup recipes that should be available.

Piedone commented 3 months ago

Taking a step back, what do we try to solve? What's in the current system, that works and I haven't seen people complaining about, that's unsuitable?

Not having all recipes listed on the setup screen is a solved problem.

MikeAlhayek commented 3 months ago

I want to be able to control when recipes appear in the UI. Sometimes you want to tie a recipe to a feature in a module and not to the module itself as explain previously.

Also, for startup recipes, I want to be able to tie startup recipes to a feature as explain as well where I can use the same code with multiple environment where environment X can have a subset of startup recipes while environment B have another subset of startup recipes.

If others are not complaining, that does not mean there isn't an issue. Note that you had to create a feature to solve part of the problem because OC does not have a complete solution. Otherwise, why would you have that feature.

Piedone commented 3 months ago

That's a fair point. How would we keep supporting the scenario of the recipe in a module with otherwise not yet enabled features being available, also enabling itself? This is a valid scenario for non-setup recipes as well, and something that recipes that initialize optional feature sets employ.

MikeAlhayek commented 3 months ago

I would create an always enabled feature and I would place these kinds of recipes into. This way you have full control on which recipes make sense to enable all the time vs recipes that depend on other features. Remember that there are recipes that we want available only if an X feature is enabled too.

Piedone commented 3 months ago

Do you mean to have an always-enabled feature for every module, then, that by default does nothing but you can add a corresponding Startup?

MikeAlhayek commented 3 months ago

No. I mean in the case when you want a recipe to be always enabled, then you can create a feature and assign all the "always-enabled" recipes to it.

Piedone commented 3 months ago

Hmm, and how that would be implemented? There's a chicken-and-egg problem, since even IsAlwaysEnabled = true features need to be enabled once. I'm not sure how I'd feel about adding something like an EnabledByDefault.

MikeAlhayek commented 3 months ago

I would create a new module for recipes (you can split that module into multiple features as needed). One or more of the features in the module will have IsAlwaysEnabled set to true. Now you can associate the recipes that you always want to make available to this feature. Same true for startup recipes too. The idea is just to associate them to a feature and then control the behavior of that feature as needed "when needed".

Piedone commented 3 months ago

What would enable that module, though? Or are we talking about a special module that the container building/shell start logic knows about?

MikeAlhayek commented 3 months ago

You can use AddGlobalFeatures() from the web app startup to ensure it's always enabled.

Piedone commented 3 months ago

OK! Let's just not require anybody to have to explicitly enable this.

hishamco commented 3 months ago

What's the summary for this enhancement :) seems I lost for the last 3 days because I got a new baby

Piedone commented 3 months ago

Use case: https://github.com/OrchardCMS/OrchardCore/issues/16229#issuecomment-2157058599 Possible implementation: https://github.com/OrchardCMS/OrchardCore/issues/16229#issuecomment-2158805702

Is that some kind of figure of speech or indeed you became a father? If the latter, then a hearty congratulations, Hisham!

hishamco commented 3 months ago

Is that some kind of figure of speech or indeed you became a father? If the latter, then a hearty congratulations, Hisham!

Latter :) thanks so much, hope to join OC community ASAP

Piedone commented 3 months ago

Oh, awesome, congratulations! :)

hishamco commented 3 months ago

Thanks Zoltan