Opened 8 years ago
Closed 8 years ago
#39066 closed defect (bug) (fixed)
`fetch_feed()` changes REST API response `Content-Type`
Reported by: | dlh | Owned by: | rmccue |
---|---|---|---|
Milestone: | 4.7.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Feeds | Keywords: | has-patch commit fixed-major |
Focuses: | rest-api | Cc: |
Description
I have a custom endpoint that returns data derived from widgets. WP_Widget_RSS::widget()
calls fetch_feed()
, which sends a Content-type: text/html
header via SimplePie::handle_content_type()
. So, if I don't send another application/json
header at the end of my callback, the response content type ends up text/html
.
This could be "fixed" by resending the application/json
header after running a route callback. But any function could still change the headers later, so resending the header seems a little arbitrary.
Another approach would be to, case-by-case, catch core functions that could inadvertantly change headers during an API response. The attached patch would implement this approach for fetch_feed()
. But having to apply the same fix to multiple functions over time could also get inefficient.
Of course, a third approach could be to call it a wontfix
based on nothing being "wrong" in either fetch_feed()
or the API.
Attachments (3)
Change History (14)
#3
@
8 years ago
I like this, but am wondering how many other places this is going to pop up. I'm in favor of performing the check just for feeds for now and am taking a stab at writing some tests around it.
#4
follow-up:
↓ 5
@
8 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 4.7.1
I would rather we simply not call handle_content_type()
. This only exists to set the charset in the header to match the feed, but we already set the charset to get_option( 'blog_charset' )
above, so it's not necessary. At best, it's redundant already.
#5
in reply to:
↑ 4
;
follow-up:
↓ 8
@
8 years ago
Replying to rmccue:
I would rather we simply not call
handle_content_type()
. This only exists to set the charset in the header to match the feed, but we already set the charset toget_option( 'blog_charset' )
above, so it's not necessary. At best, it's redundant already.
I can confirm this resolves the header "Content-type:" issue and have attached 39066.3.diff
I used to code below to verify functionality. However, I am questioning if it is okay to force the character set from a 3rd party to the same one used by the WP instance.
<?php /** * Grab 3rd party feed * * @param array $data Options for the function. * @return string URI of the feed. */ function my_custom_rest_endpoint( $data ) { $uri = 'https://make.wordpress.org/core/feed/'; $feed = fetch_feed( $uri ); return $uri; } // e.g.) http://src.wordpress-develop.dev/wp-json/custom/v1/feed add_action( 'rest_api_init', function () { register_rest_route( 'custom/v1', '/feed/', array( 'methods' => 'GET', 'callback' => 'my_custom_rest_endpoint', ) ); } );
#6
@
8 years ago
- Keywords has-patch added; needs-patch removed
- Owner set to rmccue
- Status changed from new to reviewing
@rmccue Can you review @stevenkword's patch and decide if this is committable or needs to be punted to 4.7.2?
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#8
in reply to:
↑ 5
@
8 years ago
- Keywords commit added
Replying to stevenkword:
However, I am questioning whether it is okay to force the character set from a 3rd party to the same one used by the WP instance.
This already happens (and has done so since we originally added SimplePie, I think) with the set_output_encoding
call. Internally, SimplePie uses mbstring/iconv to handle encodings, so I don't forsee any issues with it really.
It seems like it would be worth also checking if the request is an ajax or xmlrpc request, since this bug could impact those responses as well.