REST Export Views format should be configurable

The new routing system can handle that config for us: routes can be restricted to only support certain formats given in the HTTP Accept headers. So we need to move the registration of routes from views_menu_alter() to a proper route subscriber in Views module. I think we should open an issue to convert Views to the new Routing system first.

We should certainly wait until the the CMF routing is in, though didn't it had the problem that you needed yml files which are kind of static?

Thank you for that information!

Just tracking some work here, This patch just adds options for the style plugin pretty much.

Good point, I will bear that in mind when I work on this next. Lucky it's just a wip ;)

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.phpundefined
@@ -60,6 +94,13 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+    $formats = array_filter($this->options['formats']);

Also just providing feedback :) We could filter that on a validateOptionsForm method.

Rerolled to include the feedback so far. I think this should just wait on #1800998: Use route system instead of hook_menu() in Views for the time being.

ok, now that #1800998: Use route system instead of hook_menu() in Views is in we can use routes and finish this issue. A 406 response will be returned by the routing system for invalid formats.

Thanks for the reviews/testing. Much appreciated. Have made those changes.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.phpundefined
@@ -59,7 +60,7 @@ class RestExport extends PathPluginBase {
+  protected $contentType = NULL;

nothing is also NULL :)

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.phpundefined
@@ -200,7 +195,33 @@ public function optionsSummary(&$categories, &$options) {
+      $formats = $style_plugin->getFormats();

It feels wrong that getFormats() doesn't return the formats

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.phpundefined
@@ -200,7 +195,33 @@ public function optionsSummary(&$categories, &$options) {
+    // Format as a string using pipes as a delimeter.
+    $requirements['_format'] = implode('|', $formats);

Should we check for formats?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.phpundefined
@@ -46,13 +46,56 @@ class Serializer extends StylePluginBase {
+    $container = \Drupal::getContainer();
...
+    $this->serializer = $container->get('serializer');
+    $this->formats = $container->getParameter('serializer.formats');

You know we can inject stuff now?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.phpundefined
@@ -46,13 +46,56 @@ class Serializer extends StylePluginBase {
+   * Overrides \Drupal\views\Plugin\views\display\DisplayPluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\display\DisplayPluginBase::buildOptionsForm().
...
+   * Overrides \Drupal\views\Plugin\views\PluginBase::submitOptionsForm().

@inheritdoc all the things.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.phpundefined
@@ -46,13 +46,56 @@ class Serializer extends StylePluginBase {
+      '#options' => drupal_map_assoc($this->formats),

Please let's us open a follow up to convert that function.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/StyleSerializerTest.phpundefined
@@ -125,11 +125,40 @@ public function testSerializerResponses() {
+  public function testReponseFormatConfiguration() {
+    $this->drupalLogin($this->adminUser);
+
+    $style_options = 'admin/structure/views/nojs/display/test_serializer_display_field/rest_export_1/style_options';
+
+    // Select only 'xml' as an accepted format.
+    $this->drupalPost($style_options, array('style_options[formats][xml]' => 'xml'), t('Apply'));
+    $this->drupalPost(NULL, array(), t('Save'));
+
+    // Should return a 406.
+    $this->drupalGet('test/serialize/field', array(), array('Accept: application/json'));

We agreed to split the tests into UI and the rest

Done everything except splitting up the tests. Just running out of time.

I think we should have a follow up now for the decoupling of tests. Seems slightly too far for this issues scope. Created #2004670: Split Views style serializer tests in to UI and functional tests

I would say so :) Also, just opened #2004798: Replace drupal_map_assoc with a utility class method

Me and Alex had a chat on IRC about this. As we cannot properly do content negotiation yet, we need to keep the 'fallback' format selection for html as we had before. When we improve the content negotiation, we can fix this.

I have added some @todo's in the code, as well as some additional test assertions for browser headers.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/StyleSerializerTest.phpundefined
@@ -151,6 +151,15 @@ public function testReponseFormatConfiguration() {
+    // Should return a 200. Emulates a sample firefow header.

What is firefow ;)

You are seriously behind the times if you aren't using the firefow browser yet!

Manual testing worked fine.

Just in case someone is interested, here is a try to write a proper unit test ... still not possible.

#25: 1891202-25.patch queued for re-testing.

ReRTBC

+++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/StyleSerializerTest.phpundefined
@@ -125,11 +125,49 @@ public function testSerializerResponses() {
+    // Should return a 200.
+    // @todo This should be fixed when we have better content negotiation.
+    $this->drupalGet('test/serialize/field', array(), array('Accept: */*'));
+    $this->assertResponse(200, 'A 200 response was returned when HTML was requested.');
+
+    // Should return a 200. Emulates a sample Firefox header.
+    $this->drupalGet('test/serialize/field', array(), array('Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'));
+    $this->assertResponse(200, 'A 200 response was returned when HTML was requested.');

The assertion messages here are incorrect. We're not requesting HTML - we're requesting something and saying we'll take anything :)

OK, how about this? As it's just a comment change, assigning to you Alex.

Seems fine for me.

Committed 1c61a22 and pushed to 8.x. Thanks!

Unassigning...