Problem/Motivation

There is a leftover @todo in the \Drupal\rest\Plugin\views\display\RestExport class. From what I understand content negotiation is is set for the moment.

Proposed resolution

Remove the todo and clean up any related code.

Remaining tasks

Remove the todo and clean up any related code.

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#1 remove_todo_in-2541490-1.patch772 bytesisholgueras

Comments

isholgueras’s picture

isholgueras commented
Status: Needs work » Needs review
StatusFileSize
new remove_todo_in-2541490-1.patch772 bytes

I've tested almost every functionality in content negotiation for REST Export. I've checked the code that I think is a related code and everything work well. So I removed only the @todo line.

Correct me if I'm wrong.

novitsh’s picture

novitsh commented

If indeed the TODO is not relevant anymore, than the patch provided in #1 is good to go.

Jaesin’s picture

Jaesin commented 
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -126,7 +126,6 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
     $request_content_type = $this->view->getRequest()->getRequestFormat();
...
-    // @todo Remove the need for this when we have better content negotiation.
     if ($request_content_type != 'html') {
...
     }

I think the comment is saying to remove the if statement when content negotiation has improved to the point where $this->setContentType($this->view->getRequest()->getRequestFormat()) works without it. Meaning that a rest request should not return "html" in the first place unless there was a "html" serializer :(.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Closed (works as designed)

Agreed with #3. This @todo is still relevant.