Move rest module's "link manager" services to serialization module

+1 for the general idea.
There was also an issue to also get rid of serialization module and move it to core instead. Serialization module was initially just there, because core was less flexible in some place.

+1 for concept.

There was also an issue to also get rid of serialization module and move it to core instead. Serialization module was initially just there, because core was less flexible in some place.

Also +1.

Is this possible to do without breaking BC?

Ok. If you can provide a patch, I'd be happy to review it.

However, how is this going to interact with #2296029: Move Serialization module back into a core/lib component? Does that mean we just move this service directly into core? In other words, this could then be a precursor for #2296029: Move Serialization module back into a core/lib component, and a much smaller one at that! :)

Another consideration is the interaction between serialization and HTTP headers. E.g., suppose URLs derived by the existing Link managers should be used in Link headers? Should that be supported without requiring event subscribers to decode the serialized result?

This is related to the general class of behaviors that might change HTTP headers or cache tags based on processing objects.

This blocks #2473749: Remove rest as a dependency of hal.

+1 on this.

Alright, let's get this going. Here's an initial patch.

Still TODO:

1. we'll also need to migrate the link_domain setting from rest.settings to serialization.settings. This is an upgrade path. And it will need tests. (See \Drupal\serialization\LinkManager\LinkManagerBase::getLinkDomain().)
2. Figure out what to do about the mention of "REST" in \Drupal\serialization\LinkManager\RelationLinkManager::getRelationUri()
3. Figure out what to do about the mention of "REST" in \Drupal\serialization\LinkManager\TypeLinkManager::getTypeUri()

But before spending time on that, I wanted a +1 on this approach so far.

This blocks #2829746: Clean up \Drupal\jsonapi\LinkManager\LinkManager(Interface), which is currently contrib, but we're working on moving the JSON API module into core (see #2757967: API-first initiative). So tagging blocker.

This also looks good to me. Thanks Wim!

#15 + #16: thanks for the swift reviews! :)

#15: Oh :( Well, great to see both of our work validated then :) Thanks for the suggestion for points 2 + 3.

This addresses #13.2 and #13.3. It also ensures BC for hook_rest_(type|relation)_uri_alter().

Ugh. I only added a new file in #17. Here's the rest of the intended changes.

In #18, I also updated the default type/relation URIs from $this->getLinkDomain() . "/rest/…" to $this->getLinkDomain() . "/serialization/…". This breaks BC. So, reverting that change.

(#18 should also fail, because RestLinkManagerTest will catch this change. #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method would also detect this BC break. We really need #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method to land.)

So #18 made it so that the invocation of hook_rest_(type|relation)_uri_alter() hooks happens in the link manager classes that have been moved to the serialization module. In other words:
these hooks are now renamed to hook_serialization_(type|relation)_uri_alter()
but the old hooks are also still called, to ensure BC

What I had not yet done, was updating \Drupal\Tests\rest\Kernel\RestLinkManagerTest. It uses the rest_test module, which contains the rest_test_rest_type_uri_alter() and rest_test_rest_relation_uri_alter() hook implementations. Those should be moved to the serialization_test module. And the implementations in the rest_test module should be kept to test BC.

Some context for the changes in #20.

1. +++ b/core/modules/serialization/tests/src/Kernel/SerializationLinkManagerTest.php
   @@ -0,0 +1,74 @@
   +  public static $modules = ['serialization', 'serialization_test', 'rest_test', 'system'];
   

   Note this is no longer installing the rest module. No more dependence on rest, yay!

   Note that we still have rest_test there, but solely for testing BC of those alter hooks.

2. +++ b/core/modules/serialization/tests/src/Kernel/SerializationLinkManagerTest.php
   @@ -0,0 +1,74 @@
   +    // Now with optional context.
   +    $link = $type_manager->getTypeUri('node', 'page', ['serialization_test' => TRUE]);
   +    $this->assertSame($link, 'serialization_test_type');
   +    // Test BC: hook_rest_type_uri_alter().
   +    $link = $type_manager->getTypeUri('node', 'page', ['rest_test' => TRUE]);
   +    $this->assertSame($link, 'rest_test_type');
   

   The last two lines used to be originally in the place of those first two lines. Now the primary test must of course be the hook_serialization_* alter hooks. So we test that first, and then assert that BC works.

Aaand upgrade path plus tests. There is explicit test coverage for both possible cases:

1. serialization module is installed, rest module is NOT installed: test coverage ensures serialization.settings is created and contains link_domain: ~.
2. serialization and rest modules are both installed: test coverage ensures serialization.settings is created, with a link_domain value that is migrated from rest.settings, and link_domain is deleted from rest.settings

error: cannot apply binary patch to 'core/modules/serialization/tests/fixtures/update/drupal-8.rest-serialization_update_8301.php' without full index line
error: core/modules/serialization/tests/fixtures/update/drupal-8.rest-serialization_update_8301.php: patch does not apply
Patch Failed to apply

git + binary patches (or files with serialized PHP, which are strings, which git insists on interpreting as binary) == pain

Identical patch, but this time with --binary.

Yay! The upgrade path tests passed :)

This is now ready for final review. Just reading the interdiffs since #13 is sufficient!

RE: rest.settings: HAH! Great catch :) Fixed.

1. Yeah, we do this same thing elsewhere.
2. Well, we also do it in \Drupal\rest\Tests\Update\EntityResourcePermissionsUpdateTest and \Drupal\rest\Tests\Update\ResourceGranularityUpdateTest.

Change record created: https://www.drupal.org/node/2830467.

From the IS:

Leave the old classes behind, make them empty shells that extend from the ones in serialization and mark them as deprecated.

This is what #28 does.

Make hal module use the serialization module version.
Make hal module no longer depend on rest module.

We don't do this yet! So, doing that now.
Perhaps this belongs in a separate issue. But I'll leave that up to the committer to decide. If we want to do it separately, we'll just need to revert this interdiff :)

fail: [PHP Fatal error] Line 13 of core/modules/serialization/src/LinkManager/TypeLinkManager.php:
 Interface 'Drupal\rest\LinkManager\TypeLinkManagerInterface' not found

Hm…

It seems this is failing because of:

namespace Drupal\serialization\LinkManager;

use Drupal\rest\LinkManager\TypeLinkManagerInterface as BcTypeLinkManagerInterface;

class TypeLinkManager extends LinkManagerBase implements TypeLinkManagerInterface, BcTypeLinkManagerInterface {

i.e. the serialization module does not depend on the rest module, so it seems it's possible that the using of the old interface to guarantee BC doesn't actually work. Presumably the autoloader doesn't allow REST module's classes to be accessed when the REST module is not installed.

It just happens to be that #30 exposes that flaw.

Is that theory correct? If so, I don't know what the best way to fix this would be, without breaking BC. IOW, what @larowlan proposed in #8 would appear to be impossible:

Yep, we have to leave the old services/classes behind extending from the parent.

We did similar for BrowserTestBase, left a deprecated shell behind in simpletest module but moved the actual guts to core's Tests namespace

Moving things to lib/Drupal/Core is possible, because that's guaranteed to be always present. In the case of this issue/patch, we're moving it to the rest module, which is not guaranteed to be present.

So, do we want to move the link manager to lib/Drupal/Core instead (like I already proposed in #9)? That'd partially solve #2296029: Move Serialization module back into a core/lib component too. But then what about the link_domain setting? That could then no longer live in serialization.settings.

Moving to NR for feedback.

Here is a reroll to start with. Plus interdiff with the main significant change conflict.

Let's try again.

Whoops.

So, can we do this instead? Agree we cannot have rest classes/interfaces being depended on in serialization module. This seems like the only real solution?

+++ b/core/modules/rest/rest.services.yml
@@ -11,18 +11,22 @@ services:
-  #   serialization.link_manager.type instead.
+  #   serialization.link_manager instead.
...
-  #   serialization.link_manager.relation instead.
+  #   serialization.link_manager instead.

Yes, I failed at copying these comments properly.

#38 passes. #30 fails. So clearly @damiankloip's approach is feasible (#38), mine is not (#30).

#38 summarizes his approach like this:

So, can we do this instead? Agree we cannot have rest classes/interfaces being depended on in serialization module. This seems like the only real solution?

Effectively, this means:

1. +++ b/core/modules/serialization/src/LinkManager/LinkManager.php
   @@ -2,9 +2,7 @@
   -use Drupal\rest\LinkManager\LinkManagerInterface as BcLinkManagerInterface;
   -
   -class LinkManager implements LinkManagerInterface, BcLinkManagerInterface {
   +class LinkManager implements LinkManagerInterface {
   

   LinkManager was moved from rest to serialization, but it continued to implement the LinkManagerInterface from rest.

   This did not work for the reasons given in #32.

   Same for the RelationLinkManager and TypeLinkManager classes.

2. +++ b/core/modules/rest/src/LinkManager/LinkManager.php
   @@ -8,4 +8,4 @@
   -class LinkManager extends MovedLinkManager {}
   +class LinkManager extends MovedLinkManager implements LinkManagerInterface {}
   

   Consequently, the thin LinkManager class that was created in rest just to avoid breaking code is now itself responsible for implementing the corresponding original interface (the one in rest). That's what's being changed here.

3. +++ b/core/modules/rest/rest.services.yml
   @@ -11,18 +11,22 @@ services:
      rest.link_manager:
   -    alias: serialization.link_manager
   +    parent: serialization.link_manager
   +    class: Drupal\rest\LinkManager\LinkManager
   

   And as a final consequence, the rest.link_manager service can no longer point to the serialization.link_manager service, because it must continue to implement the interface defined in the rest module to not break BC. So it must not be an alias to another service, but instead it must point to the class highlighted in the previous point.

AFAICT this is solid. RTBC.

Thanks, @damiankloip!

Just marking NW instead of RTBC for the nits that @damiankloip already pointed out himself in #39, then this is RTBC.

Thanks, Wim! Thanks for the comprehensive write up. That seems 100% accurate :) We keep the REST services (as they extend the serialization ones now, but change the class only), and implement the original interfaces in the original classes only.

Whew! So glad to see this being ready :)

Also, bumping this to major, since it living in the rest module causes other modules to have to reimplement this. Without this being fixed, certain contrib modules can't move forward (see original report by @larowlan, plus the JSON API module, which has had to introduce its own link manager).

HEAD broke due to a broken Outside-In commit. This is still RTBC.

+++ b/core/modules/serialization/src/LinkManager/LinkManagerBase.php
@@ -44,6 +44,7 @@ public function setLinkDomain($domain) {
+      // @todo move this to serialization.settings, and provide an upgrade path.
       if ($domain = $this->configFactory->get('rest.settings')->get('link_domain')) {

This is the only @todo in this patch, and the upgrade path is already in the patch, so we should fix which config file we get this from, especially since the issue summary is about not requiring rest.module to be enabled. This patch does that.

Ticking credit boxes.

Removed unused use statements.

Pushed to 8.4.x and cherry picked to 8.3.x.

Normally, the change in #49 would be something I would want another person to review, but because we're only a few hours from the alpha1 code freeze, I took the liberty of committing #51 without that review. I hope it's ok.

However, related to that change:

+++ b/core/modules/serialization/tests/src/Kernel/SerializationLinkManagerTest.php
@@ -0,0 +1,74 @@
+  public static $modules = ['serialization', 'serialization_test', 'rest_test', 'system'];
...
+  public function testSerializationLinkManagersSetLinkDomain() {

A follow-up issue to improve this test to not enable rest.module (implicitly via rest_test) would be great.

Oops, #49 was indeed an oversight. Thanks!

A follow-up issue to improve this test to not enable rest.module (implicitly via rest_test) would be great.

Done, includes RTBC patch: #2848933: SerializationLinkManagerTest should not enable the rest_test module (because it implicitly enables the rest module).

And hah, we apparently already fixed #2473749: Remove rest as a dependency of hal as part of this patch! :)

This broke tests in entity_reference_revisions and possibly other modules as well.

https://www.drupal.org/pift-ci-job/589215

Was confused for a moment because the service still exists, but we were relying on the fact that hal depends on rest.module and didn't explicitly enable that in the test. The runtime code also only implicitly checks for rest, FileEntityServiceProvider (ERR has the same check without the comment):

    // Check for installed REST & HAL modules, as HAL requires REST.
    if (isset($modules['hal']) ) {

Not sure if changing module dependencies is an API change or not? :)

Was confused for a moment because the service still exists, but we were relying on the fact that hal depends on rest.module and didn't explicitly enable that in the test.

Glad to hear it's just that :)

The hal module depending on rest was always a flaw, as #2473749: Remove rest as a dependency of hal explained. Never rely on implicit module dependencies. Fortunately, it's an easy fix on your end — and for anybody else who may be using this :)

Added this to the CR. And also published the CR — apparently @effulgentsia forgot to publish it :)

Critical follow-up for this issue: #2854830: Move rest/serialization module's "link manager" services to HAL module.