REST views: Pass views style plugin instance to REST Export serializer

Looks good. Is there anyway we can test it manually? or possible to write test?

Does $this->options contain information you actually want in there? Aren't those two different kind of options?

Well yeah I just fear that random configuration of one might leak into the serializer and then cause something which was not intended to do so.

+++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
@@ -136,7 +136,7 @@ public function render() {
-    return $this->serializer->serialize($rows, $content_type);

I think we should not pass all the options on the top level of the $context array for the serializer. So something like return $this->serializer->serialize($rows, $content_type, ['views_style_options' => $this->options])

For the test case I think a unit test that asserts that the context is passed to a serializer mock should suffice. Extra points if you use Prophecy for the mocking, see https://www.drupal.org/node/2554745

This is apparently a contrib project blocker (for https://www.drupal.org/project/views_data_export). Tagging as such.

For consistency with the rest of that method, either use old-style array() syntax, or fix the line:

 $rows = array();

Here's a unit test that tries to keep the mocking to the minimum needed, and fixed the syle concern.

Not really very useful, but it's a start for future test writing around this.

1. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -136,7 +136,7 @@ public function render() {
   +    return $this->serializer->serialize($rows, $content_type, array('views_style_options' => $this->options));
   

   Ah nice, so in this way it at least doesn't conflict with anything else. Thank you!

2. +++ b/core/modules/rest/tests/src/Unit/RestSerializerTest.php
   @@ -0,0 +1,81 @@
   +/**
   + * Tests the REST export view plugin.
   + *
   + * @group rest
   + */
   +class RestSerializerTest extends UnitTestCase {
   ...
   +  /**
   +   * Tests that the symfony serializer receives options from the render() method.
   +   */
   +  public function testSerializerReceivesOptions() {
   

   We could add @coversDefaultClass | @covers

ok, fixed the doc.

1. +++ b/core/modules/rest/tests/src/Unit/RestSerializerTest.php
   @@ -0,0 +1,82 @@
   +class RestSerializerTest extends UnitTestCase {
   

   Can we name the class "ViewsRestSerializerTest" or "RestViewsSerializerTest" to indicate that this is about the Views integration of REST module?

2. +++ b/core/modules/rest/tests/src/Unit/RestSerializerTest.php
   @@ -0,0 +1,82 @@
   +    $this->view = $this->getMock('\Drupal\views\Entity\View', array('initHandlers'), array(
   

   don't use the class name as literal string, use ::class instead. Also elsewhere.

3. +++ b/core/modules/rest/tests/src/Unit/RestSerializerTest.php
   @@ -0,0 +1,82 @@
   +      ->will($this->returnValue(''));
   

   can use willReturn()

Can we name the class "ViewsRestSerializerTest" or "RestViewsSerializerTest" to indicate that this is about the Views integration of REST module?

It should be moved IMHO to the right

+++ b/core/modules/rest/tests/src/Unit/RestSerializerTest.php
@@ -0,0 +1,82 @@
+class RestSerializerTest extends UnitTestCase {

let's move the test to src/Unit/Plugin/views/style/SerializerTest.php to be more parallel to the actual source code.

Since we are testing a rest module class the test should live with rest module

renamed class to \Drupal\Tests\rest\Unit\RestViewsStyleSerializerTest

fixed other concerns.

Well, I disagree :)
In general this test also used the wrong class ...

1. --- a/core/modules/rest/tests/src/Unit/RestViewsStyleSerializerTest.php
   +++ b/core/modules/rest/tests/src/Unit/Plugin/views/style/SerializerTest.php
   

   Do we have a coding standard for this that we have to nest unit tests that deep in plugin folders? Since the test is not a plugin we can avoid those annoying subdirectories.

2. +++ b/core/modules/rest/tests/src/Unit/Plugin/views/style/SerializerTest.php
   @@ -10,19 +10,19 @@
   -class RestViewsStyleSerializerTest extends UnitTestCase {
   +class SerializerTest extends UnitTestCase {
   

   the name "SerializerTest" in REST module is too generic I would say and makes it harder to find. I know you tried to compensate that with the nested directories, but for such a few number of unit tests that we have in REST module a longer class name makes more sense IMO. Less annoying than 3 additional levels of directories.

3. +++ b/core/modules/rest/tests/src/Unit/Plugin/views/style/SerializerTest.php
   @@ -65,18 +65,13 @@ protected function setUp() {
   +    $mock_serializer->serialize([], 'json', ['views_style_options' => $options])->willReturn('')->shouldBeCalled();
   

   can we split this up over multiple lines as we did before? Makes it easier to read.

Anyway, just non-blocking nitpicks.

Do we have a coding standard for this that we have to nest unit tests that deep in plugin folders? Since the test is not a plugin we can avoid those annoying subdirectories.

Well, IMHO this is common pratice we apply for 95% of all our unit tests.

the name "SerializerTest" in REST module is too generic I would say and makes it harder to find. I know you tried to compensate that with the nested directories, but for such a few number of unit tests that we have in REST module a longer class name makes more sense IMO. Less annoying than 3 additional levels of directories.

This is why we have namespaces ... This naming scheme also helps to quickly switch between test and code class.

Firstly:

This arguable contradicts the intention of the EncoderInterface used by the Serialization API, as the serialize() method already allows for an option $context parameter to be passed--it simply isn't being used at present.

Not really. This is a totally optional parameter. I don't think anything is contradicted here. Just as you say, un used.

Do you have a more specific use case here? As having a normalizer or encoder that depends on view configuration could easily break when you are not using it from views.

If we are doing this, which I don't have a problem with, I think we should pass more context, not style options only. Style configuration is arguable one of the least helpful IMO :) If we can fix your case, and others that may potentially arise, that would be good.

As a contrib project blocker, this would be a good issue to consider for RC target triage.

Hm that test result is on QA which is now essentially switched off... canceling that run and reuploading the patch.

I think #29 through #32 still need to be addressed in full.

You'd need the following ...

The scope of this class is to provide a new views style that displays serialized data.

Yes. I wrote the class originally.. :)

Have you seen views? Displays, styles, rows are all interlinked.

Why not just pass the actual style plugin itself?

Yeah I kinda agree that it would be better to pass on the style plugin.

Thank you @grasmash!

Code makes sense.

PHPUnit error:

There was 1 error:

1) Drupal\Tests\rest\Unit\Plugin\views\style\SerializerTest::testSerializerReceivesOptions
Prophecy\Exception\Call\UnexpectedCallException: Method call:
  - serialize([], "json", ["views_style_plugin" => Drupal\rest\Plugin\views\style\Serializer:000000006965302f000000017eeec142 Object (
    'usesRowPlugin' => true
    'usesGrouping' => false
    'serializer' => Double\SerializerInterface\P1:000000006965301d000000017eeec142 Object (
        'objectProphecy' => Prophecy\Prophecy\ObjectProphecy Object (*Prophecy*)
    )
    'formats' => Array (
        0 => 'json'
        1 => 'xml'
    )
    'usesOptions' => true
    'rowTokens' => Array ()
    'usesRowClass' => false
    'usesFields' => false
    'rendered_fields' => null
    'groupingTheme' => 'views_view_grouping'
    'defaultFieldLabels' => false
    'options' => Array (
        'formats' => Array (
            0 => 'xml'
            1 => 'json'
        )
    )
    'view' => Mock_ViewExecutable_d02d052b:000000006965306d000000017eeec142 Object (
        '__phpunit_invocationMocker' => null
        '__phpunit_originalObject' => null
        'storage' => null
        'built' => false
        'executed' => false
        'args' => Array ()
        'build_info' => Array ()
        'ajaxEnabled' => false
        'result' => Array ()
        'current_page' => null
        'items_per_page' => null
        'offset' => null
        'total_rows' => null
        'attachment_before' => Array ()
        'attachment_after' => Array ()
        'feedIcons' => Array ()
        'exposed_data' => Array ()
        'exposed_input' => Array ()
        'exposed_raw_input' => Array ()
        'old_view' => Array ()
        'parent_views' => Array ()
        'is_attachment' => null
        'current_display' => null
        'query' => null
        'pager' => null
        'display_handler' => null
        'displayHandlers' => null
        'style_plugin' => null
        'rowPlugin' => null
        'override_url' => null
        'override_path' => null
        'base_database' => null
        'field' => null
        'argument' => null
        'sort' => null
        'filter' => null
        'relationship' => null
        'header' => null
        'footer' => null
        'empty' => null
        'response' => null
        'request' => null
        'inited' => null
        'exposed_widgets' => null
        'preview' => null
        'get_total_rows' => null
        'build_sort' => null
        'many_to_one_tables' => null
        'dom_id' => null
        'element' => Array (
            '#attached' => Array (
                'library' => Array (
                    0 => 'views/views.module'
                )
                'drupalSettings' => Array ()
            )
            '#cache' => Array ()
        )
        'user' => null
        'showAdminLinks' => null
        'viewsData' => null
        'routeProvider' => null
        '_serviceIds' => Array ()
    )
    'displayHandler' => Mock_RestExport_2d18c76e:000000006965306e000000017eeec142 Object (
        '__phpunit_invocationMocker' => PHPUnit_Framework_MockObject_InvocationMocker:0000000069653096000000017eeec142 Object (
            'matchers' => Array (
                0 => PHPUnit_Framework_MockObject_Matcher:00000000696530ab000000017eeec142 Object (
                    'invocationMatcher' => PHPUnit_Framework_MockObject_Matcher_AnyInvokedCount:0000000069653003000000017eeec142 Object (
                        'invocations' => Array (
                            0 => PHPUnit_Framework_MockObject_Invocation_Object:0000000069653028000000017eeec142 Object (
                                'object' => Mock_RestExport_2d18c76e Object (*RECURSION*)
                                'className' => 'Drupal\rest\Plugin\views\display\RestExport'
                                'methodName' => 'getContentType'
                                'parameters' => Array ()
                            )
                        )
                    )
                    'afterMatchBuilderId' => null
                    'afterMatchBuilderIsInvoked' => false
                    'methodNameMatcher' => PHPUnit_Framework_MockObject_Matcher_MethodName:0000000069653068000000017eeec142 Object (
                        'constraint' => PHPUnit_Framework_Constraint_IsEqual:0000000069653090000000017eeec142 Object (
                            'value' => 'getContentType'
                            'delta' => 0
                            'maxDepth' => 10
                            'canonicalize' => false
                            'ignoreCase' => true
                            'lastFailure' => null
                            'exporter' => SebastianBergmann\Exporter\Exporter:0000000069653091000000017eeec142 Object ()
                        )
                    )
                    'parametersMatcher' => null
                    'stub' => PHPUnit_Framework_MockObject_Stub_Return:0000000069653092000000017eeec142 Object (
                        'value' => 'json'
                    )
                )
            )
            'builderMap' => Array ()
        )
        '__phpunit_originalObject' => null
        'usesAJAX' => false
        'usesPager' => false
        'usesMore' => false
        'usesAreas' => false
        'usesOptions' => false
        'contentType' => 'json'
        'mimeType' => null
        'renderer' => null
        'routeProvider' => null
        'state' => null
        'view' => null
        'handlers' => Array ()
        'plugins' => Array ()
        'extenders' => Array ()
        'output' => null
        'usesAttachments' => false
        'display' => null
        'options' => Array ()
        'displayHandler' => null
        'definition' => null
        'pluginId' => null
        'pluginDefinition' => null
        'configuration' => null
        'stringTranslation' => null
        '_serviceIds' => Array ()
        'dependencies' => Array ()
        'urlGenerator' => null
    )
    'definition' => Array ()
    'renderer' => null
    'pluginId' => 'dummy_serializer'
    'pluginDefinition' => Array ()
    'configuration' => Array ()
    'stringTranslation' => null
    '_serviceIds' => Array ()
)])
on Double\SerializerInterface\P1 was not expected, expected calls were:
  - serialize(exact([]), exact("json"), exact(["views_style_plugin" => Double\SerializerInterface\P1:000000006965301d000000017eeec142 Object (
    'objectProphecy' => Prophecy\Prophecy\ObjectProphecy Object (*Prophecy*)
)]))

/Users/wim.leers/Work/drupal-tres/core/modules/rest/src/Plugin/views/style/Serializer.php:138
/Users/wim.leers/Work/drupal-tres/core/modules/rest/tests/src/Unit/Plugin/views/style/SerializerTest.php:78
/Users/wim.leers/Work/drupal-tres/vendor/phpunit/phpunit/phpunit:47

Attached reroll is only fixing whitespace nitpicks, not the failures.

Let's just fix it.

I did not know about Argument::that(). Thanks!

Looks good.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Thanks to this, csv_serialization is now unblocked, yay :)