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

Don't rethrow a type error #50498

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Jan 28, 2025

Follow-up for #49004
Alternative to #49960

Summary

Rethrowing may trigger a weird code path in remote.php1 and therefore it's better to replace the type error with a generic one. The actual expection is still logged.

Test cases for manual testing:

TODO

  • Review
  • CI
  • Update existing backports for 49004

Checklist

Footnotes

  1. https://github.com/nextcloud/server/blob/2c773033bcf44268cad1e427fffdb9bbcc6a0327/remote.php#L27-L29

@kesselb kesselb force-pushed the bug/48678/restore-dav-error-response-2 branch from 80f053a to 5ddc520 Compare January 28, 2025 11:49
@kesselb kesselb self-assigned this Jan 28, 2025
@kesselb kesselb added bug 3. to review Waiting for reviews labels Jan 28, 2025
@kesselb kesselb added this to the Nextcloud 32 milestone Jan 28, 2025
try {
$this->emit('exception', [$e]);
} catch (\Exception $ignore) {
$e = new TypeError('A type error occurred. For more details, please refer to the logs, which provide additional context about the type error.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$e = new TypeError('A type error occurred. For more details, please refer to the logs, which provide additional context about the type error.');
$e = new TypeError('A type error occurred. For more details, please refer to the logs, which provide additional context about the type error.', 0, $e);

Would it be a problem to store the previous exception? It seems it’s only part of the response in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question 👍

The debugExceptions blocks appear to be dead code because there's no code in server setting it to true. Sounds like a nice enhancement to also include a stack trace, when debug enabled, for dav requests.

For non-dav requests we already include the stack trace with debug true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants