-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(setup-check): clarify server connectivity help text and improve trait docblocks #55640
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,19 @@ | |||||||||||||
| use Psr\Log\LoggerInterface; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Common trait for setup checks that need to use requests to the same server and check the response | ||||||||||||||
| * Trait CheckServerResponseTrait | ||||||||||||||
| * | ||||||||||||||
| * Provides common functionality for setup checks that require sending HTTP requests | ||||||||||||||
| * to the same server and analyzing the responses. | ||||||||||||||
| * | ||||||||||||||
| * This trait assists with: | ||||||||||||||
| * - Determining all possible server URLs to test, including trusted domains and CLI overrides. | ||||||||||||||
| * - Running HTTP requests with configurable options for SSL, error handling, and custom client options. | ||||||||||||||
| * - Generating helpful error messages when server connectivity is unavailable. | ||||||||||||||
| * - Normalizing URLs and building request options for consistent server checks. | ||||||||||||||
| * | ||||||||||||||
| * Intended only for use in Nextcloud setup checks. | ||||||||||||||
| * | ||||||||||||||
| * @since 31.0.0 | ||||||||||||||
| */ | ||||||||||||||
| trait CheckServerResponseTrait { | ||||||||||||||
|
|
@@ -27,21 +39,32 @@ trait CheckServerResponseTrait { | |||||||||||||
| protected LoggerInterface $logger; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Common helper string in case a check could not fetch any results | ||||||||||||||
| * Generates a help string explaining what needs to be configured | ||||||||||||||
| * for local server connectivity checks to succeed. | ||||||||||||||
| * | ||||||||||||||
| * Used primarily in the event a check is unable to fetch any results. | ||||||||||||||
| * | ||||||||||||||
| * @return string Local server configuration help text | ||||||||||||||
| * @since 31.0.0 | ||||||||||||||
| */ | ||||||||||||||
| protected function serverConfigHelp(): string { | ||||||||||||||
| $l10n = \OCP\Server::get(IFactory::class)->get('lib'); | ||||||||||||||
| return $l10n->t('To allow this check to run you have to make sure that your Web server can connect to itself. Therefore it must be able to resolve and connect to at least one of its `trusted_domains` or the `overwrite.cli.url`. This failure may be the result of a server-side DNS mismatch or outbound firewall rule.'); | ||||||||||||||
| // TODO: Technically it's necessary the web server, but the PHP SAPI. | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well its not about apache or nginx but the web server as the machine (or container) as the system of the webserver needs proper DNS setup
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well it's whatever PHP is running on plus the web server effectively, no? So in, say, an nginx plus PHP-FPM scenario, the query is going to come from the PHP-FPM server and depend on it reaching the web server. It's kinda "both". That's what I was getting at when it says "web server can connect to itself". It's a simplification. For some cases it's the same (effectively), but for others it may not be. And since it's a troubleshooting suggestion I was realizing it may be misleading to just say "web server". But I also haven't come up with a clear, simple way to say it that is both precise and also not overly vague or somewhat obscure/unclear for other reasons. ;-) Some ideas that sound kind of good to me:
But then the sentence structure gets funky. lol Maybe: return $l10n->t(
'This check failed because your PHP runtime environment was unable to connect to itself via your web server.' . "\n\n" .
'To fix this, please ensure:' . "\n" .
'- The server can resolve and connect to at least one of its configured `trusted_domains`, or the value set in `overwrite.cli.url`.' . "\n" .
'- There are no DNS mismatches, or outbound firewall rules blocking connections.'
); |
||||||||||||||
| return $l10n->t( | ||||||||||||||
| 'This check failed because your web server was unable to connect to itself.' . "\n\n" . | ||||||||||||||
| 'To fix this, please ensure:' . "\n" . | ||||||||||||||
| '- The server can resolve and connect to at least one of its configured `trusted_domains`, or the value set in `overwrite.cli.url`.' . "\n" . | ||||||||||||||
|
||||||||||||||
| 'This check failed because your web server was unable to connect to itself.' . "\n\n" . | |
| 'To fix this, please ensure:' . "\n" . | |
| '- The server can resolve and connect to at least one of its configured `trusted_domains`, or the value set in `overwrite.cli.url`.' . "\n" . | |
| "This check failed because your web server was unable to connect to itself.\n\n" . | |
| "To fix this, please ensure:\n" . | |
| "- The server can resolve and connect to at least one of its configured `trusted_domains`, or the value set in `overwrite.cli.url\n" . |
Outdated
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.
Does this work with the translation extraction?
I only know for frontend code where no operations are allowed within the t method.
Meaning it needs to be one string.
But not sure for PHP code.
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.
Explicit concatenation is fine. It'll be the same string passed to t(). Or it's supposed to be. The only other option for breaking up the long strings here I see is heredoc, but that can't be indented nicely (to match surrounding code) without the indentations ending up in the passed on string. (And I dislike having it be not indented with the code it's surrounded by).
Outdated
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.
This does neither provide additional information nor adds type information so just no @return needed.
Outdated
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.
same here
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.
Same here, no new information or types so no need for the annotation.