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.

Sign up for GitHub

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

Jump to bottom

fix(setup-checks): Ensure URL with webroot works #47883

Merged
merged 2 commits into from
Sep 13, 2024
Merged

fix(setup-checks): Ensure URL with webroot works #47883

merged 2 commits into from
Sep 13, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 10, 2024 •
edited
Loading

Summary

We basically mock the way URLGenerator::getAbsoluteURL works, so we must make sure that the URL might already contain the webroot. Because baseURL and cliURL also contain the webroot we need to remove the webroot from the URL first.

Checklist

@susnux susnux added this to the Nextcloud 31 milestone Sep 10, 2024
@susnux susnux requested review from kesselb and come-nc September 10, 2024 23:22
@susnux
Copy link
Contributor Author

susnux commented Sep 10, 2024

/backport to stable30

@kesselb

This comment was marked as resolved.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@susnux susnux requested a review from solracsf September 11, 2024 09:28
@kesselb

This comment was marked as resolved.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@kesselb

This comment was marked as resolved.

Sign in to view
@susnux

This comment was marked as resolved.

Sign in to view
@kesselb
Copy link
Contributor

kesselb commented Sep 11, 2024

Too many forward slashes for me 🙈

@kesselb
Copy link
Contributor

kesselb commented Sep 11, 2024

/backport to stable29

@solracsf solracsf mentioned this pull request Sep 11, 2024
Closed
8 tasks
@kesselb kesselb mentioned this pull request Sep 11, 2024
Closed
8 tasks
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 12, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@susnux susnux requested a review from come-nc September 12, 2024 18:51
come-nc
come-nc approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I think fixing the wrong type in the phpdoc should fix psalm errors.
Nice job 👍

@come-nc
Copy link
Contributor

come-nc commented Sep 13, 2024

phpunit tests also need to be adapted to change from getSystemValue to getSystemValueString

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 13, 2024
View reviewed changes
apps/settings/lib/SetupChecks/CheckServerResponseTrait.php
if ($removeWebroot) {
$segments = parse_url($url);
$port = isset($segments['port']) ? (':' . $segments['port']) : '';
return $segments['scheme'] . '://' . $segments['host'] . $port;

Check notice

Code scanning / Psalm

PossiblyUndefinedArrayOffset Note

Possibly undefined array key $segments['scheme'] on array{fragment?: string, host?: string, pass?: string, path?: string, port?: int, query?: string, scheme?: string, user?: string}|false
apps/settings/lib/SetupChecks/CheckServerResponseTrait.php
if ($removeWebroot) {
$segments = parse_url($url);
$port = isset($segments['port']) ? (':' . $segments['port']) : '';
return $segments['scheme'] . '://' . $segments['host'] . $port;

Check notice

Code scanning / Psalm

PossiblyUndefinedArrayOffset Note

Possibly undefined array key $segments['host'] on array{fragment?: string, host?: string, pass?: string, path?: string, port?: int, query?: string, scheme?: string, user?: string}|false
susnux and others added 2 commits September 13, 2024 13:06
@susnux @kesselb
fix(setup-checks): Ensure URL with webroot works
9e979d4 
We basically mock the way `URLGenerator::getAbsoluteURL` works,
so we must make sure that the URL might already contain the webroot.
Because `baseURL` and `cliURL` also contain the webroot we need to remove
the webroot from the URL first.

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Daniel <mail@danielkesselberg.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @come-nc
refactor(settings): CheckServerResponseTrait always expect absolute…
be1cd7a 
… path

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux merged commit a90f0a6 into master Sep 13, 2024
174 checks passed
@susnux susnux deleted the fix/setup-checks branch September 13, 2024 11:42
This was referenced Sep 13, 2024
Merged
Merged
@solracsf solracsf mentioned this pull request Sep 13, 2024
Closed
8 tasks
@MichaIng MichaIng linked an issue Oct 11, 2024 that may be closed by this pull request
[Bug]: NC29 .well-known URLs, failed on: /.well-known/caldav #45033
Closed
8 tasks
@susnux susnux mentioned this pull request Oct 12, 2024
Closed
8 tasks
@rob-sve rob-sve mentioned this pull request Oct 31, 2024
Open
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb approved these changes

@come-nc come-nc come-nc approved these changes

@solracsf solracsf Awaiting requested review from solracsf

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: settings
Projects
None yet
Milestone
Nextcloud 31
4 participants
@susnux @kesselb @come-nc @solracsf