-
Notifications
You must be signed in to change notification settings - Fork 660
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
[5.x] Fix #1419 #1424
[5.x] Fix #1419 #1424
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
Wouldn't a 404 status code be more applicable here? |
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
@taylorotwell - From a consistency standpoint, other pages in the dashboard do not expose the exception that the batch preview does when the ID is not found. I don't normally work with Vue, so not sure if the 500s (or 404s to Dries' point) in the console are normal or intended behavior. If that is the case, my mistake, I have more to learn. |
I still want to try to get this one in tbh as this has come up before... the errors just don't sit right imo. @tmayrand so what does the screen look like if we apply the changes from this PR? |
@driesvints - Nothing appears in console. The rendered page does not change and still displays "Loading..." Batch previewConsistent with all the job preview screens: PendingCompletedSilencedFailed |
@tmayrand I think ideally we'd want to show a "not found" error message but that could be a bigger project. I agree that for now it's best to be consistent with other screens and then maybe improve things with a dedicated 404 message in a different PR. |
@taylorotwell gonna re-open this again for the reasons above. Right now, the behaviour here isn't wanted (500 server errors in the console log). This PR makes the behaviour consistent with other not found resources. I think we should merge this. |
Fixes #1419
Updates to the BatchesController
show()
andretry()
functions to prevent 500 XHR responses.