-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
perf(settings): Speed up InternetConnectivity setup check #49977
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, that only speeds up things when internet connectivity is broken, no?
Okay, read the code, and it’s backwards for protocol I think, it should only check https if http fails.
How about: diff --git a/apps/settings/lib/SetupChecks/InternetConnectivity.php b/apps/settings/lib/SetupChecks/InternetConnectivity.php
index 3a0af06e71b..0b46ac2de5b 100644
--- a/apps/settings/lib/SetupChecks/InternetConnectivity.php
+++ b/apps/settings/lib/SetupChecks/InternetConnectivity.php
@@ -57,17 +57,15 @@ class InternetConnectivity implements ISetupCheck {
* @param string $site site domain or full URL with http/https protocol
*/
private function isSiteReachable(string $site): bool {
+ if (preg_match('/^https?:\/\//', $site) !== 1) {
+ // if there is no protocol, test http:// AND https://
+ $httpSite = 'http://' . $site . '/';
+ $httpsSite = 'https://' . $site . '/';
+ return $this->isSiteReachable($httpSite) || $this->isSiteReachable($httpsSite);
+ }
try {
$client = $this->clientService->newClient();
- // if there is no protocol, test http:// AND https://
- if (preg_match('/^https?:\/\//', $site) !== 1) {
- $httpSite = 'http://' . $site . '/';
- $client->get($httpSite);
- $httpsSite = 'https://' . $site . '/';
- $client->get($httpsSite);
- } else {
- $client->get($site);
- }
+ $client->get($site);
} catch (\Exception $e) {
$this->logger->error('Cannot connect to: ' . $site, [
'app' => 'internet_connection_check', ? But adding https to the default values would still make sense I suppose. |
Specify default protocol (https://) rather than let default handling test both http:// and https:// Signed-off-by: Josh <[email protected]>
2a79fbf
to
35cb5d8
Compare
Summary
Specify default protocol (https://) for the default domains rather than let default handling trigger both http:// and https:// tests.
Reduces network category check time by >20% (#49978) and overall setup check time by ~16% in my test environment.
TODO
Checklist