Make Relation and Type domain configurable based on context

1. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -37,14 +42,23 @@ class RelationLinkManager implements RelationLinkManagerInterface{
   +    $relationDomain = \Drupal::config('rest.settings')->get('relation_domain');
   
   +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -27,6 +32,8 @@ class TypeLinkManager implements TypeLinkManagerInterface {
   +    $relationDomain = \Drupal::config('rest.settings')->get('relation_domain');
   

   Please inject the config factory instead of calling out to the global singleton

2. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -37,14 +42,23 @@ class RelationLinkManager implements RelationLinkManagerInterface{
   +      return \Drupal::service('unrouted_url_assembler')->assemble('base://' . ltrim($base, '/'), array('absolute' => TRUE));
   
   +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -41,8 +48,15 @@ public function __construct(CacheBackendInterface $cache) {
   +      return \Drupal::service('unrouted_url_assembler')->assemble('base://' . ltrim($base, '/'), array('absolute' => TRUE));
   

   Please inject the service instead of using the global singleton

I don't think this goes far enough. In default_content (http://github.com/larowlan/default_content) we have to override the type link manager and the relation link manager to provide an alternate domain so that content can be exported from one site and imported into another. However due to the design of this system this is a global override. So other modules might wish to use the actual url, or the url as configured by this patch, but cannot. I think a more flexible approach would be to inject the module handler and fire an alter hook on the generated links.

1. +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -27,6 +32,8 @@ class TypeLinkManager implements TypeLinkManagerInterface {
   +    $relationDomain = \Drupal::config('rest.settings')->get('relation_domain');
   +    $this->relationDomain = $relationDomain ? rtrim($relationDomain, '/') : null;
      }
    
      /**
   @@ -41,8 +48,15 @@ public function __construct(CacheBackendInterface $cache) {
   
   @@ -41,8 +48,15 @@ public function __construct(CacheBackendInterface $cache) {
       *   The URI that identifies this bundle.
       */
      public function getTypeUri($entity_type, $bundle) {
   -    // @todo Make the base path configurable.
   -    return _url("rest/type/$entity_type/$bundle", array('absolute' => TRUE));
   +    $base = "/rest/type/$entity_type/$bundle";
   +    if($this->relationDomain)
   +    {
   +      return $this->relationDomain . $base;
   +    }
   +    else
   +    {
   +      return \Drupal::service('unrouted_url_assembler')->assemble('base://' . ltrim($base, '/'), array('absolute' => TRUE));
   +    }
      }
    
      /**
   

   Code duplications. This suggests to use a Trait.

2. +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -41,8 +48,15 @@ public function __construct(CacheBackendInterface $cache) {
   +    $base = "/rest/type/$entity_type/$bundle";
   ...
   +      return $this->relationDomain . $base;
   ...
   +      return \Drupal::service('unrouted_url_assembler')->assemble('base://' . ltrim($base, '/'), array('absolute' => TRUE));
   

   Remove the ltrim by removing the '/' in $base.
   [edit]And add it @ appropriate places[/edit]

Remove the ltrim by removing the '/' in $base.

It might not always be there

@larowlan what did I miss? New code should be

+    $base = "rest/type/$entity_type/$bundle";
...
+      return $this->relationDomain . '/' . $base;
...
+      return \Drupal::service('unrouted_url_assembler')->assemble('base://' . $base, array('absolute' => TRUE));

New patch fixes #9 and #10 - passes the $context from the normalize/serialize around and then two new hooks to allow other modules to alter.

To-do: RelationLinkManagerInterface::getRelationInternalIds doesn't exist but is referenced in docblock of RelationLinkManager - quite a few missing public methods on interface.

Missed a spot

New issue summary detailing scope/changes/justifications

Filed #2355977: Code references RelationLinkManagerInterface::getRelationInternalIds but no such method for out-of-scope bug found

hook_rest_resource_alter seems an unrelated but very useful addition right?

Following the How to test steps the resulting _links.type.hrefand link names like http://x.y.z/rest/relation/node/article/uid are changed accordingly.

Reading the specs http://stateless.co/hal_specification.html I'm wondering whether curries wouldn't be a better solution. Is this issue blocking implementing curries? Or is this pretty close to implementing curries?

I guess this is RTBC apart from the added untested rest_resource_alter. Guess that should be in another issue.

The relation alter is for the links in embedded, ie entity reference fields

@larowlan I'm talking about hook_rest_resource_alter. That has no test, seems unrelated to this issue as I haven't found any usage either but is a nice addition so deserves its own issue.

My question about curries isn't answered either.

I'm talking about hook_rest_resource_alter. That has no test, seems unrelated to this issue as I haven't found any usage either

This is the title of the issue 'make relation domains configurable', although the solution also deals with the type domain too (so updated title) - note that both TypeLinkManager and RelationLinkManager have a todo to make the domain configurable - those todos are the point of this issue.
There is test implementation, test coverage and invocation as follows:

1. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -27,31 +29,50 @@ class RelationLinkManager implements RelationLinkManagerInterface{
   +    $this->moduleHandler->alter('rest_relation_uri', $uri, $context);
   

   Here is where it is invoked

2. +++ b/core/modules/rest/src/Tests/RestLinkManagerTest.php
   @@ -0,0 +1,55 @@
   +    /* @var \Drupal\rest\LinkManager\RelationLinkManagerInterface $relation_manager */
   +    $relation_manager = \Drupal::service('rest.link_manager.relation');
   +    $link = $relation_manager->getRelationUri('node', 'page', 'field_ref');
   +    $this->assertEqual($link, $base . 'rest/relation/node/page/field_ref');
   +    // Now with optional context.
   +    $link = $relation_manager->getRelationUri('node', 'page', 'foobar', ['rest_test' => TRUE]);
   +    $this->assertEqual($link, 'rest_test_relation');
   

   Here is the test coverage

3. +++ b/core/modules/rest/tests/modules/rest_test/rest_test.module
   @@ -0,0 +1,24 @@
   +/**
   + * Implements hook_rest_relation_uri_alter().
   + */
   +function rest_test_rest_relation_uri_alter(&$uri, $context = array()) {
   +  if (!empty($context['rest_test'])) {
   +    $uri = 'rest_test_relation';
   +  }
   +}
   

   Here is the test implementation

I've never heard of curies before, but after reading the linked document - it seems that they are very useful - but are more applicable to sub-resources or related resources? Eg node/1 could have curies for node/1/comments and node/1/revisions. Am I reading that correctly? I'm not sure that will work because the links aren't sub-resources, they're not in the same relative path. Eg relation URIs are rest/relation/$entity_type/$bundle/$field_name and type URIs are rest/type/$entity_type/$bundle whereas entity URIs are /entity/{entity_type}/{entity} - however what this patch does allow is for a contrib project to implement curies and refactor all of these URIs to suit. This isn't possible in HEAD as it stands.

Setting back for more eyes

(Forget the discussion about hook_rest_resource_alter above. (I had a weird .gitignore so assumed to much))

Darn. I missed the upgrade route. Are we now required to support beta upgrades? I guess so so 'Needs work'.

1. Install Drupal without the patch but with rest and hal modules enabled.
2. Run curl --user admin:admin --header 'Accept: application/hal+json' --request GET http://drupal.d8/node/1
3. Check the Relation ids in _links. That should start with http://drupal.d8/rest/relation
4. Now apply the patch and rerun curl. Note the links are changed into http://drupal.d8/rest/relation

Root cause is

$ drush @drupal.d8 config-get rest.settings.relation_domain
Config rest.settings.relation_domain does not exist in active configuration.

which should have a value.

Are we now required to support beta upgrades?

No, an upgrade path isn't supported yet - see #2341575: [meta] Provide a beta to beta/rc upgrade path

Based on feedback from #28 this is RTBC to me.

1. +++ b/core/modules/rest/rest.api.php
   @@ -29,5 +29,42 @@ function hook_rest_resource_alter(&$definitions) {
   + * @param array $context
   + *   The context from the serializer/normalizer operation.
   ...
   + * @param array $context
   + *   The context from the serializer/normalizer operation.
   

   It would be good to have at least an @see to where the context is set and what are the possible values here.

2. +++ b/core/modules/rest/rest.install
   @@ -0,0 +1,17 @@
   +function rest_install() {
   +  // Set the relation domain.
   +  \Drupal::config('rest.settings')
   +    ->set('relation_domain', Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString())
   +    ->save();
   +}
   

   I don't think this is going to be correct enough of the time - if you install on a dev server and then push config to live this automatically set value is likely to be incorrect. Therefore I think we should have a hook_requirements warning to advise the user to set it - or an automatic fallback. Discussed this with @larowlan on IRC and he agreed.

3. +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -9,6 +9,9 @@
   +use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
   

   Not used.

4. +++ b/core/modules/rest/src/LinkManager/TypeLinkManager.php
   @@ -36,20 +57,23 @@ public function __construct(CacheBackendInterface $cache) {
       * @return array
       *   The URI that identifies this bundle.
   

   This is a string not an array.

Fixes #30 1 » 4
In the case of 2 » if the config value isn't set we default to the <front> page URI.

But then takes it further - adds a new interface thus:

interface ConfigurableLinkManagerInterface {
  public function setLinkDomain($domain);
}

Makes RelationLinkManager, LinkManager and TypeLinkManager implement this interface by way of a new abstract base class BaseLinkManager which RelationLinkManager and TypeLinkManager extend from.

This new interface makes it easier for modules that need to call ->deserialize/normalize/denormalize/serialize themself as they can set the domain before hand and then set it back afterwards instead of relying on the hook.

High-level review.

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -211,13 +211,14 @@ protected function getEntityUri($entity) {
   +   * @param array $context
   ...
   +  protected function getTypedDataIds($types, $context = array()) {
   

   If required to be a string, could/should be type-hinted. Here and elsewhere.

2. +++ b/core/modules/rest/config/install/rest.settings.yml
   @@ -44,3 +44,7 @@ resources:
   +# Set the domain for rest relations.
   

   s/rest/REST/
   ?

   Here and elsewhere.

3. +++ b/core/modules/rest/src/LinkManager/ConfigurableLinkManagerInterface.php
   @@ -0,0 +1,25 @@
   +   * Sets the link domain used in constructing link URIs.
   

   Link domain or relation domain?

If required to be a string, could/should be type-hinted. Here and elsewhere.

Do you mean array?

Link domain or relation domain?

It applies to relation links and type links, so used the generic term link - noticed that many places in the patch refer to 'relation domain' so cleaned them up, including the config key name to use 'link domain' and link_domain consistently.

reroll

Code looks really good. Below are a few minor nit-picks:

1. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -27,31 +30,49 @@ class RelationLinkManager implements RelationLinkManagerInterface {
   +    $domain = $config_factory->get('rest.settings')->get('link_domain');
   +    if (empty($domain)) {
   +      $domain = Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString();
   +    }
   +    $this->linkDomain = rtrim($domain, '/');
   

   Since when are we using the short [] array syntax in core? I've got nothing against it, I'm just curious :-) (Edit: We could perhaps add this as an acceptable thing here: https://www.drupal.org/coding-standards#array)

2. +++ b/core/modules/rest/src/LinkManager/BaseLinkManager.php
   @@ -0,0 +1,29 @@
   +abstract class BaseLinkManager {
   
   +++ b/core/modules/rest/src/LinkManager/LinkManager.php
   @@ -38,22 +38,22 @@ public function __construct(TypeLinkManagerInterface $type_link_manager, Relatio
      }
   

   Don't we usually suffix Base on abstract classes, rather than prefixing?

3. +++ b/core/modules/rest/src/LinkManager/LinkManager.php
   @@ -38,22 +38,22 @@ public function __construct(TypeLinkManagerInterface $type_link_manager, Relatio
      /**
       * Implements \Drupal\rest\LinkManager\TypeLinkManagerInterface::getTypeUri().
       */
   -  public function getTypeUri($entity_type, $bundle) {
   -    return $this->typeLinkManager->getTypeUri($entity_type, $bundle);
   +  public function getTypeUri($entity_type, $bundle, $context = array()) {
   +    return $this->typeLinkManager->getTypeUri($entity_type, $bundle, $context);
      }
    
      /**
       * Implements \Drupal\rest\LinkManager\TypeLinkManagerInterface::getTypeInternalIds().
       */
   -  public function getTypeInternalIds($type_uri) {
   -    return $this->typeLinkManager->getTypeInternalIds($type_uri);
   +  public function getTypeInternalIds($type_uri, $context = array()) {
   +    return $this->typeLinkManager->getTypeInternalIds($type_uri, $context);
      }
    
      /**
       * Implements \Drupal\rest\LinkManager\RelationLinkManagerInterface::getRelationUri().
       */
   -  public function getRelationUri($entity_type, $bundle, $field_name) {
   -    return $this->relationLinkManager->getRelationUri($entity_type, $bundle, $field_name);
   +  public function getRelationUri($entity_type, $bundle, $field_name, $context = array()) {
   +    return $this->relationLinkManager->getRelationUri($entity_type, $bundle, $field_name, $context);
      }
   

   Since we're extending the interface with an optional parameter it could be worth to either A) add phpDoc on the LinkManager to explain what $context is/does or B) actually add the $context parameter to the LinkManagerInterface itself, include that in the interface's phpDoc and change these lines to /** {@inheritdoc} */.

1. we're using it for most new code - as long as it's consistent (ie don't mix the two)
2. Fixed
3. LinkManagerInterface extends TypeLinkManagerInterface and RelationLinkManagerInterface, both of which contain the phpdoc. The 'Implements Interface::method' docblocks are as per HEAD, I think changing them here is out of scope.

missed the file rename

3. LinkManagerInterface extends TypeLinkManagerInterface and RelationLinkManagerInterface, both of which contain the phpdoc. The 'Implements Interface::method' docblocks are as per HEAD, I think changing them here is out of scope.

Ok, that makes sense.

Patch is green and I think the code is really solid. Ready to go IMO.

looks like a random fail

twas a random fail

+++ b/core/modules/rest/rest.install
@@ -0,0 +1,17 @@
+  \Drupal::config('rest.settings')
+    ->set('link_domain', Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString())
+    ->save();

I thought we agreed to remove this? Especially give that this is what we fallback to anyway. See #30

Sure, sorry
Boldly sticks back to rtbc.

@alexpott - looks like we can't - it causes a recursive router rebuild in views serializer style plugin.

RuntimeException: Recursive router rebuild detected. in Drupal\Core\Routing\RouteBuilder->rebuild() (line 128 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Routing/RouteBuilder.php). Drupal\Core\Routing\RouteBuilder->rebuild() Drupal\Core\Routing\RouteBuilder->rebuildIfNeeded() Drupal\Core\Routing\RouteProvider->getRoutesByNames(Array) Drupal\Core\Routing\RouteProvider->getRouteByName('') Drupal\Core\Routing\UrlGenerator->getRoute('') Drupal\Core\Routing\UrlGenerator->generateFromRoute('', Array, Array) Drupal\Core\Url->toString() Drupal\rest\LinkManager\TypeLinkManager->__construct(Object, Object, Object) ReflectionClass->newInstanceArgs(Array) Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'rest.link_manager.type') Symfony\Component\DependencyInjection\ContainerBuilder->get('rest.link_manager.type', 1) Drupal\Core\DependencyInjection\ContainerBuilder->get('rest.link_manager.type', 1) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Object) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array) Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'rest.link_manager') Symfony\Component\DependencyInjection\ContainerBuilder->get('rest.link_manager', 1) Drupal\Core\DependencyInjection\ContainerBuilder->get('rest.link_manager', 1) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Object) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array) Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'serializer.normalizer.file_entity.hal') Symfony\Component\DependencyInjection\ContainerBuilder->get('serializer.normalizer.file_entity.hal', 1) Drupal\Core\DependencyInjection\ContainerBuilder->get('serializer.normalizer.file_entity.hal', 1) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Object) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array) Symfony\Component\DependencyInjection\ContainerBuilder->resolveServices(Array) Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object, 'serializer') Symfony\Component\DependencyInjection\ContainerBuilder->get('serializer', 1) Drupal\Core\DependencyInjection\ContainerBuilder->get('serializer') Drupal\rest\Plugin\views\style\Serializer::create(Object, Array, 'serializer', Array) Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('serializer', Array) Drupal\Component\Plugin\PluginManagerBase->createInstance('serializer') Drupal\views\Plugin\views\display\DisplayPluginBase->getPlugin('style') Drupal\rest\Plugin\views\display\RestExport->collectRoutes(Object) Drupal\views\EventSubscriber\RouteSubscriber->routes() call_user_func(Array) Drupal\Core\Routing\RouteBuilder->rebuild() drupal_flush_all_caches() Drupal\simpletest\WebTestBase->resetAll() Drupal\views\Tests\ViewTestBase->enableViewsTestModule() Drupal\rest\Tests\Views\StyleSerializerTest->setUp() Drupal\simpletest\TestBase->run() simpletest_script_run_one_test('1136', 'Drupal\rest\Tests\Views\StyleSerializerTest') 

Back to RTBC for @alexpott to review.

Well doesn't that mean that the fallback is not going work then?

Re-roll and solves the recursion issue, removes the hook_install as requested.
Also, I think this is major - hardcoded is not the Drupal way.

Backed out the hook_install but not the test :)

back to rtbc anyone?

reroll

1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
   index d841ddf..6ec22da 100644
   --- a/core/modules/hal/src/Tests/FileNormalizeTest.php
   
   --- a/core/modules/hal/src/Tests/FileNormalizeTest.php
   +++ b/core/modules/hal/src/Tests/FileNormalizeTest.php
   
   +++ b/core/modules/hal/src/Tests/FileNormalizeTest.php
   +++ b/core/modules/hal/src/Tests/FileNormalizeTest.php
   @@ -39,7 +39,7 @@ protected function setUp() {
   
   @@ -39,7 +39,7 @@ protected function setUp() {
        $this->installEntitySchema('file');
    
        $entity_manager = \Drupal::entityManager();
   -    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default')), new RelationLinkManager(new MemoryBackend('default'), $entity_manager));
   +    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default'), \Drupal::moduleHandler(), \Drupal::service('config.factory')), new RelationLinkManager(new MemoryBackend('default'), $entity_manager, \Drupal::moduleHandler(), \Drupal::service('config.factory')));
    
        // Set up the mock serializer.
        $normalizers = array(
   
   +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
   @@ -134,7 +135,7 @@ protected function setUp() {
   -    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default')), new RelationLinkManager(new MemoryBackend('default'), $entity_manager));
   +    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default'), \Drupal::moduleHandler(), \Drupal::service('config.factory')), new RelationLinkManager(new MemoryBackend('default'), $entity_manager, \Drupal::moduleHandler(), \Drupal::service('config.factory')));
   

   Its quite annoying that these kernel tests don't get services from the container.

2. +++ b/core/modules/rest/src/LinkManager/ConfigurableLinkManagerInterface.php
   @@ -0,0 +1,25 @@
   +/**
   + * Defines an interface for a link manager with a configurable domain.
   + */
   +interface ConfigurableLinkManagerInterface {
   

   Usually we don't use composable interface but rather extend existing ones, is there a reason why we don't extend? If you don't extend typehinting gets a little bit tricky.

3. +++ b/core/modules/rest/src/LinkManager/LinkManagerBase.php
   @@ -0,0 +1,44 @@
   +      $domain = Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString();
   +      $this->linkDomain = rtrim($domain, '/');
   

   So you wanted to get the base url + base path?

4. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -27,31 +29,46 @@ class RelationLinkManager implements RelationLinkManagerInterface {
   +    $domain = $config_factory->get('rest.settings')->get('link_domain');
   +    $this->linkDomain = rtrim($domain, '/');
   

   It would be great to avoid calling to the config factory on constructor time, given that this could always lead somehow by accident to circular dependencies or simply slow down things. You already have a method: getLinkDomain, so you can call it there.

Re-roll

1. yep
2. RelationLinkManagerInterface extends ConfigurableLinkManagerInterface, as does TypeLinkManagerInterface - I think this composition is pre-existing in rest module where we have the common LinkManager which wraps TypeLinkManager and RelationLinkManager, but not a common base class
3. Injected request stack instead, much cleaner
4. fixed

Test fixes

This has been rtbc three times already, any takers for number four?

Yeah sure.

Do we need a CR? Maybe there are other affect contrib or custom modules?

+++ b/core/modules/rest/src/LinkManager/ConfigurableLinkManagerInterface.php
@@ -0,0 +1,25 @@
+  /**
+   * Sets the link domain used in constructing link URIs.
+   *
+   * @param string $domain
+   *   The link domain to use for constructing link URIs.
+   *
+   * @return $this
+   */
+  public function setLinkDomain($domain);

+++ b/core/modules/rest/src/LinkManager/LinkManagerBase.php
@@ -0,0 +1,63 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function setLinkDomain($domain) {
+    $this->linkDomain = rtrim($domain, '/');
+  }

Let's make this fluent and return $this.

Fixed #67

Draft CR is https://www.drupal.org/node/2484399

Back to RTBC.

Committed 20add38 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

diff --git a/core/modules/rest/src/Tests/ReadTest.php b/core/modules/rest/src/Tests/ReadTest.php
index 315bbd7..d5f2fc1 100644
--- a/core/modules/rest/src/Tests/ReadTest.php
+++ b/core/modules/rest/src/Tests/ReadTest.php
@@ -8,7 +8,6 @@
 namespace Drupal\rest\Tests;
 
 use Drupal\Component\Serialization\Json;
-use Drupal\Core\Url;
 use Drupal\rest\Tests\RESTTestBase;
 
 /**

Removed unused use.