REST views: JSONP callback support

@andrea.paiola can you please add some pointers to specs etc plus

Please update the issue summary by applying the template from http://drupal.org/node/1155816.

Here's the piece of code that I've used for testing this problem in RestExport.php

    $callback = $this->view->getRequest()->get('callback');
    $response = new JsonResponse(drupal_render_root($output), 200, array());
    $response->setCallback($callback);
    return $response;

See http://en.wikipedia.org/wiki/JSONP

This needs handling single entities like node/1 too.

As this is a feature request I'm not sure we will get this into 8.0.x or should we declare this a bug?

I've created a patch based on #3 @pcambra why didn't you?

I've created a patch based on #3 @pcambra why didn't you?

I was going to tomorrow, lack of time :). Was working on #2396803: Remove unused test view in rest module and #2396253: Respect format configuration on REST views display today.

This needs handling single entities like node/1 too.

Agreed. I'll try to take a look to that.

1. +++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
   @@ -69,6 +69,8 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +    echo __METHOD__ . PHP_EOL;
   +    echo print_r($data, true) . PHP_EOL;
   

   Left over debug statement.

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -271,6 +272,17 @@ public function execute() {
   +    // In JSONP mode?
   +    $callback = $this->view->getRequest()->get('callback');
   +    $jsonp = $this->view->getRequest()->get('jsonp');
   +    if ($callback || $jsonp) {
   +      if ($jsonp) {
   +        $callback = $jsonp;
   ...
   +      $response = new JsonResponse(drupal_render_root($output), 200, array());
   +      $response->setCallback($callback);
   +      return $response;
   +    }
   

   What about using something like

   // In JSONP mode?
   $callback = $this->view->getRequest()->get('jsonp') ?: $this->view->getRequest()->get('callback');
   if ($callback) {
     $response = new JsonResponse(drupal_render_root($output), 200, array());
     $response->setCallback($callback);
     return $response;
   }

   which seems easier to read?

Thanks for the review @dawehner!

Here's a patch taking into account #7

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -271,6 +272,15 @@ public function execute() {
+      $response = new JsonResponse(drupal_render_root($output), 200, array());

Let's inject the 'renderer' service and use it here ... drupal_render_root() itself is deprecated already.

Another take fixing #9.

Had a chat with @dawehner on IRC about the entity resource JSONP. EntityResource uses ResourceResponse that has nothing special to do with json and all end up in RequestHandler where the response is encoded without taking into account a callback. From RequestHandler->handle()

    $data = $response->getResponseData();
    if ($data != NULL) {
      $output = $serializer->serialize($data, $format);
      $response->setContent($output);
      $response->headers->set('Content-Type', $request->getMimeType($format));
    }

Daniel suggests that we could add an event listener to catch the request and check if it's jsonp to replace the Response by a JsonResponse at that point but that might result in losing some information about the request. Maybe this belongs to another issue just for the entity resources and keep this one for views.

This seems fixed, but I can't figure out when. Feel free to reopen.

This seems to be a real ongoing issue. Re-opening.

I'm not too familiar with views in D8 but this patch I created solves the problem for me. Not sure if this is the best way to handle it.

Re-rolled the patch with an "isset" check.

We should at least add some form of testing here

This will not work as we cache requests based on _format.

Running

drush cache-rebuild
curl --request GET "http://drupal.d8/node?format=_json"
curl --request GET "http://drupal.d8/node?format=_json&callback=x"

gives both plain JSON.

I failed to get curl --request GET 'http://drupal.d8:8080/node/1?_format=json&callback=x' which should be another issue I guess.

Adding "url.query_args:callback" to the cache contexts resolved caching issue. Unsure if this is the best method to add context, but it worked for me:

$build['#cache']['contexts'][] = "url.query_args:callback";

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -281,13 +282,20 @@ public function collectRoutes(RouteCollection $collection) {
   +    $build['#cache']['contexts'][] = "url.query_args:callback";
   

   The cache context is set here, but the callback is being specified elsewhere.

2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -314,7 +322,16 @@ public function render() {
   +    $callback = $this->view->getRequest()->get('jsonp') ?: $this->view->getRequest()->get('callback');
   +    if ($callback) {
   +      $this->view->element['#content_type'] = 'text/javascript';
   +      $this->view->element['#jsonp_callback'] = $callback;
   

   This is where the callback is being added. Therefore this is where the cache context should be added.

   This makes it much easier to maintain in the long term.

… but actually even that is not enough, as you can tell because of the test failures. Due to the fact that Views has its own render pipeline and we want to be able to cache views efficiently, it's the responsibility of Views handlers/plugins to specify cacheability metadata ahead of time even. So, you'll want to make RestExport implement CacheableDependencyInterface. See \Drupal\user\Plugin\views\access\Permission() for an example.

#1869548: Opt-in CORS support is RTBC, which further reduces the need for this. Remember, JSONP is read-only, CORS is not.

This should do it.

And a reroll for 8.3.x.

Still needs tests. Other than that, looks good!

Actually, with only 10 followers on this issue, 8 of which are commenters on this issue… I think there's so little interest in this feature that it's simply not worth our time.

Also let's not forget that JSONP was invented as a work-around for CORS restrictions. So this would have made sense in 2013, when this issue was first created, but not today, in 2017.

And, quoting https://en.wikipedia.org/wiki/JSONP:

Due to inherent insecurities JSONP is being replaced by CORS.

So this would need security review too!

As of Drupal 8.2.0, we have opt-in CORS support: https://www.drupal.org/node/2715637. So, that even further reduces the need for this feature.

Therefore, closing this.