[#45419] - [5.3] Set http status header for XML/feed responses
- Pending
- Medium
- Build: 5.3-dev
- # 45419
- Diff
- ManuelHu:patch-2
User tests: Successful: Unsuccessful:
Pull Request for Issue #45398.
Summary of Changes
set the error code on the HTTP response
this is similar to the other error renderes (i.e. copied from https://github.com/joomla/joomla-cms/blob/5.3-dev/libraries/src/Error/Renderer/JsonRenderer.php)
Testing Instructions
-
set a menu item that shows an article as the home menu item
2, openhttps://<your-site>/?format=feed-- of course, this is not a useful feed link (I know that; thats the whole point. -
see a response such as
<?xml version="1.0" encoding="UTF-8"?>
<error>
<code>404</code>
<message>View nicht gefunden [Name, Typ, Präfix]: article, feed, site</message>
</error>- open the browser DevTools to see the http status code reported from the server
Actual result BEFORE applying this Pull Request
the HTTP response status is 200
Expected result AFTER applying this Pull Request
the HTTP response status (not the one listed as <code>404</code>) is actually 404
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
richard67
- | Title |
|
||||||
well, yes, if we assume that
- XML is only ever sent over HTTP (i.e. CLIApplication has no
setHeadermethod)
I am not sure if this is the case? - and that every exception sets a HTTP status (which is not the case). The default value is 0, and 0 is not a valid HTTP status
note: a similar check is not only implemented in JsonRenderer, but also in ErrorDocument
joomla-cms/libraries/src/Document/ErrorDocument.php
Lines 119 to 131 in 74e625d
also in the joomla-framework/application documentation:
https://github.com/joomla-framework/application/blob/e063a4a308729b930d853e29a9f7c303cac68629/README.md?plain=1#L54
However, this check is missing in other places. Also note, I am not an export in this (all of this just based on my code reviw of the last day), so I am not really sure what to decide here.
The instanceof check is fine, but I think ->setHeader('status', $error->getCode() < 400 ? 500 : $error->getCode()); would be enough. Or do you see an issue with it?
No, this should be equivalent in all typical cases. This very verbose code is just the same as already in the Json response, but I can certainly simplify the code.
| Labels |
Added:
bug
PR-5.3-dev
|
||
I have tested this item ✅ successfully on fbc2426
I see the network response code change from 200 to 404 after applying the patch and the Firefox Browser Tools show a problem icon. So it works as described but I can't comment on the experts comments.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45419.
Not sure but is it not enough to add this single line in the function
\Joomla\CMS\Factory::getApplication()->setHeader('status', $error->getCode());