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

Consider supporting recommended feature dependencies #2901

Open
kevinchalet opened this issue Dec 19, 2018 · 5 comments
Open

Consider supporting recommended feature dependencies #2901

kevinchalet opened this issue Dec 19, 2018 · 5 comments
Milestone

Comments

@kevinchalet
Copy link
Member

Currently, there's no way to define a set of dependencies as optional/recommended. This is a bit problematic as we don't always want to force users of a module A to use B. For instance:

  • The OpenID module no longer declares OrchardCore.Users and OrchardCore.Roles as mandatory dependencies so that anyone can now write his own membership modules. Yet, these 2 modules are very likely what many/most people will want to use by default, and the OpenID module will work in a degraded mode if they - or similar modules exposing the same services - are not present.

  • Some of the existing modules expose API controllers that use token authentication, which is something offered by the OpenID module. We will likely want to avoid forcing these modules to depend on the OpenID module, but it will be the default and recommended way to do token authentication in OC.

In these scenarios, it would be nice to have a way to define "recommended features", that are declared like classical dependencies in the manifest and trigger a modal dialog when a feature that lists them is enabled, offering a simple way to install them too:

Warning: this module works best with the following modules enabled:
  - X
  - Y
  - Z

If these recommended dependencies are not enabled, this module might not work correctly or may require additional code. If you're unsure what to do, simply click 'yes' to enable the recommended dependencies.

/cc @jtkech

@jtkech
Copy link
Member

jtkech commented Dec 19, 2018

Okay i understand, good idea, i will think about it. I think it is not so hard to do but, as you said, the modal would be shown only if the new "recommended features" property is set.

What i'm used to do is to always depend on a feature which registers a default implementation (may do nothing) so that the feature works. Then you can enable a feature that overrides the default implementation.

Note: We could also think about suggesting concrete implementations, but not so easy if multiple features implement part or all of the same service(s) or other unrelated services we may not want.

As a side note this let me think that when feature A depends on feature B, some codes are executed first for feature B and then A. The order is important e.g for shape composition but it is not a concern in many other usages. E.g when overriding some implementations but called by other parts of code in the right order. Note: We can preserve some order by using the feature Priority and the startup Order.

@Piedone
Copy link
Member

Piedone commented Dec 19, 2018

I think there are two sides to this (as with feature dependencies):

  • When you enable a feature that needs the implementation of a service which comes from another (but not a specific) module then your site shouldn't blow up.
  • You shouldn't be able to blow up your site by disabling a feature that another feature is implicitly depending on.

What I suggest is being able to declare dependency on services:

Since this is about dependencies on the service level features could declare a set of services for which they require an external implementation (i.e. depend on). Like my feature requires an IService1 and IService2. Until there is an implementation present (the services can be resolved) the feature can't be enabled.

Conversely if a feature contains an implementation for IService1 and/or IService2 it couldn't be disabled if not at least 2 such implementations exist (so after disabling it one would still remain), not to break my feature.

We could support this in the module manifest as well, with UI warnings shown to the user.

I thought about solving this by having in addition to "hard" dependencies like today sets of "soft" dependencies but that wouldn't help in the case the implementation comes from a module unknown to you (i.e. you don't know its name).

@jtkech
Copy link
Member

jtkech commented Dec 20, 2018

Yes, very interesting but too busy to work on it immediately.

What I suggest is being able to declare dependency on services.

Here, if we define e.g a list of interface types, i think it is doable because we can resolve all the implementations of a given interface type. Then for each implementation type we already have a service that can retrieve for any exported type the related feature and then its name and description.

@kevinchalet
Copy link
Member Author

Having "services dependencies" would be a nice first step but it's worth noting that not everything is represented as a service in ASP.NET Core: in the token authentication example I gave, authentication handlers are given a unique name - called the authentication scheme - and registered via the AuthenticationOptions (we couldn't have a just check ensuring IAuthenticationHandler is registered because it doesn't tell you whether a handler with the API scheme exists).

@Piedone
Copy link
Member

Piedone commented Dec 20, 2018

Indeed, such very specific scenarios I think should be handled on a case by case basis. What I suggest would solve issues of the like of disabling Roles and thus breaking OpenId, see here: #2872 (comment).

@Piedone Piedone added this to the backlog milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants