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

Use proper error pages instead of always redirecting #34662

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Use proper error pages instead of always redirecting #34662

merged 1 commit into from
Oct 21, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 18, 2022 •
edited
Loading

Implements a generic 404 error page instead of always redirecting to the login page or dashboard as discussed in #33048

Can be tested by browsing to an invalid url like /index.php/apps/nonexisting

Screenshot 2022-10-18 at 20 30 14

Added the pending documentation label as we need to adjust the nginx config in the docs accordingly to what was done in the htaccess.

@juliusknorr juliusknorr requested review from nimishavijay, jancborchardt, CarlSchwan, a team, PVince81 and skjnldsv and removed request for a team October 18, 2022 18:31
@juliusknorr juliusknorr added enhancement 3. to review Waiting for reviews labels Oct 18, 2022
@juliusknorr juliusknorr added the pending documentation This pull request needs an associated documentation update label Oct 18, 2022
@szaimen
Copy link
Contributor

szaimen commented Oct 18, 2022

Does this close nextcloud/android#10628 (comment)?

szaimen
szaimen reviewed Oct 18, 2022
View reviewed changes
lib/base.php Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member Author

Does this close nextcloud/android#10628 (comment)?

It would help but doesn't explain why the redirect is happening. The 404 should already be returned by the dav app, so it might just hide another issue. But I cannot reproduce this case either. Maybe it is related to the general webserver configuration (besides .htaccess)?

CarlSchwan
CarlSchwan approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

😍

Finally the current behavior was driving me insane some times!

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 20, 2022
@szaimen
Copy link
Contributor

szaimen commented Oct 20, 2022

autoloaders not up-to-date

@szaimen szaimen added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Oct 20, 2022
@juliusknorr
Use proper error pages instead of always redirecting
11bedf1 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 21, 2022
@szaimen szaimen enabled auto-merge October 21, 2022 13:22
@szaimen szaimen merged commit 9f2ecc7 into master Oct 21, 2022
@szaimen szaimen deleted the enh/error-pages branch October 21, 2022 14:38
@juliusknorr
Copy link
Member Author

Nginx config does't seem to have this anymore since a long time:

https://github.com/nextcloud/documentation/pull/392/files

Does anyone have a nginx setup at hand to check if this is properly working there?

@ChristophWurst ChristophWurst mentioned this pull request Dec 5, 2022
Merged
5 tasks
@szaimen szaimen added this to the Nextcloud 26 milestone Dec 6, 2022
@DaphneMuller
Copy link

hello @juliushaertl ,
Thank you for your work on this pull request! This ticket seems to have the tag 'missing documentation', is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@juliusknorr
Copy link
Member Author

Documentation is already up to date, but I've added a note for administrators with steps to take in #34692

@juliusknorr juliusknorr removed the pending documentation This pull request needs an associated documentation update label Feb 21, 2023
@eppfel eppfel mentioned this pull request Mar 5, 2023
Closed
9 tasks
@juliusknorr juliusknorr mentioned this pull request Jul 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szaimen szaimen szaimen left review comments

@skjnldsv skjnldsv skjnldsv approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@PVince81 PVince81 Awaiting requested review from PVince81

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
5 participants
@juliusknorr @szaimen @DaphneMuller @skjnldsv @CarlSchwan