Seal all Startup, Configuration and Filter classes

[MikeAlhayek]

No description provided.

[hishamco]

I'm not sure if it's good option to seal the startup classes

/cc @lahma

[Piedone]

Does this bring any benefit apart from aesthetics/principle? Like, do we gain something by compiler optimizations in this case?

[hishamco]

From what I see there're many of the files that have been modified are applicable for extension which is against the seal rule

[MikeAlhayek]

Do we gain something by compiler optimizations in this case?

yes. https://www.meziantou.net/performance-benefits-of-sealed-class.htm

[lahma]

With Jint we have general rule of internalize/seal everything by default and make public/unseal when there's a good reason for it. You can also seal specific overriding methods when it's not possible to seal the whole type.

[hishamco]

With Jint we have general rule of internalize/seal everything by default and make public/unseal when there's a good reason for it. You can also seal specific overriding methods when it's not possible to seal the whole type.

It depends because we don't want to seal something that could be extensible

[Piedone]

I did not expect to see 130 new files changed after adding three PR comments :D.

[MikeAlhayek]

I did not expect to see 130 new files changed after adding three PR comments :D.

I sealed all implementations of INavigationProvider and IPermissionProvider

[hishamco]

I never see anyone react to my comments, why do we seal INavigationProvider and IPermissionProvider implementation for instance, this will break any module extends admin menus and permissions?!!

[Piedone]

I'll defer to Mike for anything more involved, but no, that would only be the case if you extended the AdminMenu or Permissions class of another module. That should never be the case, and there is no instance of this in OC.

[MikeAlhayek]

@hishamco why would once ever need to extend these types of classes? If you need register permissions or navigation items you create a new class never extend these classes. + the methods in these classes are not marked virtual or abstract. So by nature you can't really override the methods that are there.

Is there a reason you think these should not be sealed?

[hishamco]

Let me clarify, in admin menus and permissions you're right, maybe it's our bad that we don't allow to override such methods, assume I created a custom module that wants to extend one of the OC admin menus or permission. Nowadays we need to rewrite the code from scratch

The right case is in startup, while the current methods are overridable, adding Seal prevents the inheritance

[MikeAlhayek]

Can you think of one case where you need to inherit from a Startup, PermissionProvider or NavigationProvider? What benefit would that provide you?

[hishamco]

For those who work on custom modules like in OCC, if I need to add a feature to a certain OC module either I copy the current code and then add my customization or create one from scratch, both are tedious, but if there's a possibility to override admin menu or permission I will easily use what OC has + my own

[MikeAlhayek]

Logically you can't override any of these class types not in OC or on OCC. If you don't like one of these providers, you'll have to remove it from IoC and register you own or replace implementation. Unless the methods are marked abstract, override or virtual, you can't really override them. and if you made these methods virtual, the you'll end up with duplicate registered permissions or menu items.

[hishamco]

Logically you can't override any of these class types not in OC or on OCC

Why?

and if you made these methods virtual, the you'll end up with duplicate registered permissions or menu items.

You can replace the services in IoC like what we did in OC.Localization module

[MikeAlhayek]

Logically you can't override any of these class types not in OC or on OCC

Try unsealing this class, and then override the methods. Can you do that?

OrchardCore/src/OrchardCore.Modules/OrchardCore.Sms/SmsPermissionProvider.cs

Line 7 in f1daaa7

public sealed class SmsPermissionProvider : IPermissionProvider

You can replace the services in IoC like what we did in OC.Localization module

Yes that is the recommended approach if the class provider you a way to override it's methods.

[hishamco]

Try unsealing this class, and then override the methods. Can you do that?

As I mentioned before, for permissions and menus we could mark them as overridable, but while we implement the interface this is not possible

[MikeAlhayek]

it's the same as adding a new implantation. For things like providers or event handler, is most cases these should not be overridden. Remember inheritance adds complexity to the code and not always a good choice. In this case, its a bad choice to override these classes which is why they are now sealed.

[hishamco]

Remember inheritance adds complexity to the code

Could you please elaborate?

I think it's good to make use of what the framework provides you plus adding your customization instead of creating a new implementation from scratch

[MikeAlhayek]

Not sure what is the debate here. You already agreed above that the PermissionProviders and MenuAdmin should be inhertited because they implement an interface and do not mark the methods virtual.

[hishamco]

I forgot at the beginning that we need to implement the interface for IPermissionProvider so making them sealed will prevent the inheritance which might be right currently but if anyone wants to extend the permission we could mark them virtual

If you looked into my first comment #16238 (comment) startup methods are overridable but marking the classes sealed makes the extension impossible

I will not go further on this, but the idea is that the types are sealed when it's suitable otherwise I expect all the types in ASP.NET Core are sealed if they think for PERF, but that's not the case

[MikeAlhayek]

But in OC we never inherit from Startup class because that just seems like a bad idea for OC. If you feel you need to override Startup class, you'll create a new feature that depend on the feature you want to override and then you can replace service. Never override a Startup class. Feel free to reach out to me offline if you have questions. But let table this debate here please.