Closed mpa12 closed 1 month ago
This is fine so far, though the main thing to resolve for this issue is to make our server return different response codes for invalid url combinations.
Currently every combination of parameters gives a 200 response, but there are some possible error cases where it is not "ok" and should set the http response code to something else.
For example:
Missing or invalid parameters could be a 422 (unprocessable entity)
missing lines parameter - https://readme-typing-svg.demolab.com?lines=
negative height, width, pause, duration, font size, font weight, etc - https://readme-typing-svg.demolab.com?width=-1&lines=X
If there is an issue getting a font from Google fonts, assuming an error message appears to the user, it could be 503 (service unavailable)
@DenverCoder1 As I understand it, you need query validation. In addition to statuses, maybe it can also return the error text? For example, if some parameter is missing, then write Missing parameter <parameter_name>
It already displays error messages, it just doesn't have different status codes for different errors, for example, this page shows the response code is 200, when it would be more accurate to say 422 since there was an error
One possible way to do this could be to pass the codes to the exceptions, and call http_response_code
on the error's code before rendering an error svg.
This is a PR to a similar issue in a different repository for reference: https://github.com/DenverCoder1/github-readme-streak-stats/pull/192/files
I didn't display an error message when I failed to get the font. This could cause animations to crash for some users. In the current version, if you can't get the desired font, the default font is used
Thanks so much for all the work you've put in to solving this!
While this does work, as far as the implementation, there are a few things I'd do differently for code readability and maintainability.
Constructors should not typically have side effects, in other words, using the construction of the exception to change the response code is unexpected and as a principle should be avoided.
I think the most expected place to find this would be in the RenderedController
class when setting the headers.
$statusCode
field could be added to RendererController
defaulting to 200
, but set in the constructor's catch block if an exception raised.500
as that would indicate an unknown server error occurred. Alternatively, the class of the exception could be checked.setHeaders()
method of RendererController
which is called from index.php
, the http_status_code()
function can be used to set the status code using the value in the status code field.Feel free to let me know your thoughts and if you have any questions.
Thanks
Quite a good point! I'll fix it
Thank you for your attention to my work and valuable comments! I took all your recommendations into account and corrected the errors
Summary
Added enumeration of HTTP response status codes
Resolves #59
Type of change
How Has This Been Tested?
composer test