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

[1.0] Loaders should not throw http exceptions. #373

Closed
makasim opened this issue Mar 28, 2014 · 7 comments · Fixed by #403
Closed

[1.0] Loaders should not throw http exceptions. #373

makasim opened this issue Mar 28, 2014 · 7 comments · Fixed by #403
Labels
Attn: Blocker This item blocks other issue(s) or PR(s) and therefore must be resolved prior. Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality.

Comments

@makasim
Copy link
Collaborator

makasim commented Mar 28, 2014

No description provided.

@ama3ing
Copy link

ama3ing commented Apr 22, 2014

@makasim, I partially disagree with this. If source image can not be found on the server client should receive 404 otherwise it will be a bit weird. I can agree that if you call cache manager in controller like
$this->get('liip_imagine.cache.manager')->getBrowserPath('/relative/path/to/image.jpg', 'my_thumb', true) it is not very semantic to receive HttpNotFoundException, but in other cases, this exception should be thrown.

ama3ing pushed a commit to ama3ing/LiipImagineBundle that referenced this issue Apr 22, 2014
@makasim
Copy link
Collaborator Author

makasim commented Apr 22, 2014

@Me1ifaro loaders should not throw http exception but something like NotLoadableException like we already have. Later in the controller this exception could be wrapped by http exception. Think of cli command, what http exception means there?

@ama3ing
Copy link

ama3ing commented Apr 22, 2014

@makasim, already implemented in that way. If SourceNotFoundException in your opinion is not good, let's change this to NotLoadableException :)

@makasim
Copy link
Collaborator Author

makasim commented Apr 22, 2014

I dont like it either but it at least consistent with other exception we have already.

@makasim
Copy link
Collaborator Author

makasim commented Apr 22, 2014

@Me1ifaro could you open a PR?

@ama3ing
Copy link

ama3ing commented Apr 22, 2014

It's already opened, see #403. Actually now Travis build fails, but it is not caused by my changes. There is some madness in symfony dev-master

@makasim
Copy link
Collaborator Author

makasim commented Apr 23, 2014

Actually now Travis build fails, but it is not caused by my changes. There is some madness in symfony dev-master

yeah, I see it too, I am going to use only symfony 2.3 version on travis. I'll fix it today

ama3ing pushed a commit to ama3ing/LiipImagineBundle that referenced this issue Apr 23, 2014
makasim added a commit that referenced this issue Apr 28, 2014
…xceptions

Fixes #373. Replace NotFoundHttpException with SourceNotFoundException
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Attn: Blocker This item blocks other issue(s) or PR(s) and therefore must be resolved prior. Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants