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
Display error when Mantis log file is not writable #1483
Conversation
This PR is built on top of #1482, which should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test how this works with API calls? I wonder if we need to be using exceptions vs triggering errors.
Thanks for the feedback @vboctor. I'm not sure I understand what kind of testing you expect me to do, and what you mean with needing exceptions, as the new code does not trigger any errors (see changes in logging_api.php), it just adds a message to the queue for display at page bottom, and prints it to the system log as well (which would be how the problem would be logged in the case of an API call). Are you saying that I should instead be throwing an exception to interrupt the execution flow ? I can certainly do that, but my thinking was that a failure to log is not a blocking error. |
@vboctor you have not responded to #1483 (comment). |
87ff2c8
to
8ecf8a9
Compare
@vboctor without feedback from you, I'll go ahead and merge this coming week-end. |
Encapsulates the logic to enqueue error messages for later display. Issue #19642
8ecf8a9
to
bfc3827
Compare
@dregad The thing I wanted to make sure doesn't happen is that if:
The result shouldn't be that execution stops, but API is successful. This may not be an issue due to different error handlers, but it is worth making sure that is the case. |
@vboctor thanks for your feedback, I understand what you mean now. Will test and confirm that the system behaves per your expectation. |
@vboctor the test is successful - the system behaves as per your expectations, just as I thought it would. Testing scenario
|
Thanks @dregad for testing this. |
Replaces PR #606
Fixes #19642
Here's how it looks...