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.

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

Use proper error pages instead of always redirecting #34662

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Oct 18, 2022

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.

@juliushaertl juliushaertl requested review from nimishavijay, jancborchardt, CarlSchwan, a team, PVince81 and skjnldsv and removed request for a team October 18, 2022 18:31
@juliushaertl juliushaertl added enhancement 3. to review Waiting for reviews labels Oct 18, 2022
@juliushaertl juliushaertl 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)?

lib/base.php Outdated Show resolved Hide resolved
@juliushaertl
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)?

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
@juliushaertl juliushaertl 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 merged commit 9f2ecc7 into master Oct 21, 2022
@szaimen szaimen deleted the enh/error-pages branch October 21, 2022 14:38
@juliushaertl
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?

@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?

@juliushaertl
Copy link
Member Author

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

@juliushaertl juliushaertl removed the pending documentation This pull request needs an associated documentation update label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants