OrchardCore
OrchardCore copied to clipboard
OrchardCMS
Missing OC.Tenants Dependency on OC.Settings
Seems you need it in your PR #11890 ;) If it's like that I'm suggesting to do the changes in the other PR, so everyone will know what's the need for OC.Settings
@jtkech the last change is great! But wondering why aren’t we using something like IOptions<PagerOptions> instead? Similar approach was done in PR #11569 which remove the need of ISiteService altogether. This could probably be beneficial in other modules as well.
@CrestApps
Yes I added some comments in your PR, open to any suggestion but there are many scenario, see my comments in your PR.
For now, here it works as before and just fixes an issue I encountered.
Also, just to say that here it only uses ISiteService for the pager PageSize.
@jtkech I am not against what you did, but I think it’s a workaround not a fix. I think a fix could be by adding IOptions<PagerOptions> which will provides easy way to override options without having to implement ISiteService. My suggestion will involve a lot more work that you may want to avoid for now. Perhaps my suggestion could be a second PR at some point “unless you disagree with the approach”
Yes I understand.
I also thought about a default value in the Pager class itself.
As said there are many scenario, for example do we want all admin settings configurable? Not sure, maybe would need to be discussed more globally, or not I don't know, maybe better to consider each specific case, this is not because a pattern is good that we have to use it everywhere.
For now I think that managing IOptions<PagerOptions> here is too much, but yes I understand what you mean.
Yea I agree with you. We don’t use a pattern just because. Just for future reference, the reason I suggested introducing PagerOptions is because in many places in OC, we inject ISiteService only to get pager info like is done here and many other places like it. Having PagerOptions will fix your issue and help in many other cases. But, the workaround you provided will defiantly work for the time being.
That said for the example related to the OC.Contents module.
No problem as it depends on OC.Settings
[assembly: Feature(
Id = "OrchardCore.Contents",
Name = "Contents",
Description = "The contents module enables the edition and rendering of content items.",
Dependencies = new[]
{
"OrchardCore.Settings",
"OrchardCore.Liquid"
},
Category = "Content Management"
)]
Yes I saw that. Not looking at the code now to find out the real reason, but probably they ran into the same problem you ran into which lead you to this PR :)
Yes, maybe also for info because OC.Contents is part of the CMS modules meta package, whereas other modules are more intended to be standalone modules that only "depend" on the OC infrastructure.
Didn't read the comments, can the SiteSettings module be a dependency of Tenants?
Yes, that was my first way to do this
But I thought that it would be useful to not define this dependency for some scenario
But as you want, easy to do