search

laravel / horizon

Dashboard and code-driven configuration for Laravel queues.
https://laravel.com/docs/horizon
MIT License
3.88k stars 658 forks source link

[5.x] Fix #1419 #1424

Closed tmayrand closed 7 months ago

tmayrand commented 7 months ago

Fixes #1419

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

github-actions[bot] commented 7 months ago

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.

driesvints commented 7 months ago

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

taylorotwell commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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