REST views: add special handling for collections

1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityCollectionNormalizer.php
   @@ -0,0 +1,53 @@
   +class EntityCollectionNormalizer extends NormalizerBase {
   

   Could you extend the example HAL representation in the issue summary with the title, description and other properties, so that we get a picture what the full result looks like?

2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityCollectionNormalizer.php
   @@ -0,0 +1,53 @@
   +  public function denormalize($data, $class, $format = NULL, array $context = array()) {
   

   So this method is empty, why? Are we not going to support denormalization? Please add a comment.

3. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.php
   @@ -115,17 +116,7 @@ public function submitOptionsForm(&$form, &$form_state) {
   +    return $this->serializer->serialize($this->getEntityCollection(), $this->displayHandler->getContentType());
   

   So as you said this works well for full entities, could we also use the EntityCollection class to pass in primitive rows built by views?

4. +++ b/core/modules/serialization/lib/Drupal/serialization/EntityCollection.php
   @@ -0,0 +1,145 @@
   +class EntityCollection implements \IteratorAggregate {
   

   So this looks like a data junk class. Basically we are just using this class so that the serializer can detect it and knows what to do, right? Actually we are just dealing with some metadata like title, URI and description of the collection + an array of entities, that's it. Could we just use an getCollectionMetadata() method that returns an array with all those properties? Is there a compelling reason why we want that all as separate properties? I'm not sure about this, the empty data junk class just looks a bit weird to me. And what properties do we define, which ones are required?

Overall I think we are on the right track here. Haven't looked to closely at the link manager changes, so someone else should review that.

This affects Views primarily so perhaps this tag is appropriate.

Rerolled linclark's patch.

Added & adjusted integration test cases in core/modules/rest/lib/Drupal/rest/Tests/Views/StyleSerializerTest.php
Added & adjusted unit tests in core/modules/hal/tests/EntityCollectionNormalizerTest.php

There are a few todos and prickly points left, and in general we should consider renaming EntityCollection to RowCollection (as we could leverage it for paging all kinds of views), we need to move the collection factory (getEntityCollectionFromView) from \Drupal\rest\Plugin\views\style\Serializer to somewhere more reasonable ...

Missed re-adding the CollectionLinkManager to the LinkManager instantiation in NormalizerTestBase (changed signature of RelationLinkManager). Interdiff and new patch attach.

http://tools.ietf.org/html/draft-kelly-json-hal-05#section-6 HAL spec info

- Improved test coverage (more comprehensive integration tests for views.view.test_serializer_display_entity.yml and views.view.test_serializer_display_field.yml including paging; added unit test for \Drupal\serialization\Collection).
- Refactored/Renamed \Drupal\serialization\EntityCollection to \Drupal\serialization\Collection
- Use injected UrlGenerator service instead of url() style/Serializer.php-Plugin
- drupalcs love.

Hefty interdiff due to lot's of renaming.

Impressive work, indeed.
Is there any way to spin of part of the big patch?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.php
@@ -143,4 +142,87 @@ public function getFormats() {
+
+    // Determine whether we have more items than we are showing, in that case
+    // we are a pageable collection.
+    if ($pager->getTotalItems() > $pager->getItemsPerPage()) {

I wonder whether we should implement a last page but rather not use a count query.

Rerolled patch as i botched the rebase in #14 :(

@dawehner: happy to split, slice and dice as you see fit :) I'll ping you on IRC.

1. +++ b/core/modules/hal/tests/CollectionNormalizerTest.php
   @@ -0,0 +1,210 @@
   +    $this->assertFalse($normalizer->supportsNormalization($collection, 'json'));
   

   Why don't we support JSON? I think it should just look the same as HAL.

2. +++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.php
   @@ -143,4 +142,87 @@ public function getFormats() {
   +    // @todo Determine how to handle field-based views.
   

   I think this @todo can be removed? It should work regardless of what the items are?

hej klausi,

re: 1. that's been like that since lin's patch in #8 (https://drupal.org/files/2100637-08-collections.patch); in principle we can of course serialize to JSON, but there would be no defined way to add link relations (prev, next, etc.) I dumping the collection to JSON is a piece of pie.

re: 2. i think that's true - i would remove and revisit all todos in one go if that's ok?

best, f.

Added a manual test section

Did a reroll. Let's see what fails.

[edit]Pager is working fine[/edit]

[edit]deleted as pager is working fine.[/edit]

Fixed the duplicate 'use ...String' failure.

Views preview is not working while it was in the old rest_display

I get
[Tue Jul 29 13:05:16 2014] [error] [client 127.0.0.1] Uncaught PHP Exception Symfony\\Component\\Serializer\\Exception\\UnexpectedValueException: "Serialization for the format drupal_ajax is not supported" at /Users/clemens/Sites/drupal/d8/www/core/vendor/symfony/serializer/Symfony/Component/Serializer/Serializer.php line 77, referer: http://drupal.d8/admin/structure/views/view/frontpage/edit/rest_export_1

Fixed failing tests (at least locally)

Sorry testbot for misnaming the interdiff :-(

I think, based on #31?

Patch adds documentation and variables annotations. This make the code a little more navigable.

1. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +/**
   + * Converts the Drupal entity object structure to a HAL array structure.
   + */
   +class CollectionNormalizer extends NormalizerBase {
   ...
   +   * Constructs an EntityNormalizer object.
   

   The one line description is a copy and paste

2. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +   *
   +   * @todo Implement denormalization once normalization has settled.
   

   Do we have a follow up for this already?

3. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -53,6 +53,8 @@ class ContentEntityNormalizer extends NormalizerBase {
   +   * @param EntityManagerInterface $entity_manager
   +   * @param ModuleHandlerInterface $module_handler
   

   Let's use absolute documentation.

4. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -61,11 +63,16 @@ public function __construct(LinkManagerInterface $link_manager, EntityManagerInt
   +   *
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   +   * @param null $format
   +   * @param array $context
   +   *
   +   * @return array|scalar
   
   +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   @@ -52,10 +52,14 @@ public function __construct(LinkManagerInterface $link_manager, EntityResolverIn
   +   * @param \Drupal\Core\Field\FieldItemInterface $field_item
   +   * @param null $format
   +   * @param array $context
   +   * @return array|scalar
   

   If we document this here, which sounds like a good idea, let's not try to shortcut it.

5. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -94,6 +101,7 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +      // @todo: Method 'normalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   
   @@ -194,6 +204,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +      // @todo: Method 'denormalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   

   ???

6. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -35,6 +38,7 @@ public function normalize($field_item, $format = NULL, array $context = array())
   +    /** @var \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface $field */
   

   Can't you just typehint type data itself? this is where getParent lives.

7. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -42,22 +46,24 @@ public function normalize($field_item, $format = NULL, array $context = array())
   +    /** @var FieldItemInterface $field_item */
   ...
   +      /** @var FieldDefinitionInterface $field_definition */
   

   absolute.

8. +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -24,14 +24,14 @@
        if (in_array($format, $this->formats) && (class_exists($this->supportedInterfaceOrClass) || interface_exists($this->supportedInterfaceOrClass))) {
   diff --git a/core/modules/hal/src/Tests/FileDenormalizeTest.php b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   
   diff --git a/core/modules/hal/src/Tests/FileDenormalizeTest.php b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   index 853b06d..e950506 100644
   
   index 853b06d..e950506 100644
   --- a/core/modules/hal/src/Tests/FileDenormalizeTest.php
   
   --- a/core/modules/hal/src/Tests/FileDenormalizeTest.php
   +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   
   +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   @@ -35,13 +35,15 @@ public function testFileDenormalize() {
   
   @@ -35,13 +35,15 @@ public function testFileDenormalize() {
          'filemime' => 'text/plain',
          'status' => FILE_STATUS_PERMANENT,
        );
   -    // Create a new file entity.
   +
   +    /** @var \Drupal\file\Entity\File $file */
        $file = entity_create('file', $file_params);
        file_put_contents($file->getFileUri(), 'hello world');
        $file->save();
    
        $serializer = \Drupal::service('serializer');
        $normalized_data = $serializer->normalize($file, 'hal_json');
   +    /** @var \Drupal\file\Entity\File $denormalized */
        $denormalized = $serializer->denormalize($normalized_data, 'Drupal\file\Entity\File', 'hal_json');
    
        $this->assertTrue($denormalized instanceof File, 'A File instance was created.');
   

   It is just crazy how much this patch is blown up by unrelated changes

9. +++ b/core/modules/hal/src/Tests/NormalizeTest.php
   @@ -27,8 +29,10 @@ function setUp() {
   +    /** @var ContentEntityBase $target_entity_de */
        $target_entity_de = entity_create('entity_test', (array('langcode' => 'de', 'field_test_entity_reference' => NULL)));
        $target_entity_de->save();
   +    /** @var ContentEntityBase $target_entity_en */
        $target_entity_en = entity_create('entity_test', (array('langcode' => 'en', 'field_test_entity_reference' => NULL)));
        $target_entity_en->save();
    
   @@ -53,6 +57,7 @@ public function testNormalize() {
   
   @@ -53,6 +57,7 @@ public function testNormalize() {
          )
        );
    
   +    /** @var ContentEntityBase $entity */
        $entity = entity_create('entity_test', $values);
        $entity->save();
        // Add an English value for name and entity reference properties.
   @@ -168,13 +173,13 @@ public function testNormalize() {
   
   @@ -168,13 +173,13 @@ public function testNormalize() {
      /**
       * Constructs the entity URI.
       *
   -   * @param $entity
   +   * @param ContentEntityBase $entity
       *   The entity.
   

   absolute again.

10. +++ b/core/modules/hal/tests/CollectionNormalizerTest.php
    @@ -0,0 +1,208 @@
    + *
    + * @group HAL
    + */
    +class CollectionNormalizerTest extends UnitTestCase {
    

    Let's use @coversDefaultClass and @covers here.

11. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
    @@ -0,0 +1,25 @@
    +/**
    + * Interface for mapping collection (e.g. Views) link relations.
    + */
    +interface CollectionLinkManagerInterface {
    

    It is odd that we talk about mapping link relations but the class name nor namespace mentions it.

12. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
    @@ -0,0 +1,25 @@
    +   */
    +  public function getCollectionItemRelation($collection_id);
    +}
    

    Ensure that all files have an empty line before the last }

13. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -144,4 +143,87 @@ public function getFormats() {
    +    // @todo Determine how to handle field-based views.
    +    foreach ($this->view->result as $row) {
    +      $rows[] = $this->view->rowPlugin->render($row);
    +    }
    

    Isn't that solved already, as the row plugin would be different and so different values are returned? I guess we can drop the todo here? Afaik we should have test coverage so we are safe.

14. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -144,4 +143,87 @@ public function getFormats() {
    +    $collection->setTitle($display->getOption('title'));
    

    I would rather go with $view->getTitle() to be honest.

15. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -144,4 +143,87 @@ public function getFormats() {
    +    // Route as defined in e.g. \Drupal\rest\Plugin\views\display\RestExport.
    +    $route_name = 'view.' . $view_id . '.' . $display_id;
    +
    +    // Get base url path for the view; getUrl returns a path not an absolute
    +    // URL (and no page information).
    +    $view_base_url = $this->view->getUrl();
    +
    +    // Inject the page into the canonical URI of the view.
    +    if ($this->view->getCurrentPage() > 0) {
    ...
    +    }
    +    else {
    ...
    +    }
    +
    

    Mh, this works fine for the normal cases, but we might have a different route name if something overrides an existing route. You could also use something like $state = \Drupal::state(); $route_names = $state->get('views.view_route_names'); $route_names["$view_id.$display_id"];

16. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -28,26 +32,58 @@ class StyleSerializerTest extends PluginTestBase {
     
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Style: Serializer plugin',
    +      'description' => 'Tests the serializer style plugin.',
    +      'group' => 'Views Plugins',
    +    );
    +  }
    +
    +  /**
    

    we don't need getInfo any longer.

17. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -91,41 +157,146 @@ public function testSerializerResponses() {
    +    $view->setDisplay('rest_export_1');
    +    $view->initDisplay();
    

    there is no reason for the initDisplay() call. setDisplay() already does that.

18. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -91,41 +157,146 @@ public function testSerializerResponses() {
    +    // Get the serializer service.
    +    $serializer = $this->container->get('serializer');
    

    i don't really see a value in this line of documentation, do you?

19. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,214 @@
    + * Provides a wrapper for a collection of entities, e.g. a feed channel.
    ...
    +   * The entities in the collection.
    ...
    +   * var array
    

    Are these any kind of things or just entities?

#4

If we document this here, which sounds like a good idea, let's not try to shortcut it.

Do you mean add a sentence?

I will work through the list from #36 tomorrow.

#2 No follow up found. Not sure what to create yet.
#5 See #2
#6 PHP Storm seems to need this
#8 I wasn't aware of https://www.drupal.org/coding-standards

Note: Do not squeeze coding standards updates/clean-ups into otherwise unrelated patches.

so will rethink next issue.
#10 not sure what to add as a whole. Added only @coversDefaultClass ... https://www.drupal.org/node/2325985
#11 skipped for now
#13 @todo removed
#16 removed the public static function getInfo() { but should we save to info data?
#19 skipped for now

Added to summary:

#2 No follow up found. Not sure what to create yet.
#11 skipped for now
#19 skipped for now

Argh

Output: [PHP Fatal error: Uncaught exception 'LogicException' with message 'Missing @group for Drupal\hal\Tests\NormalizeTest.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/TestDiscovery.php:362

+++ b/core/modules/hal/src/Tests/NormalizeTest.php
@@ -12,7 +12,7 @@
- * @group hal

Is that required?!? See http://phpunit.de/manual/current/en/appendixes.annotations.html#appendix...

Added to #2325985: No documentation about @group @coversDefaultClass @covers

Added @group back to test

Just a reroll.

Somehow the rest.services.yml missed

+  rest.link_manager.collection:
+    class: Drupal\rest\LinkManager\CollectionLinkManager

That makes the views Rest export display working. Tests still failing. It cannot load the test views somehow.

Fixed missing pagers thanks to @dawehner

This still needs some work.

Added outstanding issue from #36 to summary.

Just some short feedback. I think this patch should get some review from damiankloip, as he is aware of both serialization and views.

1. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +   * @todo Implement denormalization once normalization has settled.
   

   So let's do it here, I guess? This then also needs a test coverage.

2. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +  }
   +}
   

   Nitpick: Add a newline between.

3. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -93,6 +108,7 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +      // @todo: Method 'normalize' not found in class \Symfony\Component\Serializer\SerializerInterface
          $normalized_property = $this->serializer->normalize($field, $format, $context);
   

   Ah I see, so normalize() exists on the serializer, not on the interface, maybe we want to typehint against the NormalizerInterface in that case.

@dawehner

#1 : This needs some hints :-/

Fix #2 and #3

reroll

Needed a reroll for fgm

Generally, this is looking good. An epic bit of work!

1. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +    if (is_array($links) && count($links)) {
   

   I don't think we should check if it's an array here, that silent failure stuff...

2. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +   * @todo Implement denormalization once normalization has settled.
   

   Fair enough, let's create a follow up and link it here.

3. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function supportsDenormalization($data, $type, $format = NULL) {
   +    return FALSE;
   +  }
   

   Let's add a todo here too to remove this method based on above.

4. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -115,6 +130,8 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +   * @return \Drupal\Core\Entity\EntityInterface|object
   

   When do we return a regular object?

5. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -187,6 +204,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +      // @todo: Method 'denormalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   

   The serializer implements the DenormalizerInterface which has this method on.

6. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   @@ -52,10 +52,14 @@ public function __construct(LinkManagerInterface $link_manager, EntityResolverIn
   +   * {@inheritdoc}
   +   *
   +   * @param \Drupal\Core\Field\FieldItemInterface $field_item
   

   Not inheritdoc and other docs. Just the first :)

7. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   @@ -120,6 +124,7 @@ public function getUuid($data) {
   +    return NULL;
   

   That's implicitly done anyway, so this doesn't make any function change. Not really needed, but meh. Don't care either way on that.

8. +++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
   @@ -101,6 +105,7 @@ protected function normalizeFieldItems($field, $format, $context) {
   +        // @todo Method 'normalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   

   Same as previous comment. NormalizerInterface has this.

9. +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
   @@ -116,7 +117,11 @@ protected function setUp() {
   +      new TypeLinkManager(new MemoryBackend('cache')),
   +      new RelationLinkManager(new MemoryBackend('cache'), $entity_manager),
   

   Won't make any difference to the tests, but we don't have a cache.cache bin anymore, Just cache.default.

10. +++ b/core/modules/hal/tests/CollectionNormalizerTest.php
    @@ -0,0 +1,208 @@
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'CollectionNormalizer test',
    +      'description' => "Unit test of the CollectionNormalizer's normalize support.",
    +      'group' => 'HAL',
    +    );
    +  }
    

    Don;t need getInfo for unit tests anymore. The group and @coversDefaultClass/@covers is good.

11. +++ b/core/modules/hal/tests/CollectionNormalizerTest.php
    @@ -0,0 +1,208 @@
    +    // Get a dummy node.
    ...
    +    // Get a dummy node.
    

    stub/mock

12. +++ b/core/modules/hal/tests/CollectionNormalizerTest.php
    @@ -0,0 +1,208 @@
    +   * @return \Symfony\Component\Serializer\SerializerInterface
    

    That is not true, Serializer implements many interfaces. Can also pipe| the phpunit mock object class to that.

13. +++ b/core/modules/rest/rest.services.yml
    @@ -15,13 +15,15 @@ services:
    +  rest.link_manager.collection:
    +    class: Drupal\rest\LinkManager\CollectionLinkManager
    
    @@ -30,3 +32,5 @@ services:
    +  rest.link_manager.collection:
    +    class: Drupal\rest\LinkManager\CollectionLinkManager
    

    This is added twice

14. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -53,6 +55,11 @@ class Serializer extends StylePluginBase {
    +   * @var \Drupal\Core\Routing\UrlGeneratorInterface
    

    Needs a one line description like 'The URL generator' pointless, yes pretty much. That's just the way it is.

15. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -145,4 +144,88 @@ public function getFormats() {
    +    $state = \Drupal::state();
    

    This should be injected too.

16. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -145,4 +144,88 @@ public function getFormats() {
    +    $collection->setItems($rows);
    

    Move the processing of the rows to near here. Easier to read and keep track of then.

17. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -58,6 +83,37 @@ protected function setUp() {
    +   *   Array of options to pass to url().
    

    The url generator

18. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -58,6 +83,37 @@ protected function setUp() {
    +    return $view->getStyle()->getCollection($view);
    

    Maybe at least initStyle() should be called here?

19. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -91,41 +146,141 @@ public function testSerializerResponses() {
    +    $this->assertTrue(isset($actual_json['_embedded']) && isset($actual_json['_links']), 'Has _links and _embedded keys');
    ...
    +    $this->assertTrue(isset($actual_json['_embedded']) && isset($actual_json['_links']), 'Has _links and _embedded keys');
    ...
    +    $this->assertTrue(isset($actual_json_page_1['_embedded']) && isset($actual_json_page_1['_links']), 'Has _links and _embedded keys');
    

    Two separate assertions please.

20. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -294,6 +585,8 @@ public function testFieldapiField() {
    +    // @todo: add HAL+JSON and paging.
    

    Is this getting done? :)

21. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,214 @@
    +   * The entities in the collection.
    

    This does not have to apply only to entities. E.g. what happens if you use the field data row plugin? I guess it's fine just docs need updating?

22. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,214 @@
    +  public function setLinks($links) {
    

    Should typehint an array

23. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,214 @@
    +    return is_array($this->links) && count($this->links);
    

    Should always be an array, so just return (bool) count()

24. +++ b/core/modules/serialization/tests/Drupal/serialization/Tests/CollectionTest.php
    @@ -0,0 +1,113 @@
    +    $this->assertEquals($collection->getTitle(), NULL, 'Collection title is not set');
    ...
    +    $this->assertEquals($collection->getTitle(), $collection_id, 'Collection title has been set');
    ...
    +    $this->assertEquals($collection->getUri(), NULL, 'Collection URI is not set');
    ...
    +    $this->assertEquals($collection->getUri(), 'http://example.com/' . $collection_id, 'Collection URI has been set');
    ...
    +    $this->assertEquals($collection->getDescription(), NULL, 'Collection description is not set');
    ...
    +    $this->assertEquals($collection->getDescription(), $collection_id, 'Collection description has been set');
    

    expected value first.

Also, another problem is that this will totally break RestExport/Serializer if hal is not enabled. As serialization module does not have a normalizer for collections.

There is also quite a lot of processing we don't want to do unless we are using the hal format. e.g. regular json does not care about links etc... I just want the raw data.

I rerolled the patch this morning too: very little difference, mostly:

- removed a duplicate service definition
- fixed the JSON namespace import
- 2 instances of == NULL replaced by === NULL for accuracy
- removed various unused imports and out of date /** var ...*/

Forgot to set CNR to trigger bot.

Rerolled:

- $this->getName is now $this->getMachineName() in tests
- url() is now Url::fromUri().

Note: the massive fails are due to a problem with the testbots (too many SQL connections), not with the test itself, so retrying until the bots work again.

@fgm do you have an interdiff and have you time for the feedback of @damiankloip in #57 or should I take those?

Added #2350401: [PP-1] Collection denormalization thus removed one TODO from summary.

@clemens.tolboom : here is the diff between the two versions, in patch format. Yes, I can continue with Damiankoip's remarks today.

@fgm awesome. Your interdiff contained unrelated js stuff .. np. The interdiff could have the .txt extension to skip testing. Thanks for working on this :0

#58 is the one the biggest problems here at the moment, I think.

@damiankloip

Also, another problem is that this will totally break RestExport/Serializer if hal is not enabled. As serialization module does not have a normalizer for collections.

Is this patch causing this? Or is the current 'Rest export display' breaks when disabling hal?

There is also quite a lot of processing we don't want to do unless we are using the hal format. e.g. regular json does not care about links etc... I just want the raw data.

Guess that's a serialization problem right. What about #2343431: Make JSON data non-Drupalish and #2339795: Add "REST modes", just like we have "view modes" and "form modes" AND depth argument to reduce # of requests

Regarding 58, I think Hal should be just a serialization format, and "plain JSON", whatever it is, be another, just like XML, or site-specific serializations like application/myawesomesite+json

Hopefully everything passing this time.

Since the bot doesn't seem to have sent its notification yet: the test suite passes.

Started work on issues in #57, time for a first check:

1. I think we need to check if it is an array (would we accept an iterator ?), to avoid an error in the subsequent foreach.

1bis. Since $object is not type-hinted, we need to behave propertly if the method is not called with a Collection object.

2: Followup is #2350401: [PP-1] Collection denormalization.

3. Done.

4. Actually, we only return an array now. Phpdoc changed.

5. Actually, the code only implies that ContentEntityNormalizer, not
ContentEntityNormalizer->serializer implements DenormalizerInterface: the
serializer property is declared in SerializerAwareNormalizer as implementing
only SerializerInterface, which does not include (de)normalize methods. So
that remains a @todo

6. Done.

7. Probably more readable that way, let's keep it.

8. Same remaining problem as 5.

9. Bin changed to "default", but the MemoryBackend ignores the bin anyway.

10. Done.

11. Done.

12. Done. Loooon type info: does it really make sense ?

13. Done.

14. Done.

Also removed a duplicate copy of CollectionNormalizerTest (one in src/Tests/, one in tests/ ).

Addressed the rest of the #57 points:

15. Done

16. Done

17. Done

18. Probably not ? $view->getStyle() includes a call to initStyle()

19. Done

20. Not yet

21. Done

22. Done

23. Done

24. Done

So the failing test is a consequence of point 22 : typehinting an array on Collection::setLinks().

There are two ways to fix this:
- removing the type hint
- removing the failing test : this is part of a unit test in which there is an explicit call to setLinks(NULL), which does not seem to make sense as all other use cases correctly pass a (possibly empty) array

So rerolling with the setLinks(NULL) test removed and the type hint kept : this makes the API more consistent.

Also fixed a remining getName() -> getMachineName() call and removed another duplicated test.

First observation - there are a lot of out of scope coding-standards changes in this patch that make reviews difficult - please split them into a separate patch/issue to reduce the size of this patch. Given they're all minor those changes could be in by now if they were in a separate issue and reviews would be more forthcoming here.

There are two issues I see here, one is there is no interface for Collection object, the other is the line that does $this->view = $this->view, which seems like an error. The rest is CS nitpicks in the new code or CS changes in existing code that are out of scope.

Other than that, looks great.

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -52,6 +52,10 @@ class ContentEntityNormalizer extends NormalizerBase {
   +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   +   *   The entity manager.
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   +   *   The module handler.
   
   @@ -60,11 +64,22 @@ public function __construct(LinkManagerInterface $link_manager, EntityManagerInt
   -   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
   +   * {@inheritdoc}
   +   *
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   +   *   We only process Content Entitities here.
   +   * @param string $format
   +   *   Format the normalization result will be encoded as.
   +   * @param array $context
   +   *   Context options for the normalizer.
   +   *
   +   * @return array
   +   *
   +   * @see \Drupal\hal\HalSubscriber
   +   *   Defines the format to support.
   ...
   -    /** @var $entity \Drupal\Core\Entity\ContentEntityInterface */
   
   @@ -101,7 +116,7 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   -   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize().
   +   * {@inheritdoc}
   
   @@ -115,6 +130,8 @@ public function normalize($entity, $format = NULL, array $context = array()) {
   +   * @return \Drupal\Core\Entity\EntityInterface|object
   +   *
   
   @@ -187,6 +204,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +      // @todo: Method 'denormalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   
   @@ -245,4 +263,5 @@ protected function getTypedDataIds($types) {
   +
   
   +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   @@ -52,10 +52,9 @@ public function __construct(LinkManagerInterface $link_manager, EntityResolverIn
   -   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
   +   * {@inheritdoc}
   ...
   -    /** @var $field_item \Drupal\Core\Field\FieldItemInterface */
   
   @@ -86,6 +85,7 @@ public function normalize($field_item, $format = NULL, array $context = array())
   +
   
   @@ -97,7 +97,7 @@ public function normalize($field_item, $format = NULL, array $context = array())
   -   * Overrides \Drupal\hal\Normalizer\FieldItemNormalizer::constructValue().
   +   * {@inheritdoc}
   
   @@ -106,6 +106,7 @@ protected function constructValue($data, $context) {
   +
   
   @@ -118,8 +119,11 @@ public function getUuid($data) {
   +
   ...
   +
   +    return NULL;
   
   +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Field\FieldDefinitionInterface;
   
   @@ -23,9 +24,11 @@ class FieldItemNormalizer extends NormalizerBase {
   -   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
   +   * {@inheritdoc}
   ...
   +    /** @var  $field_item \Drupal\Core\Field\FieldItemInterface */
   +
   
   @@ -35,6 +38,7 @@ public function normalize($field_item, $format = NULL, array $context = array())
   +    /** @var \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface $field */
   
   @@ -42,22 +46,25 @@ public function normalize($field_item, $format = NULL, array $context = array())
   -   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize()
   +   * {@inheritdoc}
   ...
   +    /** @var \Drupal\Core\Field\FieldItemInterface $field_item */
   ...
   +    if ($field_item->getParent() === NULL) {
   +      throw new InvalidArgumentException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
   +    }
   ...
   +
   +      /** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */
   
   +++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
   @@ -23,7 +23,9 @@ class FieldNormalizer extends NormalizerBase {
   -   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
   +   * {@inheritdoc}
   +   *
   +   * @var \Drupal\Core\Field\FieldItemListInterface $field
   
   @@ -58,17 +60,19 @@ public function normalize($field, $format = NULL, array $context = array()) {
   -   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize()
   +   * {@inheritdoc}
   ...
   -    if ($context['target_instance']->getParent() == NULL) {
   +
   +    /** @var \Drupal\Core\Field\FieldItemListInterface $field */
   +    $field = $context['target_instance'];
   +    if ($field->getParent() === NULL) {
   ...
   -    $field = $context['target_instance'];
   
   @@ -77,6 +81,7 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +      // @todo Method 'denormalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   
   @@ -101,6 +106,7 @@ protected function normalizeFieldItems($field, $format, $context) {
   +        // @todo Method 'normalize' not found in class \Symfony\Component\Serializer\SerializerInterface
   
   +++ b/core/modules/hal/src/Normalizer/NormalizerBase.php
   @@ -24,14 +24,14 @@
   -   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::supportsNormalization().
   +   * {@inheritdoc}
   ...
   -   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization()
   +   * {@inheritdoc}
   
   +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
   @@ -35,13 +35,15 @@ public function testFileDenormalize() {
   -    // Create a new file entity.
   +
   +    /** @var \Drupal\file\Entity\File $file */
   ...
   +    /** @var \Drupal\file\Entity\File $denormalized */
   
   +++ b/core/modules/hal/src/Tests/FileNormalizeTest.php
   @@ -64,7 +65,8 @@ public function testNormalize() {
   -    // Create a new file entity.
   +
   +    /** @var \Drupal\file\Entity\File $file */
   
   +++ b/core/modules/hal/src/Tests/NormalizeTest.php
   @@ -27,8 +30,10 @@ protected function setUp() {
   +    /** @var \Drupal\Core\Entity\ContentEntityBase $target_entity_de */
   ...
   +    /** @var \Drupal\Core\Entity\ContentEntityBase $target_entity_en */
   
   @@ -52,6 +57,7 @@ public function testNormalize() {
   +    /** @var \Drupal\Core\Entity\ContentEntityBase $entity */
   
   @@ -167,13 +173,13 @@ public function testNormalize() {
   -   * @param $entity
   +   * @param \Drupal\Core\Entity\ContentEntityBase $entity
   ...
   -  protected function getEntityUri($entity) {
   +  protected function getEntityUri(ContentEntityBase $entity) {
   
   +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
   @@ -14,19 +14,20 @@
   -use Drupal\simpletest\DrupalUnitTestBase;
   +use Drupal\simpletest\KernelTestBase;
   ...
   -abstract class NormalizerTestBase extends DrupalUnitTestBase {
   +abstract class NormalizerTestBase extends KernelTestBase {
   
   +++ b/core/modules/rest/src/LinkManager/LinkManagerInterface.php
   @@ -16,8 +16,8 @@
   - * custom logic, it is expected to be more common for plugin managers to proxy
   + * custom logic, it is expected to be more common for link managers to proxy
   
   +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -246,7 +246,7 @@ public function optionsSummary(&$categories, &$options) {
   -    parent::collectRoutes($collection);
   +    $result = parent::collectRoutes($collection);
   
   @@ -261,6 +261,8 @@ public function collectRoutes(RouteCollection $collection) {
   +
   +    return $result;
   

   out of scope

2. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
   @@ -0,0 +1,199 @@
   +}
   
   +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
   @@ -0,0 +1,25 @@
   +}
   
   +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -150,4 +156,86 @@ public function getFormats() {
   +  }
   

   missing a blank line before the last }

3. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -150,4 +156,86 @@ public function getFormats() {
   +    $this->view = $this->view;
   

   this looks wrong

4. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -29,26 +33,47 @@ class StyleSerializerTest extends PluginTestBase {
   -  public static $modules = array('views_ui', 'entity_test', 'hal', 'rest_test_views', 'node', 'text', 'field');
   +  public static $modules = array(
   +    'views_ui',
   +    'entity_test',
   +    'hal',
   +    'rest_test_views',
   +    'node',
   +    'text',
   +    'field',
   +  );
   ...
   -  public static $testViews = array('test_serializer_display_field', 'test_serializer_display_entity', 'test_serializer_node_display_field');
   +  public static $testViews = array(
   +    'test_serializer_display_field',
   +    'test_serializer_display_entity',
   +    'test_serializer_node_display_field',
   +  );
   ...
   -   * A user with administrative privileges to look at test entity and configure views.
   +   * A user with administrative privileges to look at test entity and configure
   +   * views.
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   ...
   -    $this->adminUser = $this->drupalCreateUser(array('administer views', 'administer entity_test content', 'access user profiles', 'view test entity'));
   +    $this->adminUser = $this->drupalCreateUser(array(
   +      'administer views',
   +      'administer entity_test content',
   +      'access user profiles',
   +      'view test entity',
   +    ));
   

   out of scope

5. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -92,41 +148,162 @@ public function testSerializerResponses() {
   -  public function testReponseFormatConfiguration() {
   +  public function testResponseFormatConfiguration() {
   
   @@ -138,7 +315,7 @@ public function testReponseFormatConfiguration() {
   -     // Should return a 200.
   +    // Should return a 200.
   

   out of scope

6. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -302,6 +617,8 @@ public function testFieldapiField() {
   +    // @todo: add HAL+JSON and paging.
   

   is there an issue for this?

7. +++ b/core/modules/serialization/src/Collection.php
   @@ -0,0 +1,215 @@
   +class Collection implements \IteratorAggregate {
   

   There are a lot of public methods here so we should be adding an interface too, and type-hinting that

8. +++ b/core/modules/serialization/src/Collection.php
   @@ -0,0 +1,215 @@
   +  }
   
   +++ b/core/modules/serialization/tests/src/Unit/CollectionTest.php
   @@ -0,0 +1,110 @@
   +  }
   

   nitpick needs space before the last }

9. +++ b/core/modules/serialization/src/EntityResolver/UuidReferenceInterface.php
   @@ -20,7 +20,7 @@
   -   * @return string
   +   * @return string|null
   

   out of scope

As discussed on IRC we need to remove the out of scope changes. I'll try to do that over the weekend.

Reverted out of scope stuff (git checkout -p is the business)

re-roll

Finally reviewing this!

1. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   +
   +    // Add the list of items.
   +    $link_relation = $this->linkManager->getCollectionItemRelation($object->getCollectionId());
   +    $normalized['_embedded'][$link_relation] = $this->serializer->normalize($object->getItems(), $format, $context);
   

   The no foreach here seems suspect. If it actually works, why is this different than the way links work/are added?

2. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * @todo Implement denormalization once normalization has settled.
   +   */
   +  public function denormalize($data, $class, $format = NULL, array $context = array()) {
   +  }
   

   I'm not sure that we need denormalization of collections in the first place, in practice. Is there a use case?

3. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManager.php
   @@ -0,0 +1,23 @@
   +    // By default, use the item IANA Link Relation, which is a generic way to
   +    // link to items from a collection. See http://tools.ietf.org/html/rfc6573.
   +    return 'item';
   

   Mmm... standards. :-)

   That said, do we need a whole service to define one string? Couldn't that just be a container parameter or something if we want it swappable?

4. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
   @@ -0,0 +1,25 @@
   +  /**
   +   * Get link relating collection to item.
   +   *
   +   * @param string $collection_id
   +   *   The identifier of a collection (e.g. View name).
   +   *
   +   * @return string
   +   *   The link relation.
   +   */
   +  public function getCollectionItemRelation($collection_id);
   

   The description for this method makes no sense to me. Wha?

5. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -150,4 +156,86 @@ public function getFormats() {
   +      $collection->setLink('first', $this->urlGenerator->generateFromRoute($route_name, array(), array(
   +        'query' => array('page' => 0),
   +        'absolute' => TRUE,
   +      )));
   

   Strictly speaking, first/last have been removed from the standards since few if any implementers used them. That said, I'm fine with leaving them as I think implementers who didn't implement them are idiots. :-)

   The first link,though, should not specify a page parameter at all in order to be consistent with the way we use pagers elsewhere and to have a consistent URI.

6. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -150,4 +156,86 @@ public function getFormats() {
   +      // If we are not on the first page add a previous link.
   +      if ($current_page > 0) {
   +        $collection->setLink('prev', $this->urlGenerator->generateFromRoute($route_name, array(), array(
   +          'query' => array('page' => $current_page - 1),
   +          'absolute' => TRUE,
   +        )));
   +      }
   

   If the previous page is the first page, then we should omit the page property as above.

7. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -150,4 +156,86 @@ public function getFormats() {
   +      $collection->setLink('last', $this->urlGenerator->generateFromRoute($route_name, array(), array(
   +        'query' => array('page' => $total),
   +        'absolute' => TRUE,
   +      )));
   

   All of these setLink() lines would probably be easier to read with [] short array syntax. Especially for the empty arrays.

8. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -233,6 +389,146 @@ public function testUIFieldAlias() {
   +    $this->assertEqual(array_keys($actual_json_page_last['_links']), array(
   +      'self',
   +      'first',
   +      'prev',
   +      'last',
   +    ));
   

   Strictly speaking, this test is order-sensitive. HAL, however, is not order-sensitive on links. That the order is the same here is coincidental and based on knowledge of how the code happens to be written right now.

   It would be more stable to do a series of assertTrue(in_array()) calls, or else order the keys alphabetically and then check against a known alphabetized list.

   Actually, even the latter is wrong because it doesn't allow for the presence of additional links. The presence of additional links is entirely legal; we just want to be sure these links are present.

   So that means the most correct test would be:

   $this->assertCount(4, count(array_intersect($links, ['self', 'first', ...]));
   

   Or something like that (I didn't actually test the above line.)

9. +++ b/core/modules/serialization/src/Collection.php
   @@ -0,0 +1,215 @@
   +  protected $items;
   

   Initialize to empty array. []

10. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * The title of the collection.
    +   *
    +   * @var string
    +   */
    +  protected $title;
    

    Human-readable? Translated? If so, should it be called label (since that's what we call it on entities) or NOT be called label (since that's what we call it on entities)?

11. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * The description of the collection.
    +   *
    +   * @var string
    +   */
    +  protected $description;
    

    As above.

12. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Hypermedia links (prev, next, first, last for paging collections)
    +   *
    +   * @var array
    +   */
    +  protected $links = array();
    

    What is the structure of this array? And if each link is itself structured, should those be objects of their own (a la HtmlFragment, RIP)?

    Also, the list of link names should be explicitly listed as examples. Having other links on a collection is entirely legal and something we want to encourage.

    From the code below, it appears that we only support the link url itself here. However, that is insufficient as it is completely legal in HAL to have links that have additional metadata properties, such as hreflang, templated = true, etc. We need to allow for those.

13. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Get the items in the collection.
    +   *
    +   * @return array
    +   *   The items of the collection.
    +   */
    +  public function getItems() {
    +    return $this->items;
    +  }
    

    Wouldn't this be for iteration?

14. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Set the items list.
    +   *
    +   * @param array $items
    +   *   The items of the collection.
    +   */
    +  public function setItems($items) {
    +    $this->items = $items;
    +  }
    

    When would we want to set them all at once?

15. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Set the collection URI.
    +   *
    +   * @param string $uri
    +   *   The URI ("self") of the collection.
    +   */
    +  public function setUri($uri) {
    +    $this->uri = $uri;
    +  }
    

    Should this be a string, or a Url object? Url object would be consistent with the rest of core, but string would be more decoupled. Hm. I am not sure here.

16. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Sets a link URI for a given type.
    +   *
    +   * @param string $type
    +   *   The link relation type.
    +   * @param string $uri
    +   *   The link relation URI.
    +   */
    +  public function setLink($type, $uri) {
    +    $this->links[$type] = $uri;
    +  }
    +
    +  /**
    +   * Gets a link URI for a given type.
    +   *
    +   * @param string $type
    +   *   The link relation type.
    +   *
    +   * @return NULL|String
    +   *   The URI of the link relation type or NULL.
    +   */
    +  public function getLink($type) {
    +    return isset($this->links[$type]) ? $this->links[$type] : NULL;
    +  }
    

    While a collection won't have multiple first/last/prev/next links, realistically, it is 100% legal for there to be multiple links of the same relationship from one resource to another. We need to allow for that. Hence, this should really be addLink(), and we store potentially multiple links of the same relationship. and then getLink() returns an array of links, or [] if there are none of that type.

17. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Sets all link URIs.
    +   *
    +   * @param array $links
    +   *   Associative array of link relation types and URIs.
    +   */
    +  public function setLinks(array $links) {
    +    $this->links = $links;
    +  }
    

    Given the structure this could have (as above), this method strikes me as unweidly in practice.

    This is a similar situation as is being discussed for the PSR-7 HTTP standard interfaces in FIG, where we recently removed the set-multiple option to avoid the confusion that it brought.

18. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,215 @@
    +  /**
    +   * Returns true if (hypermedia) link relations have been added.
    +   *
    +   * @return bool
    +   *   True if link relations have been added.
    +   */
    +  public function hasLinks() {
    +    // $this->links should be an array, so no type checking is needed.
    +    return (bool) count($this->links);
    +  }
    

    I do not use case for this method. I can see hasLink($relationship), but not hasAnyLinksAtAll().

I re-rolled the patch and fixed a few of the things @Crell has commented (3, 4, 7 and 9) but it still needs work. I can continue working on it but need some guidance or feedback.

Provided an interdiff for easier review.

Ops! It seems that test fails due to change suggested by @Crell in point 9. Any thoughts?

Well, [] is not null. :-)

Changed assertion to check whether it's an empty array.

What assistance do you still need beyond the recommendations I provided in #89?

Patch needed a reroll.

#50 Open #1?
#89 Open all but 3,4,7,9

A reply to #89

1. #1

   The no foreach here seems suspect. If it actually works, why is this different than the way links work/are added?

   The relation is single: _embedded.item. The call adds the items so no need for a for loop right?

2. #2

   I'm not sure that we need denormalization of collections in the first place, in practice. Is there a use case?

   One such usecase could be the new `RSS` importing a HAL collection news feed. Not sure that is a good usecase. We do have #2350401: [PP-1] Collection denormalization

3. #3 solved
4. #4 solved
5. #5 + #6

   Strictly speaking, first/last have been removed from the standards since few if any implementers used them. That said, I'm fine with leaving them as I think implementers who didn't implement them are idiots. :-)

   The first link,though, should not specify a page parameter at all in order to be consistent with the way we use pagers elsewhere and to have a consistent URI.

   On ie a twitter timeline there is no real first/last same as watchdog log. There are always new items added in time.
   I think views should not add page# but item.id instead :p
   But let's do it the views way

6. #6 see #5
7. #7 solved
8. #8 good point. Using array_diff() the result should be empty.
9. #9 solved
10. #10
    Where is that title used? And even tested? I do not see it in the output for hal+json or json. Should it be the views title?
    Title is used on HTML and Feed. Feed has outputs description from setting in views.

    I guess we should output title and description too.

11. #11 see #10
12. #12.
    Keyed array of Link Objects.

    We use the keys previous, next, first, last to support pagination.

    Linked Objects are defined on https://tools.ietf.org/html/draft-kelly-json-hal-06#section-5

13. #13
    

    [edit]Removed code[/edit]

    Wouldn't this be for iteration?

    Please explain?

14. #14
    

    [edit]Removed code[/edit]

    When would we want to set them all at once?

    My guess is views adds all items at once.

15. #15
    

    Should this be a string, or a Url object? Url object would be consistent with the rest of core, but string would be more decoupled. Hm. I am not sure here.

    Collection->setUri is done in core/modules/rest/src/Plugin/views/style/Serializer.php:194 which seems views related. And is a string.

16. #16 agreed
    Taken from https://tools.ietf.org/html/draft-kelly-json-hal-06#section-4.1.1

    4.1.1. _links

    The reserved "_links" property is OPTIONAL.

    It is an object whose property names are link relation types (as
    defined by [RFC5988]) and values are either a Link Object or an array
    of Link Objects. The subject resource of these links is the Resource
    Object of which the containing "_links" object is a property.

17. #17 do you suggest to remove setLinks()? It is only used in tests which is odd.
18. #18 hasLinks() is only used in tests. Huh? See #17.

Patch needs a reroll from

commit 48c2e66fee7e788fe68cb792bc94bc283eddb60e
Author: Alex Pott
Date: Fri Feb 20 09:34:43 2015 +0000

Issue #2422113 by DickJohnson, emma.maria, mrjmd: Unpublished comments have lost their styles in D8

At Drupalcon LA, working on the re-roll

Rerolled the patch. I'm not 100% sure about the resolution in core/modules/rest/src/Tests/Views/StyleSerializerTest.php though.

@cyborg_572 thanks for the reroll. Let's see what the testbot thinks.

Failing tests are

Drupal\comment\Tests\Views\CommentRestExportTest
ExpandDrupal\rest\Tests\Views\StyleSerializerTest

I tried to fix StyleSerializerTest but failed. The exceptions for StyleSerializerTest are gone

This obviously need work :-/

Patch needed a reroll done from 29429bda910e398601fbace6a86f094000641ef4

I managed to come close. But /node is now taken over by rest/hal

http://drupal.d8 produces json NOT OK
http://drupal.d8?_format=json produces json OK
http://drupal.d8?_format=hal_json produces hal+json OK

Let's see what else breaks.

This needs a reroll either from #105 (guess that's the best option) or #107 (which was my bad effort)

Check the "How to test" from the issue summary too :-)

Stepping away from it for now. Will take more time than I have today to make sure that I prepare the reroll properly as there are quite a few conflicts. I will look back in next week and help out if I have some more time.

At that point in time, this is more of a 8.1.x feature to be honest.

Rerolled

Fixed CommentRestExportTest and StyleSerializerTest problems.

We need the route parameters on Serializer::getCollection() to fix CommentRestExportTest.

    // Inject the page into the canonical URI of the view.
    if ($this->view->getCurrentPage() > 0) {
      $uri = $this->urlGenerator->generateFromRoute($route_name, $view_base_url->getRouteParameters(), array('query' => array('page' => $this->view->getCurrentPage()), 'absolute' => TRUE));
    }
    else {
      $uri = $this->urlGenerator->generateFromRoute($route_name, $view_base_url->getRouteParameters(), array('absolute' => TRUE));
    }

1- Added a couple of TODOs to StyleSerializerTest

+    // @TODO needs review. the structure is not the same at all.
+    //$this->assertIdentical($actual_json['_embedded']['item'], $view_result, 'View result matches JSON returned from the view via HTTP GET');

Not sure if I can remove this assertion.

2- Not sure if we should use

$this->drupalGetWithFormat('test/serialize/field', 'json');

instead of

$this->drupalGetHalJson($actual_json_decoded['_links']['next']['href']);

I had problems using drupalGetHalJson() to obtain the hal_json format and using drupalGetWithFormat() it works as expected. Anyway we can refactor later.

Locally looks ok but let's take a look at the test results.

Removing debug() from tests. Locally the tests are working as expected ... Not sure where is the problem for the moment

Ok Let's try again.

Not sure why we have this result for totalItems using mini pager.

@marthinal,

I did some debug and found that this is coming from the mini pager plugin core/modules/views/src/Plugin/views/pager/Mini.php:84. Well, it says that ceil() will not fail but eventually it fails as per your attached screenshot. I believe, the reason behind is if a number has more digits than 13 it will automatically converted to a float value (see the references) also according to this S.O post.

So this can be solved as per the patch I am attaching by just using number_format. But from user end giving a big number as the last link is not a good way. I think in case if a view has a mini pager selected we should only show the "Previous" and "Next" links with "Self" as the HATEOAS pagination links. This should match the same behavior as per the website's views.

I am attaching a new patch based on my views above. Some tests might fail due to the changes that I've done. I'll have more updates on this after first bot review. However, feel free to review. :-)

REF:
https://en.wikipedia.org/wiki/Scientific_notation
http://php.net/manual/en/language.types.integer.php#language.types.integ...

Thanks!

This would allow to reduce the # of requests also. That's why this would also address part of #2339795: Add "REST modes", just like we have "view modes" and "form modes" AND depth argument to reduce # of requests.

Also related: #2662010: Add a derivative view for each comment field providing a REST export of comments for commented entities.

This looks extremely valuable.

1. +++ b/core/modules/hal/hal.services.yml
   @@ -30,3 +30,8 @@ services:
   +      - { name: normalizer, priority: 10 }
   

   Let's document why this priority is 10: which things should run before/after?

2. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   + * Converts the Drupal entity object structure to a HAL array structure.
   

   This needs to be updated. It's identical to \Drupal\hal\Normalizer\EntityReferenceItemNormalizer's documentation right now.

3. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   +   * Constructs an CollectionNormalizer object.
   

   Nit: s/an/a/

4. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   +   * @todo Implement denormalization once normalization has settled.
   ...
   +   * @todo Implement denormalization once normalization has settled.
   

   https://www.drupal.org/node/2350401 was opened for that, let's point to that issue URL.

5. +++ b/core/modules/hal/src/Normalizer/CollectionNormalizer.php
   @@ -0,0 +1,92 @@
   +  }
   +}
   

   Nit: \n in between.

6. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
   @@ -0,0 +1,199 @@
   + * Tests the CollectionNormalizer's normalize supports.
   

   "normalize supports"?

7. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
   @@ -0,0 +1,199 @@
   +   * Tests the supportsNormalization method.
   

   @covers supportsNormalization

8. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
   @@ -0,0 +1,199 @@
   +   * Tests the normalize method.
   ...
   +   * Tests the normalize method on pageable collection.
   

   @covers normalize

9. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
   @@ -0,0 +1,199 @@
   +    $collection->setLinks(array(
   ...
   +    return array(
   

   Nit: s/array()/[]/

10. +++ b/core/modules/hal/tests/src/Unit/CollectionNormalizerTest.php
    @@ -0,0 +1,199 @@
    +  }
    +}
    

    Nit: \n in between.

11. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
    @@ -0,0 +1,25 @@
    +interface CollectionLinkManagerInterface {
    

    Why does this live in rest and Collection lives in serialization?

12. +++ b/core/modules/rest/src/LinkManager/CollectionLinkManagerInterface.php
    @@ -0,0 +1,25 @@
    +  public function getCollectionItemRelation($collection_id);
    +}
    

    Nit: \n in between.

13. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -61,19 +78,38 @@ public static function create(ContainerInterface $container, array $configuratio
    -   * Constructs a Plugin object.
    +   * Serializer constructor.
    +   *
    +   * @param array $configuration
    +   *
    +   * @param string $plugin_id
    +   *
    +   * @param array $plugin_definition
    +   *
    +   * @param \Symfony\Component\Serializer\SerializerInterface $serializer
    +   *
    +   * @param array $serializer_formats
    +   *
    +   * @param UrlGeneratorInterface $url_generator
    +   *   The URL generator service.
    

    Need fixing

14. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -172,4 +188,97 @@ public function getCacheTags() {
    +   * @return \Drupal\serialization\Collection
    +   *   Collection object wrapping items/rows of the view.
    

    Hm… this is returning entity field data, plus URLs. Both of those things come with cacheability metadata. So this sounds like this object should implement \Drupal\Core\Cache\CacheableDependencyInterface.

15. +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
    @@ -172,4 +188,97 @@ public function getCacheTags() {
    +  public function getCollection() {
    +
    +    $display = $this->view->getDisplay();
    

    Nit: extraneous \n.

16. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -73,6 +76,182 @@ protected function setUp() {
    +   * Requests a Drupal path in HAL+JSON format, and JSON decodes the response.
    

    Misplaced. (And perhaps intended to be deleted?)

17. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -73,6 +76,182 @@ protected function setUp() {
    +  }
    +
    +
    +
    +  /**
    

    Far too much \n.

18. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -73,6 +76,182 @@ protected function setUp() {
    +  public function testSerializerFieldDisplayResponse() {
    +
    +    $view = Views::getView('test_serializer_display_field');
    

    Too much \n.

19. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -73,6 +76,182 @@ protected function setUp() {
    +    // @TODO needs review. the structure is not the same at all.
    +    //$this->assertIdentical($actual_json['_embedded']['item'], $view_result, 'View result matches JSON returned from the view via HTTP GET');
    ...
    +    // @TODO Remove? the structure is not the same at all.
    +    //$this->assertIdentical(Json::decode($actual_json)['_embedded']['item'], $view_result, 'View result matches JSON returned from the view via HTTP GET');
    +
    

    Looks like this needs to be resolved.

20. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -187,6 +377,110 @@ public function testSerializerResponses() {
    +   * @param ViewExecutable $view
    +   *   The View being executed.
    +   * @param null $page
    +   *
    +   * @return string
    

    Needs some love.

21. +++ b/core/modules/serialization/src/Collection.php
    @@ -0,0 +1,216 @@
    +  }
    +}
    
    +++ b/core/modules/serialization/tests/src/Unit/CollectionTest.php
    @@ -0,0 +1,110 @@
    +class CollectionTest extends UnitTestCase {
    +  /**
    ...
    +  }
    +}
    

    Nit: \n in between.

22. +++ b/core/modules/serialization/tests/src/Unit/CollectionTest.php
    @@ -0,0 +1,110 @@
    +    return array(
    

    Nit: array() -> []

What is not 100% clear to me yet is whether #121 is actually correct, i.e. whether this would allow you to for example retrieve a Node and have all the Taxonomy Terms it references be embedded in the response. i.e. removing the need to request each of the Taxonomy Terms separately.

@win-leers in [#2100637#121] you say "This ...". What are you referring to 'this'?

AFAIK this collection will not be a compound request having a node and it's comments/terms. I used collection to fetch all comments/terms for a node but that still requires 2 requests (1 for node and 1 for comments/terms by nid).

"This" = this issue/patch.

Assigning to myself to re-roll and fix as much as I can from @Wim Leers' review in #124

Ok actually just a re-roll for now.

I think some tests are going to fail but I am getting some weird time out issues when I try to run tests locally.

Un-assigning myself but I will look at this issue and the failing test tomorrow.

Update constructor call in SerializerTest to match updates.

Tried #133 out with Drupal 8.1 as the basis for a REST plugin to expose custom entity queries as resources. Works well. Title and Description are not surfaced in the normalizer.

Took a stab at an issue summary.

Points that struck me while summarizing:

• From the proposed resolution, not sure about "Use a link manager to determine the link relations from collection to item".
• In considering the overall summary and scope, I think this issue could be split between something that provides a Collection mechanism and the specifics of HAL support.

This issue facilitates functional use cases, developer learnability, and developer experience.

Regarding #36 item (19) called out in the summary, Collections should be agnostic about what kind of thing can be a member. Some facility to declare a type for collection members would help enforce consistency, but the flexibility should be there as a starting point.

In my own implementation, I've extended the Collection class as a RefinableCacheableDependency so I can bubble up entity metadata in custom REST resources. The serializers do not care.

Working on 8.3 reroll.

Since #133 did not apply, I attempted to use the interdiff tool but it also failed. Tested the attached reroll manually and saw collections for a simple HAL JSON view.

Unfortunately out of time to continue exploring this right now.

The fail in core/modules/rest/tests/src/Unit/Plugin/views/style/SerializerTest.php seems to be an insufficiency in how the Views object is being mocked.

Minor cleanups - removed unneeded file docbocks and an instanceof check in the CollectionNormalizer out of step with how other normalizers are set up.

It was pointed out in by @dawehner in #2803413: [PP-1] REST views: add HTTP Link pager that the total count should not be executed for mini pager so there is an ability to have some pager links without the overhead of a potentially complex count query. (Biggest previous discussion on mini pager was in #119 and #120)

Changes in that case will require mini_pager logic that is more prone to providing pager links that are not needed. I tend to think a next page link which gets no further results is better than no pager link at all, I don't recall how mini pager is handled as a UI element.

This patch is for 8.2.x branch, I use this in my current project, I just update if anyone needs it

I apply the patch for v8.2.3. it does not work. instead of the error below.

The website encountered an unexpected error. Please try again later.
TypeError: Argument 3 passed to Drupal\rest\LinkManager\LinkManager::__construct() must implement interface Drupal\rest\LinkManager\CollectionLinkManagerInterface, none given, called in /var/www/cylife/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 272 in Drupal\rest\LinkManager\LinkManager->__construct() (line 38 of core/modules/rest/src/LinkManager/LinkManager.php).

Drupal\rest\LinkManager\LinkManager->__construct(Object, Object) (Line: 272)
Drupal\Component\DependencyInjection\Container->createService(Array, 'rest.link_manager') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('rest.link_manager', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer.normalizer.file_entity.hal') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('serializer.normalizer.file_entity.hal', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 522)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'serializer') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('serializer', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'simple_oauth.repositories.access_token') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('simple_oauth.repositories.access_token', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'simple_oauth.server.resource_server') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('simple_oauth.server.resource_server', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'simple_oauth.authentication.simple_oauth') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('simple_oauth.authentication.simple_oauth', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 329)
Drupal\Component\DependencyInjection\Container->createService(Array, 'authentication_collector') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('authentication_collector', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'authentication') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('authentication', 1) (Line: 494)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 236)
Drupal\Component\DependencyInjection\Container->createService(Array, 'authentication_subscriber') (Line: 177)
Drupal\Component\DependencyInjection\Container->get('authentication_subscriber') (Line: 108)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.request', Object) (Line: 125)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@fengyun, that error is typical of applying a code change without clearing the cache or running update.php. I'd suggest testing the patch against a clean install.

The core committers and Views maintainers to make this issue a major task, since there is a lack of feature completeness for serialization here.

@xjm, yeah, by clean the cache, it works! Thanks. its seems the patch has not apply in drupal core 8.3.x-dev.

Just a re-roll #145 for now. Then will look at test failures.

Lately I’ve been thinking we should “won’t fix” this in favor of JSON API. The recommendation would become:

if you want collections, use Views, or use JSON API

I just think that it’s a LOT of effort to replicate all the collection handling that JSON API has in the crazy non-structured/non-strict world of @RestResource plugins: how will @RestResource plugins even be able to define their own collections? They're currently centered on individual resources. Or would we need to add "collection" resources?

The issue summary actually specifically calls out Views. But … REST Export views are not a generic collection solution: views only works for entities.

I'm not saying we MUST not do this. I'm saying that this requires a lot of work, that may be better spent elsewhere. Especially now that the JSON API module is in a usable shape, and has well-documented (because the spec defines it) methods for filtering, sorting and paging collections.

OTOH, the concept of a Collection value object is also something the JSON API module has. But it may be smarter to first focus on JSON API complying with the spec, and then (once we have JSON API in core) consider doing this issue. Both rest.module/serialization.module and jsonapi.module would then be able to use this common Collection value object concept. We would have two implementations using it, which is always better than just one: more validation, more certainty.

In other words: I think it's best to mark this issue postponed. Which it effectively has been since roughly #90 (the end of 2014) anyway.

I'd love to hear others' opinions. Again: I'm not saying this won't happen, I'm saying this is not the biggest win right now. It's been up for the taking for years for somebody to bring to completion, and it hasn't happened yet. It seems like we can do without for a bit longer, especially if JSON API can fill this gap for the time being.

I have to agree, a custom not flexible solution feels like a waste of time. On top of wasting time now we would have to support the feature in the future. If we want to do that, than I would strongly suggest to put it into a experimental module and drop it maybe after a while.

REST Export views are not a generic collection solution: views only works for entities.

That's simply not true. Views can query everything which is needed, like for example dblog entries.

That's simply not true. Views can query everything which is needed, like for example dblog entries.

I was thinking about that just after I posted #156. You're of course right. But of course, not everybody with custom data is going to be writing views integration. Perhaps we should be recommending that, and perhaps even require that if the future "collections" support in REST depends on views.

So yeah the question is, do we somehow want to recommend people how they can integrate themselves with our REST interfaces.
Would we recommend them to somehow magically integrate themselfes into JSONAPI?

I found Views too limiting but the Collection serializer very useful to facilitate uniform list structure with views output. For creating custom list resources I think this has value in core that would be very limited in contrib.

For the same reasons in #156, we should also postpone, and actually even close #2662010: Add a derivative view for each comment field providing a REST export of comments for commented entities. So, did that.

Note that since this blocks #2099281: [PP-1] REST views: pagination link relations, that issue is also affected.

This is a feature request, that has been open for years, and we just decided to postpone it even further. I think demoting to Normal priority makes sense.

I mean honestly, people can append the ?page query parameter in their client code and they probably much more likely do that than wanting to look at some page relation header/JSON.

Besides #2099281: [PP-1] REST views: pagination link relations (as mentioned in #164), this also blocks #2803413: [PP-1] REST views: add HTTP Link pager.

I guess the JSON API mentioned is #156 is actually RESTful Web Services API right?

I mean honestly, people can append the ?page query parameter in their client code and they probably much more likely do that than wanting to look at some page relation header/JSON.