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

Remove double database connection validation in setup #12826

Merged

Conversation

TFleury
Copy link
Contributor

@TFleury TFleury commented Nov 16, 2022

Removes double database connexion validation in SetupService as it's already done by IDbConnectionValidator.ValidateAsync.
Removes OrchardCore.Setup.Core dependency on YesSql.Abstractions

Fixes #12825

…in SetupService

Removes OrchardCore.Setup.Core dependency on YesSql.Abstractions
@Skrypt
Copy link
Contributor

Skrypt commented Nov 16, 2022

If you look at the comments it says that it could throw an exception if someone tries to use the same database with the same prefix. This is not something that the DB connection validator will test unless I'm wrong. Maybe it should then.

@TFleury
Copy link
Contributor Author

TFleury commented Nov 16, 2022

It's tested by DbConnectionValidator and there is a dedicated validator result for this case : DocumentTableFound
I tested the PR on my local env, and already used table prefix is detected.

@Skrypt
Copy link
Contributor

Skrypt commented Nov 16, 2022

I'm ok with this as long as we are sure that it is now impossible for the IStore to be null when resolved with scope.GetRequiredService<IStore>().

@TFleury
Copy link
Contributor Author

TFleury commented Nov 16, 2022

I wrapped shell descriptor manager initialization into a try/catch so it will always fail gracefully with error feedback in the UI.

@Skrypt Skrypt requested a review from jtkech November 16, 2022 21:15
Copy link
Member

@jtkech jtkech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one suggestion but in the meantime I still approved.

src/OrchardCore/OrchardCore.Setup.Core/SetupService.cs Outdated Show resolved Hide resolved
@hishamco hishamco changed the title Fixes #12825 : remove double database connexion validation in setup Fixes #12825 : remove double database connecion validation in setup Nov 19, 2022
@jtkech jtkech changed the title Fixes #12825 : remove double database connecion validation in setup Fixes #12825 : remove double database connection validation in setup Nov 19, 2022
@agriffard agriffard changed the title Fixes #12825 : remove double database connection validation in setup Remove double database connection validation in setup Dec 9, 2022
@agriffard agriffard merged commit a69c190 into OrchardCMS:main Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double database connexion validation in SetupService
4 participants