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

[5.x] Fix #1419 #1424

Merged
merged 2 commits into from
Apr 25, 2024
Merged

[5.x] Fix #1419 #1424

merged 2 commits into from
Apr 25, 2024

Conversation

tmayrand
Copy link
Contributor

Fixes #1419

Updates to the BatchesController show() and retry() functions to prevent 500 XHR responses.

Copy link

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.

@tmayrand tmayrand marked this pull request as ready for review April 24, 2024 02:50
@driesvints
Copy link
Member

Wouldn't a 404 status code be more applicable here?

@taylorotwell
Copy link
Member

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.

@tmayrand
Copy link
Contributor Author

tmayrand commented Apr 25, 2024

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

horizon500

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.

@driesvints
Copy link
Member

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?

@tmayrand
Copy link
Contributor Author

@driesvints - Nothing appears in console. The rendered page does not change and still displays "Loading..."

Batch preview

image

Consistent with all the job preview screens:

Pending

image

Completed

image

Silenced

image

Failed

image

@driesvints
Copy link
Member

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

@driesvints driesvints reopened this Apr 25, 2024
@driesvints
Copy link
Member

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

@taylorotwell taylorotwell merged commit 0f08459 into laravel:5.x Apr 25, 2024
21 checks passed
@tmayrand tmayrand deleted the fix-1419 branch April 25, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchesController missing check for existence of batch
3 participants