fix(setup-check): clarify server connectivity help text and improve trait docblocks #55640
Conversation
Updated the documentation for CheckServerResponseTrait to clarify functionality and improve readability. Also adjusted wording for the serverConfigHelp() output for clarity. Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
requested review from
icewind1991
and removed request for
a team
joshtrichards
added
the
♻️ refactor
| '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" . | ||
| '- There are no DNS mismatches, or outbound firewall rules blocking connections.' |
There was a problem hiding this comment.
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.
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).
| '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" . |
There was a problem hiding this comment.
Then maybe just
| '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" . |
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- "web stack"
- "Web server + application server" <-- probably most accurate
- "web server + PHP runtime environment" <-- probably the clearest
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.'
);| * | ||
| * @param bool $ignoreSSL If true, disables SSL verification. | ||
| * @param bool $httpErrors If true, sets whether HTTP error responses should trigger exceptions. | ||
| * @return array Options array for the HTTP client. |
There was a problem hiding this comment.
This does neither provide additional information nor adds type information so just no @return needed.
| * | ||
| * Used primarily in the event a check is unable to fetch any results. | ||
| * | ||
| * @return string Local server configuration help text |
There was a problem hiding this comment.
Same here, no new information or types so no need for the annotation.
| * | ||
| * @param string $url Absolute URL containing scheme, host, and optionally the webroot. | ||
| * @param bool $removeWebroot If true, removes the webroot from the URL, returning only the scheme, host, and optional port. | ||
| * @return string Normalized URL. |
Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
changed the title
joshtrichards
changed the title
Signed-off-by: Josh <josh.t.richards@gmail.com>
Summary
This PR updates the help message shown when server connectivity checks fail, making it clearer and more actionable for administrators.
Additionally, docblocks for the trait and its methods were improved for clarity.
No logic or API changes were made.
TODO
Checklist
3. to review, feature component)stable32)