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

Recipe Image Endpoint should not return two different data types #954

Closed
Teifun2 opened this issue Apr 10, 2022 · 4 comments · Fixed by #982
Closed

Recipe Image Endpoint should not return two different data types #954

Teifun2 opened this issue Apr 10, 2022 · 4 comments · Fixed by #982
Labels
Backend Issue or PR related to the backend code php Pull requests that update Php code

Comments

@Teifun2
Copy link
Collaborator

Teifun2 commented Apr 10, 2022

As discussed in #418:

@Teifun2 if this needs more discussion, we might be better of in a new issue instead of this PR.

Apart from that, can't you check the Content-Type header before?

The problem for me is that i am using Network Cached Image widgets. These use they own implementation of the network controller. I can define the behaviour if the recource is not available. However the svg return results in a lot of exceptions as it is accepted as an image but can then not be parsed.

In my opinion it would be better to store the svg in the frontend and dispaly it in case images cannot be loaded, and just return 404 on the endpoint if no recipe image is defined.

This would make it so that the endpoint itself does not change data types.

(I would be willing to try and implement this, if it would be an accepted behaviour!)

@christianlupus
Copy link
Collaborator

Hello @Teifun2 and sorry for the delay. I am a bit busy these days...

Now I see your point. I would however rather avoid replacing the endpoint in a breaking way. I'd rather deprecate the current image endpoint and add another two:

  1. Get the primary image or return 404 if nothing was found
  2. Get a generic image as a fallback asset in case of 404.

Another thing to consider would be if the Accept header (reference) was set correctly by this widget you use. We could send different image formats depending on the requested image type from the backend without changing the API.

@seyfeb
Copy link
Collaborator

seyfeb commented Apr 14, 2022

And to add to Christian's answer: A recipe image needn't be a JPEG (even though this is probably the case in >99.9%). However, if someone decides to create svg drawings of her or his recipe, we shouldn't stand in the way 😅

@seyfeb seyfeb closed this as completed Apr 14, 2022
@seyfeb seyfeb reopened this Apr 14, 2022
@Teifun2
Copy link
Collaborator Author

Teifun2 commented Apr 18, 2022

@christianlupus The idea with Accept is very interesting. I assume this does not work so far?
I can defined additional Headers with the library so to restrict the "Accept" Header would be possible.

@christianlupus
Copy link
Collaborator

No, that is not yet implemented.

I think the solution with the headset might be the cleanest solution. Also, this can be implemented additionally to any possible other solution.

@christianlupus christianlupus added Backend Issue or PR related to the backend code php Pull requests that update Php code labels May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Issue or PR related to the backend code php Pull requests that update Php code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants