Add a way to queue releasing shell context from setting display drivers #15875
Conversation
WalkthroughWalkthroughThe modifications across OrchardCore modules primarily focus on streamlining the management of shell contexts. This includes removing direct dependencies on Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Out of diff range and nitpick comments (13)
src/OrchardCore.Modules/OrchardCore.Search.Elasticsearch/Drivers/ElasticSettingsDisplayDriver.cs (1)
Line range hint
136-138: Queue shell context release only after changes.Consider queuing the release of the shell context only after confirming that changes have been made to the settings. This avoids unnecessary operations if no settings were altered.
src/docs/releases/1.9.0.md (12)
Line range hint
7-7: Add a space between sentences for better readability.
Line range hint
9-9: Add a space between sentences for better readability.
Line range hint
25-25: Correct the typo: change "nodep" to "node".- services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>()); + services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>());
Line range hint
39-39: The phrase "sub-classes" is normally spelled as one word: "subclasses".
Line range hint
84-84: Consider adding a comma after "work" for better readability.- The `OrchardCore_Twitter` key continues to work but will be deprecated in a future release. + The `OrchardCore_Twitter` key continues to work, but will be deprecated in a future release.
Line range hint
195-195: Add a space between sentences for better readability.
Line range hint
197-197: Add a space between sentences for better readability.
Line range hint
199-199: Add a space between sentences for better readability.
Line range hint
238-238: Add a space between sentences for better readability.
Line range hint
238-238: Add a space between sentences for better readability.
398-398: Add a space between sentences for better readability.
398-398: Add a space between sentences for better readability.
src/OrchardCore.Modules/OrchardCore.Sms/Drivers/TwilioSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Email.Azure/Drivers/AzureEmailSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Localization/Drivers/LocalizationSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
...Core.Modules/OrchardCore.Search.AzureAI/Drivers/AzureAISearchDefaultSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Email.Smtp/Drivers/SmtpSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.OpenId/Drivers/OpenIdClientSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
…dCMS/OrchardCore into ma/queue-release-settings
|
When there's no corresponding issue, and especially if dozens of files are changed in non-trivial ways, please always add a description to the PR. What's the goal and why? What's the context? Otherwise, the first step for the reviewer is to try to reverse-engineer these from the changes, which makes it much harder. A lot more of my brain capacity remains for the actual review if I don't have to deplete it on step zero :). |
|
@Piedone start by looking at the release file where I document the change. I'll copy and paste it in the description |
MikeAlhayek
changed the title
|
Much better, thanks! |
src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Settings/SiteExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Email/Drivers/EmailSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Facebook/Drivers/FacebookSettingsDisplayDriver.cs
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.OpenId/Drivers/OpenIdClientSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (8)
src/docs/releases/1.9.0.md (8)
Line range hint
7-7: Add a space between sentences for better readability.- Release date: Not yet released ## Breaking Changes + Release date: Not yet released. ## Breaking Changes
Line range hint
9-9: Correct the spelling mistake.- The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. + The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSQL** and **OrchardCore**.
Line range hint
9-9: Add a space between sentences for better readability.- Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. + Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities.
Line range hint
84-84: Correct the spelling mistake.- The `Twitter` module has been rebranded to `X`. + The `Twitter` module has been rebranded to `X`.
Line range hint
203-203: Correct the spelling mistake.- In the past, we utilized the injection of `ISmsProvider` for sending SMS messages. + In the past, we utilized the injection of `ISmsProvider` for sending SMS messages.
Line range hint
209-209: Correct the spelling mistake.- Previously, these signatures accepted the `BuildEditorContext` parameter. + Previously, these signatures accepted the `BuildEditorContext` parameter.
Line range hint
286-286: Add a space between sentences for better readability.- When incorporating `INavigationProvider` in your project, you can now utilize `NavigationHelper.IsAdminMenu(name)` instead of the previous approach using `string.Equals(name, "admin", StringComparison.OrdinalIgnoreCase)`. + When incorporating `INavigationProvider` in your project, you can now utilize `NavigationHelper.IsAdminMenu(name)` instead of the previous approach using `string.Equals(name, "admin", StringComparison.OrdinalIgnoreCase)`.
Line range hint
346-346: Correct the spelling mistake.- You can reach Create at `~/Admin/Person/Create` (route name `PersonCreate`) and Edit for the person whose identifier string is "john-doe" at `~/Admin/Person/john-doe` (route name `PersonEdit`). + You can reach Create at `~/Admin/Person/Create` (route name `PersonCreate`) and edit for the person whose identifier string is "john-doe" at `~/Admin/Person/john-doe` (route name `PersonEdit`).
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (5)
src/docs/releases/1.9.0.md (5)
Line range hint
7-9: Add a space between sentences for better readability.- released ## Breaking Changes ### Drop `Newtonsoft.Json` Support The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. To ensure compatibility with `System.Text.Json` during the serialization or deserialization of objects, the following steps need to be undertaken: + released + + ## Breaking Changes + + ### Drop `Newtonsoft.Json` Support + + The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. To ensure compatibility with `System.Text.Json` during the serialization or deserialization of objects, the following steps need to be undertaken:
Line range hint
25-25: Correct the typo in "admin menu nodep".- services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>()); + services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNodeProvider>());
Line range hint
39-39: The term "sub-classes" is normally spelled as one word: "subclasses".- services.AddJsonDerivedTypeInfo<UrlCondition, Condition>(); + services.AddJsonDerivedTypeInfo<UrlCondition, ConditionSubclass>();
Line range hint
84-84: Add a comma before "but" when it connects two independent clauses.- The `OrchardCore_Twitter` key continues to work but will be deprecated in a future release. + The `OrchardCore_Twitter` key continues to work, but will be deprecated in a future release.
Line range hint
197-197: Correct the grammatical number: "text file" should be plural "text files".- Additionally, if you need to enable indexing for text file with `.txt`, `.md` extensions, you will need the `OrchardCore.Media.Indexing.Text` feature. + Additionally, if you need to enable indexing for text files with `.txt`, `.md` extensions, you will need the `OrchardCore.Media.Indexing.Text` feature.
src/OrchardCore/OrchardCore/Shell/ShellContextReleaseService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Is the PR ready for another review? |
@Piedone yes |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (4)
src/docs/releases/1.9.0.md (4)
Line range hint
7-9: Add a space after the period in the sentence about Newtonsoft.Json support.- The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**.Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. + The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities.
Line range hint
88-90: Consider adding a comma before "which" in the description of theLogin.cshtmlchanges for clarity.- The previous `AfterLogin` zone, which allowed filters for injecting shapes has been replaced. + The previous `AfterLogin` zone, which allowed filters for injecting shapes, has been replaced.
Line range hint
203-205: Clarify the necessity of enabling the Twilio provider in the SMS Module section to avoid confusion.- Additionally, `Twilio` provider is no longer enabled by default. If you want to use Twilio SMS provider, you must enable the provider by visiting the email settings `Configuration` > `Settings` > `Email` and see the `Twilio` tab. + Additionally, the `Twilio` provider is no longer enabled by default. If you want to use the Twilio SMS provider, you must explicitly enable it by visiting the email settings under `Configuration` > `Settings` > `Email` and selecting the `Twilio` tab.
Line range hint
195-199: Add a space after the period in the sentence about enabling the- Now, if you want to continue to index `.PDF` file you'll need to enable the `OrchardCore.Media.Indexing.Pdf` feature. + Now, if you want to continue to index `.PDF` file, you'll need to enable the `OrchardCore.Media.Indexing.Pdf` feature.
src/OrchardCore.Modules/OrchardCore.Twitter/Drivers/TwitterSettingsDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Shell/IShellContextReleaseService.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Security/Drivers/SecuritySettingsDisplayDriver.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Shell/IShellContextReleaseService.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore/Shell/ShellContextReleaseService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <[email protected]>
Co-authored-by: Zoltán Lehóczky <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (9)
src/OrchardCore/OrchardCore.Abstractions/Shell/IDeferredShellContextReleaseService.cs (2)
8-10: Ensure the methodRequestReleaseis adequately documented to explain its effects and any side conditions.
13-15: Clarify the documentation forSuspendReleaseRequestto specify what happens if called multiple times or afterRequestRelease.src/docs/releases/1.9.0.md (7)
Line range hint
192-192: Ensure fenced code blocks are surrounded by blank lines for proper markdown rendering.
Line range hint
193-193: Headings should be surrounded by blank lines to maintain consistency and readability in markdown files.
Line range hint
250-250: Remove trailing spaces to clean up the markdown file and adhere to best practices.
Line range hint
261-261: Avoid multiple consecutive blank lines to maintain a clean and professional appearance in the markdown file.
Line range hint
358-358: Specify a language for code blocks to enable syntax highlighting and improve readability.
Line range hint
360-360: Ensure fenced code blocks are surrounded by blank lines for proper markdown rendering.
Line range hint
361-361: Lists should be surrounded by blank lines to maintain consistency and readability in markdown files.
src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Abstractions/Shell/Scope/ShellScope.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Security/Drivers/SecuritySettingsDisplayDriver.cs
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (4)
src/docs/releases/1.9.0.md (4)
Line range hint
7-9: Add a space after periods to separate sentences for better readability.- released ## Breaking Changes ### Drop `Newtonsoft.Json` Support The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. To ensure compatibility with `System.Text.Json` during the serialization or deserialization of objects, the following steps need to be undertaken: + released. ## Breaking Changes ### Drop `Newtonsoft.Json` Support The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. To ensure compatibility with `System.Text.Json` during the serialization or deserialization of objects, the following steps need to be undertaken:
Line range hint
25-25: Correct the typo in "nodep" to "node".- services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>()); + services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>());
Line range hint
39-39: The phrase "sub-classes" is typically written as one word: "subclasses".- services.AddJsonDerivedTypeInfo<UrlCondition, Condition>(); + services.AddJsonDerivedTypeInfo<UrlCondition, Condition>();
Line range hint
195-195: Add a comma after ".PDF" for better readability.- Now, if you want to continue to index `.PDF` file you'll need to enable the `OrchardCore.Media.Indexing.Pdf` feature. + Now, if you want to continue to index `.PDF` file, you'll need to enable the `OrchardCore.Media.Indexing.Pdf` feature.
|
@Piedone I like to do one last rename here. What do you think of |
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (5)
src/OrchardCore.Modules/OrchardCore.Security/Drivers/SecuritySettingsDisplayDriver.cs (1)
Line range hint
77-102: Consider making the shell release request conditional on the model state's validity to avoid unnecessary operations.- _shellReleaseManager.RequestRelease(); + if (context.Updater.ModelState.IsValid) { + _shellReleaseManager.RequestRelease(); + }src/docs/releases/1.9.0.md (4)
Line range hint
7-9: Add a space between sentences for better readability.- The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**.Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities. + The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. Instead, we have transitioned to utilize `System.Text.Json` due to its enhanced performance capabilities.
Line range hint
9-9: Correct the spelling of "YesSql" to "YesSQL" for consistency with common naming conventions.- The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSql** and **OrchardCore**. + The utilization of [Newtonsoft.Json](https://www.nuget.org/packages/Newtonsoft.Json) has been discontinued in both **YesSQL** and **OrchardCore**.
Line range hint
25-25: Correct the typo in "nodep" to "node".- services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>()); + services.AddSingleton<IAdminNodeProviderFactory>(new AdminNodeProviderFactory<PlaceholderAdminNode>());
Line range hint
39-39: Combine "sub-classes" into one word: "subclasses".- services.AddJsonDerivedTypeInfo<UrlCondition, Condition>(); + services.AddJsonDerivedTypeInfo<UrlCondition, Condition>();
src/OrchardCore.Modules/OrchardCore.Twitter/Drivers/TwitterSettingsDisplayDriver.cs
Show resolved
Hide resolved
...Core.Modules/OrchardCore.Search.AzureAI/Drivers/AzureAISearchDefaultSettingsDisplayDriver.cs
Show resolved
Hide resolved
|
@Piedone anything else you like to add here, or should we merge it? |
|
As you've seen, I'm quite responsive with reviews, so rest assured that if you requested review, or asked something, I'll be there shortly :). |
|
|
||
| ShellScope.AddDeferredTask(async scope => | ||
| { | ||
| if (!_release) |
There was a problem hiding this comment.
Does it work? The closure is not obvious here, it's ambiguous if this will be the value when the lambda is added or if it will evaluate the boolean once it is executed (what you want here).
A more robust option could be to set a flag in ShellScope, with ShellScope.Set/Get.
There was a problem hiding this comment.
I tried and it did work, though perhaps not in an exhaustive way.
There was a problem hiding this comment.
I was sure it works and you tried it, but reading the code I wondered how the value was copied. Which is why we usually have to copy it in closures to be sure it's not changed from the outside. I think the current implementation is fine, we could add a comment to disambiguate. I will definitely have the same question next time I see it.
There was a problem hiding this comment.
Indeed it works @sebastienros I also thought it is very weird to get that value with in the deferred call back.
There was a problem hiding this comment.
@sebastienros I made the changes you requested here #15960
…rs (#15875) Co-authored-by: Zoltán Lehóczky <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fix #15884
Goals
Summary
Reloading Tenants
A new extension was added to
SignalReleaseShellContext()extension methodHttpContextto allow you to signal the app to release the shell context after the current request finish executing. In most cases, it is better to use the new extension over manually releasing the shell usingawait _shellHost.ReleaseShellContextAsync(_shellSettings). In you want to suspend the signal or restating the shell before the request is finish executing, you can invoke theHttpContext.ConcealReleaseShellContext()extension which will conceal the signal and the tenant will not be reloaded.Reloading Tenants Display Drivers
When implementing a site settings display driver, you have the option to reload the shell (restart the tenant). If you choose to do so, manually releasing the shell context using
await _shellHost.ReleaseShellContextAsync(_shellSettings)is unnecessary. Instead, you should use the newHttpContext.SignalReleaseShellContext(). This ensures that the shell stays intact until all settings are validated.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Chores